Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC: Add parameter to addmultisigaddress / createmultisig to sort public keys #8751

Closed
wants to merge 5 commits into from

Conversation

afk11
Copy link
Contributor

@afk11 afk11 commented Sep 17, 2016

I figured it may be useful for these RPC methods to allow sorting public keys (BIP67) The PR adds a new boolean to createmultisig / addmultisigaddress at the end of their parameter list. By default, this is set to false to avoid a BC break.

I added a RPC test file sort_multisig.py for testing createmultisig. Tests for addmultisigaddress went in wallet-accounts.py.

Note: Code to check whether sorting is desired had to be replicated in both RPC methods (not in _createmultisig_redeemScript) because addmultisigaddress already takes a parameter at position 3.

@dcousens
Copy link
Contributor

dcousens commented Sep 18, 2016

concept ACK

@laanwj
Copy link
Member

laanwj commented Sep 22, 2016

Concept ACK, although I really don't like multiple-optional-positional-boolean APIs. Wish we switched to named arguments any day.

One nit: the RPC help should mention BIP67 by name.

@MarcoFalke
Copy link
Member

MarcoFalke commented Nov 8, 2016

Needs rebase

@afk11 afk11 force-pushed the sort-multisigs branch 2 times, most recently from 0c9f570 to 7d7a647 Compare Nov 8, 2016
@afk11
Copy link
Contributor Author

afk11 commented Nov 8, 2016

@MarcoFalke thanks, done. @laanwj I should have mentioned, nits addressed.

One travis run failed due to the compactblocks RPC test.

@ryanofsky
Copy link
Contributor

ryanofsky commented Nov 8, 2016

I can't see anything on travis right now (503 errors), but the compactblocks error is probably just the spurious #8842 / #9058 failures.

class SortMultisigTest(BitcoinTestFramework):
def __init__(self):
super().__init__()
self.num_nodes = 4
Copy link
Member

@MarcoFalke MarcoFalke Nov 8, 2016

Choose a reason for hiding this comment

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

Nit: A single node should be enough?

@MarcoFalke
Copy link
Member

MarcoFalke commented Nov 8, 2016

Concept ACK 7d7a64726991ff087cb8125e0c7277173a688dc7

src/rpc/misc.cpp Outdated
@@ -293,6 +294,7 @@ UniValue createmultisig(const JSONRPCRequest& request)
" \"key\" (string) bitcoin address or hex-encoded public key\n"
" ,...\n"
" ]\n"
"3. \"fSort\" (bool, optional) Whether to sort public keys according to BIP67. Default setting is false.\n"
Copy link
Member

@luke-jr luke-jr Nov 24, 2016

Choose a reason for hiding this comment

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

Is it a string or a boolean?

Copy link
Contributor Author

@afk11 afk11 Nov 24, 2016

Choose a reason for hiding this comment

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

It should be a boolean. Just observed they aren't usually quoted in RPC output, fixing now.

@afk11
Copy link
Contributor Author

afk11 commented Nov 24, 2016

I probably shouldn't have squashed @MarcoFalke, I'm sorry for rebasing out the commit you reviewed. The only thing to change this time was the removal of "'s from the RPC help message.

Copy link
Member

@luke-jr luke-jr left a comment

Code looks reasonably correct, just a few nits. Did not verify tests.

src/rpc/misc.cpp Outdated
"2. \"keys\" (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
" [\n"
" \"key\" (string) bitcoin address or hex-encoded public key\n"
" ,...\n"
" ]\n"
"3. fSort (bool, optional) Whether to sort public keys according to BIP67. Default setting is false.\n"
Copy link
Member

@luke-jr luke-jr Nov 25, 2016

Choose a reason for hiding this comment

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

Rather have a named-options Object interface here.

vEncoded.resize(keys.size());
BOOST_FOREACH(const CPubKey& key, keys) {
vEncoded[nEncoded++] = ToByteVector(key);
}
Copy link
Member

@luke-jr luke-jr Nov 25, 2016

Choose a reason for hiding this comment

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

Seems like the loop would be better as:

for (size_t n = 0; n < keys.size(); ++n) {
    vEncoded[n] = ToByteVector(keys[n]);
}

CScript script;
int nEncoded = 0;
std::vector<std::vector<unsigned char>> vEncoded;
vEncoded.resize(keys.size());
Copy link
Member

@luke-jr luke-jr Nov 25, 2016

Choose a reason for hiding this comment

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

Wouldn't it be better to reserve and then emplace_back?

}

if (fSorted) {
std::sort(vEncoded.begin(), vEncoded.end());
Copy link
Member

@luke-jr luke-jr Nov 25, 2016

Choose a reason for hiding this comment

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

I think this should do what BIP 67 requires, but someone more familiar with C++ and its locale support (or lack thereof) should probably confirm.

@@ -78,7 +78,7 @@ bool ExtractDestinations(const CScript& scriptPubKey, txnouttype& typeRet, std::

CScript GetScriptForDestination(const CTxDestination& dest);
CScript GetScriptForRawPubKey(const CPubKey& pubkey);
CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys);
CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys, bool fSorted);
Copy link
Member

@luke-jr luke-jr Nov 25, 2016

Choose a reason for hiding this comment

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

Maybe default fSorted to false here rather than modify all the tests?

"2. \"keysobject\" (string, required) A json array of bitcoin addresses or hex-encoded public keys\n"
" [\n"
" \"address\" (string) bitcoin address or hex-encoded public key\n"
" ...,\n"
" ]\n"
"3. \"account\" (string, optional) DEPRECATED. An account to assign the addresses to.\n"
"4. fSort (bool, optional) Whether to sort public keys according to BIP67. Default setting is false.\n"
Copy link
Member

@luke-jr luke-jr Nov 25, 2016

Choose a reason for hiding this comment

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

As before, rather turn param 3 into an options Object.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
…g_redeemScript to allow sorting of public keys (BIP67)

addmultisig/createmultisig RPC documentation: Remove stray quotes from fSort parameter

Github-Pull: bitcoin#8751
Rebased-From: 7439562df15db643253c6ac734aeaa0ef66c0c88
@luke-jr
Copy link
Member

luke-jr commented Dec 21, 2016

Should update doc/bips.md also.

@luke-jr
Copy link
Member

luke-jr commented Feb 4, 2017

I rebased and addressed all the nits; pushed this to luke-jr/sort-multisigs

@afk11 Are you still maintaining this? Can you pull my changes?

git checkout sort-multisigs
git fetch git://github.com/luke-jr/bitcoin sort-multisigs
git reset --hard FETCH_HEAD
git push ...

@afk11
Copy link
Contributor Author

afk11 commented Feb 6, 2017

Sorry, yep I can pull these!

I wanted to wait until named parameters was merged before hand, so I could avoid adding a positional parameter before the accounts parameters were changed

I'll look at this in the next day or so (away from internet atm) wanna finish this up

@afk11 afk11 force-pushed the sort-multisigs branch 2 times, most recently from 8d5106b to 71c4420 Compare Mar 8, 2017
@afk11
Copy link
Contributor Author

afk11 commented Mar 8, 2017

Merged commits and rebased. Apologies for the delay!

@afk11
Copy link
Contributor Author

afk11 commented Mar 8, 2017

The Apple build failed because the job time exceeded the maximum :/

@afk11 afk11 force-pushed the sort-multisigs branch 3 times, most recently from ebca39a to e00d003 Compare Mar 14, 2017
@afk11
Copy link
Contributor Author

afk11 commented Mar 14, 2017

Rebased

@jnewbery
Copy link
Member

jnewbery commented Jun 15, 2017

Perhaps I'm missing something, but I don't see the need for this. The addmultisigaddress RPC creates the multisig script with the keys in the order provided. Why not just have the client provide keys in sorted order if you want the script to be BIP-67 compliant?

It doesn't look like this PR is enforcing that the provided keys are compressed, so even with this PR, there are still expectations placed on the client.

@afk11
Copy link
Contributor Author

afk11 commented Jun 16, 2017

I think if developers are already committing to using the RPC to make a multisig script, making it easier to produce the same representation is more important than not.

You are correct the PR as it stands doesn't validate it.. fixing this now.

@jnewbery
Copy link
Member

jnewbery commented Jun 16, 2017

I'm still a weak concept NACK for this. I don't agree that we should add complexity to the server when the same outcome can be achieved by simply running a sort() function on the client before calling the RPC. Sometimes there's good reason to add that complexity to the server - see for example #9991 which adds a filter to save significantly on bandwidth and server resources. In this case I don't see the benefit.

Sorry if that sounds negative - I think there needs to be some bar for adding new RPCs and arguments to avoid bloat.

However, if I'm wrong and there's widespread consensus that this is useful functionality and should be merged, can I at least ask that you use named arguments instead of an Options object? There's really no need for Options objects in RPC calls since named args were added in #8811.

@afk11
Copy link
Contributor Author

afk11 commented Jun 19, 2017

Both RPC methods take an options object for this (sorry, the PR description wasn't updated with this)
https://github.com/bitcoin/bitcoin/pull/8751/files#diff-ad6efdc354b57bd1fa29fc3abb6e2872R353
https://github.com/bitcoin/bitcoin/pull/8751/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR1050

I appreciate where you are coming from and agree that most people can probably sort themselves, but they could also build a multisig script out of the keys and m/n. It's been a while since I've even used the RPC, but remember well the time when I didn't have a bitcoin library to do it all.

I think it's worth including since once they continue using the flag, requests which mistakenly use the wrong order will reproduce the same redeem script (instead of always having a stateful order of public keys), and likewise with libraries that support it.

CScript script;
std::vector<std::vector<unsigned char>> vEncoded;
vEncoded.reserve(keys.size());
BOOST_FOREACH(const CPubKey& key, keys) {
Copy link
Member

@luke-jr luke-jr Aug 18, 2017

Choose a reason for hiding this comment

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

We're not using BOOST_FOREACH anymore I think. (Also below)

@luke-jr
Copy link
Member

luke-jr commented Aug 21, 2017

Rebased and squashed a bit.

git checkout sort-multisigs && git fetch git://github.com/luke-jr/bitcoin sort-multisigs && git reset --hard FETCH_HEAD && git push ...

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Sep 29, 2017

Concept ACK. Care to rebase?

@afk11
Copy link
Contributor Author

afk11 commented Sep 30, 2017

Rebased, sorry for the delay. Updated to check that keys are compressed before allowing sorting, and added more tests for this.

Updated the docs/bips.md document to mention 0.15.1 instead of 0.15.0 (let me know whatever's best for this)

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Oct 2, 2017

Hmm, hate to reopen it, but now that we do actually have named arguments, could you rever to just adding a new boolean argument? options objects are just redundant now, and having options alias account in addmultisigaddress is just gross. Everything else seems fine at first glance.

@jnewbery I'd generally agree with you, but, at least in principal, I think BIP67 is worth it.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017
…sig methods

Also add accounts parameter to vRPCConvertParams (required by RPC mappings test)

Github-Pull: bitcoin#8751
Rebased-From: 4833935
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017
sort_multisig test: check uncompressed keys are disallowed
sort_multisig: add test demonstrating sorting
wallet-accounts: test addmultisigaddress fails if sort=true and (wallet) address is uncompressed

Github-Pull: bitcoin#8751
Rebased-From: 50e2ff5
afk11 and others added 5 commits Dec 2, 2017
…g_redeemScript to allow sorting of public keys (BIP67)

addmultisig/createmultisig RPC documentation: Remove stray quotes from fSort parameter
sort_multisig test: check uncompressed keys are disallowed
sort_multisig: add test demonstrating sorting
wallet-accounts: test addmultisigaddress fails if sort=true and (wallet) address is uncompressed
@afk11
Copy link
Contributor Author

afk11 commented Dec 2, 2017

@TheBlueMatt that's fine, revised the PR now.

I missed the boat again for v0.15.1, suggestions for a release to mention in bips.md?

@luke-jr
Copy link
Member

luke-jr commented Mar 1, 2018

Hmm, hate to reopen it, but now that we do actually have named arguments, could you rever to just adding a new boolean argument? options objects are just redundant now, and having options alias account in addmultisigaddress is just gross. Everything else seems fine at first glance.

Strongly disagree. Named arguments is not a reason to have a terrible positional arguments API. Uncommon options should go through an options argument when positional arguments are used.

The account alias is merely for backward compatibility.

{
std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" )\n"
std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" ) ( sort )\n"
Copy link
Member

@luke-jr luke-jr Mar 2, 2018

Choose a reason for hiding this comment

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

Please switch back to an options object for this.

"\nAdd a nrequired-to-sign multisignature address to the wallet. Requires a new wallet backup.\n"

Copy link
Member

@luke-jr luke-jr Mar 2, 2018

Choose a reason for hiding this comment

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

This blank line won't be in the actual help, so has no purpose here.

@laanwj
Copy link
Member

laanwj commented Aug 31, 2018

Closing and putting "up for grabs" label

@laanwj laanwj closed this Aug 31, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants