Skip to content

Conversation

julianlen
Copy link
Contributor

Added the auxiliary functions to create the custom redeem script.

pubkeys = new ArrayList<BtcECKey>(pubkeys);
Collections.sort(pubkeys, BtcECKey.PUBKEY_COMPARATOR);
if (pubkeys == null) {
throw new NullPointerException("PubKeys is null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new NullPointerException("PubKeys is null");
throw new NullPointerException("PubKeys are null");

:p

Copy link
Contributor

@julia-zack julia-zack Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not throw an NPE but an IllegalArgumentException here. Throwing an NPE does not differ from actual behavior

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @julia-zack

}

/**
* Creates a custom redeem script with given public keys and threshold. Given public keys will be placed in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Creates a custom redeem script with given public keys and threshold. Given public keys will be placed in
* Creates a custom multisig redeem script with given public keys and threshold. Given public keys will be placed in

*/
public static Script createCustomRedeemScript(int threshold, List<BtcECKey> pubkeys) {
if (pubkeys == null) {
throw new NullPointerException("PubKeys is null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

* Creates a custom redeem script with given public keys and threshold. Given public keys will be placed in
* redeem script in the lexicographical sorting order.
*/
public static Script createCustomRedeemScript(int threshold, List<BtcECKey> pubkeys) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static Script createCustomRedeemScript(int threshold, List<BtcECKey> pubkeys) {
public static Script createCustomMultisigRedeemScript(int threshold, List<BtcECKey> pubkeys) {

}

private static Script createCustomMultiSigOutputScript(int threshold, List<BtcECKey> pubkeys) {
checkArgument(threshold > 0, "Default threshold must be greater than 0");
Copy link
Contributor

@julia-zack julia-zack Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
checkArgument(threshold > 0, "Default threshold must be greater than 0");
checkArgument(threshold > 0, "threshold must be greater than 0");

default word is no needed in this context, since is to differentiate emergency and default keys

private static Script createCustomMultiSigOutputScript(int threshold, List<BtcECKey> pubkeys) {
checkArgument(threshold > 0, "Default threshold must be greater than 0");
checkArgument(threshold <= pubkeys.size(), "The number of default public keys must be greater or equal than default threshold");
checkArgument(pubkeys.size() <= MAX_PUBLIC_KEYS_FOR_CUSTOM_REDEEM_SCRIPT, "The protocol only supports 66 signers");
Copy link
Contributor

@julia-zack julia-zack Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

careful here. The 62 limit comes from the poc we did.
But we should be more specific about this constant, since it does not depend on the number of pub keys itself. Lets remove this check for now and create a task to address it, wdyt?

Comment on lines +470 to +473
BtcECKey lastKey = pubkeys.get(pubkeys.size() - 1);
builder.data(lastKey.getPubKey());
builder.op(OP_CHECKSIG);
for (int i = pubkeys.size() - 2; i >= 0; i --) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets define pubkeys.size() as a variable since we are using it in two different places

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a little comment just mentioning we will set the keys in the reverse order could be good

* protocol at a lower level.</p>
*/
public class ScriptBuilder {
public static final int MAX_PUBLIC_KEYS_FOR_CUSTOM_REDEEM_SCRIPT = 62;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why public? It's been used only in this class.

pubkeys = new ArrayList<BtcECKey>(pubkeys);
Collections.sort(pubkeys, BtcECKey.PUBKEY_COMPARATOR);
if (pubkeys == null) {
throw new NullPointerException("PubKeys is null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @julia-zack

* Creates a custom redeem script with given public keys and threshold. Given public keys will be placed in
* redeem script in the lexicographical sorting order.
*/
public static Script createCustomRedeemScript(int threshold, List<BtcECKey> pubkeys) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants