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

pull afk11 wants to merge 5 commits into bitcoin:master from afk11:sort-multisigs changing 9 files +169 −21
  1. afk11 commented at 3:44 pm on September 17, 2016: contributor

    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.

  2. afk11 force-pushed on Sep 17, 2016
  3. dcousens commented at 7:46 am on September 18, 2016: contributor
    concept ACK
  4. fanquake added the label RPC/REST/ZMQ on Sep 18, 2016
  5. laanwj commented at 4:58 am on September 22, 2016: member

    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.

  6. afk11 force-pushed on Oct 4, 2016
  7. MarcoFalke commented at 10:30 am on November 8, 2016: member
    Needs rebase
  8. afk11 force-pushed on Nov 8, 2016
  9. afk11 force-pushed on Nov 8, 2016
  10. afk11 commented at 3:53 pm on November 8, 2016: contributor

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

    One travis run failed due to the compactblocks RPC test.

  11. ryanofsky commented at 4:11 pm on November 8, 2016: member
    I can’t see anything on travis right now (503 errors), but the compactblocks error is probably just the spurious #8842 / #9058 failures.
  12. in qa/rpc-tests/sort_multisig.py: in 7d7a647269 outdated 
     9+from test_framework.util import *
10+
11+class SortMultisigTest(BitcoinTestFramework):
12+    def __init__(self):
13+        super().__init__()
14+        self.num_nodes = 4
    MarcoFalke commented at 5:24 pm on November 8, 2016:
    Nit: A single node should be enough?
  13. MarcoFalke commented at 5:26 pm on November 8, 2016: member
    Concept ACK 7d7a64726991ff087cb8125e0c7277173a688dc7
  14. in src/rpc/misc.cpp: in 7d7a647269 outdated 
    293@@ -293,6 +294,7 @@ UniValue createmultisig(const JSONRPCRequest& request)
294             "       \"key\"    (string) bitcoin address or hex-encoded public key\n"
295             "       ,...\n"
296             "     ]\n"
297+            "3. \"fSort\"      (bool, optional) Whether to sort public keys according to BIP67. Default setting is false.\n"
    luke-jr commented at 6:08 am on November 24, 2016:
    Is it a string or a boolean?
    afk11 commented at 10:04 am on November 24, 2016:
    It should be a boolean. Just observed they aren’t usually quoted in RPC output, fixing now.
  15. afk11 force-pushed on Nov 24, 2016
  16. afk11 commented at 10:25 am on November 24, 2016: contributor
    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.
  17. in src/rpc/misc.cpp: in 7439562df1 outdated 
    295             "2. \"keys\"       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
296             "     [\n"
297             "       \"key\"    (string) bitcoin address or hex-encoded public key\n"
298             "       ,...\n"
299             "     ]\n"
300+            "3. fSort        (bool, optional) Whether to sort public keys according to BIP67. Default setting is false.\n"
    luke-jr commented at 8:57 am on November 25, 2016:
    Rather have a named-options Object interface here.
  18. in src/script/standard.cpp: in 7439562df1 outdated 
    297+    int nEncoded = 0;
298+    std::vector<std::vector<unsigned char>> vEncoded;
299+    vEncoded.resize(keys.size());
300+    BOOST_FOREACH(const CPubKey& key, keys) {
301+        vEncoded[nEncoded++] = ToByteVector(key);
302+    }
    luke-jr commented at 8:59 am on November 25, 2016:

    Seems like the loop would be better as:

    0for (size_t n = 0; n < keys.size(); ++n) {
1    vEncoded[n] = ToByteVector(keys[n]);
2}
  19. in src/script/standard.cpp: in 7439562df1 outdated 
    294+CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys, bool fSorted)
295 {
296-    CScript script;
297+    int nEncoded = 0;
298+    std::vector<std::vector<unsigned char>> vEncoded;
299+    vEncoded.resize(keys.size());
    luke-jr commented at 9:00 am on November 25, 2016:
    Wouldn’t it be better to reserve and then emplace_back?
  20. in src/script/standard.cpp: in 7439562df1 outdated 
    300+    BOOST_FOREACH(const CPubKey& key, keys) {
301+        vEncoded[nEncoded++] = ToByteVector(key);
302+    }
303+
304+    if (fSorted) {
305+        std::sort(vEncoded.begin(), vEncoded.end());
    luke-jr commented at 9:02 am on November 25, 2016:
    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.
  21. in src/script/standard.h: in 7439562df1 outdated 
    77@@ -78,7 +78,7 @@ bool ExtractDestinations(const CScript& scriptPubKey, txnouttype& typeRet, std::
78 
79 CScript GetScriptForDestination(const CTxDestination& dest);
80 CScript GetScriptForRawPubKey(const CPubKey& pubkey);
81-CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys);
82+CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys, bool fSorted);
    luke-jr commented at 9:03 am on November 25, 2016:
    Maybe default fSorted to false here rather than modify all the tests?
  22. in src/wallet/rpcwallet.cpp: in 7439562df1 outdated 
     998             "     [\n"
 999             "       \"address\"  (string) bitcoin address or hex-encoded public key\n"
1000             "       ...,\n"
1001             "     ]\n"
1002             "3. \"account\"      (string, optional) DEPRECATED. An account to assign the addresses to.\n"
1003+            "4. fSort          (bool, optional) Whether to sort public keys according to BIP67. Default setting is false.\n"
    luke-jr commented at 9:04 am on November 25, 2016:
    As before, rather turn param 3 into an options Object.
  23. luke-jr changes_requested
  24. luke-jr commented at 9:06 am on November 25, 2016: member
    Code looks reasonably correct, just a few nits. Did not verify tests.
  25. luke-jr referenced this in commit 0b0f4f71e3 on Dec 21, 2016
  26. luke-jr commented at 9:14 am on December 21, 2016: member
    Should update doc/bips.md also.
  27. luke-jr commented at 8:51 pm on February 4, 2017: member

    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?

    0git checkout sort-multisigs
1git fetch git://github.com/luke-jr/bitcoin sort-multisigs
2git reset --hard FETCH_HEAD
3git push ...
  28. afk11 commented at 8:10 pm on February 6, 2017: contributor

    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

  29. afk11 force-pushed on Mar 8, 2017
  30. afk11 force-pushed on Mar 8, 2017
  31. afk11 commented at 7:55 pm on March 8, 2017: contributor
    Merged commits and rebased. Apologies for the delay!
  32. afk11 commented at 8:21 pm on March 8, 2017: contributor
    The Apple build failed because the job time exceeded the maximum :/
  33. afk11 force-pushed on Mar 13, 2017
  34. afk11 force-pushed on Mar 13, 2017
  35. afk11 force-pushed on Mar 14, 2017
  36. afk11 commented at 8:47 am on March 14, 2017: contributor
    Rebased
  37. afk11 force-pushed on Mar 30, 2017
  38. afk11 force-pushed on May 9, 2017
  39. afk11 commented at 7:32 pm on May 9, 2017: contributor
    Rebased
  40. luke-jr commented at 3:50 am on June 6, 2017: member
    The description for d70583a2f9ae8bb4bda2e6224e699ac2cbb583ee is no longer correct.
  41. afk11 commented at 8:56 am on June 6, 2017: contributor
    @luke-jr Oops, yeah the rebase mightn’t have been the easiest. So re-add the contents of setup_network?
  42. luke-jr referenced this in commit e790f49485 on Jun 15, 2017
  43. luke-jr referenced this in commit ed5bd9fcad on Jun 15, 2017
  44. jnewbery commented at 2:20 pm on June 15, 2017: member

    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.

  45. afk11 commented at 2:30 pm on June 16, 2017: contributor

    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.

  46. jnewbery commented at 3:23 pm on June 16, 2017: member

    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.

  47. afk11 commented at 8:53 am on June 19, 2017: contributor

    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.

  48. in src/script/standard.cpp:295 in 69a90ec573 outdated 
    292+CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys, bool fSorted)
293 {
294-    CScript script;
295+    std::vector<std::vector<unsigned char>> vEncoded;
296+    vEncoded.reserve(keys.size());
297+    BOOST_FOREACH(const CPubKey& key, keys) {
    luke-jr commented at 2:49 pm on August 18, 2017:
    We’re not using BOOST_FOREACH anymore I think. (Also below)
  49. luke-jr commented at 9:33 pm on August 21, 2017: member

    Rebased and squashed a bit.

    0git checkout sort-multisigs && git fetch git://github.com/luke-jr/bitcoin sort-multisigs && git reset --hard FETCH_HEAD && git push ...
  50. afk11 force-pushed on Aug 24, 2017
  51. afk11 force-pushed on Sep 7, 2017
  52. TheBlueMatt commented at 6:13 pm on September 29, 2017: member
    Concept ACK. Care to rebase?
  53. afk11 force-pushed on Sep 30, 2017
  54. afk11 commented at 2:31 pm on September 30, 2017: contributor

    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)

  55. TheBlueMatt commented at 10:37 pm on October 2, 2017: member
    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.
  56. luke-jr referenced this in commit e0a68b1fad on Nov 6, 2017
  57. luke-jr referenced this in commit 48517f96e8 on Nov 6, 2017
  58. luke-jr referenced this in commit 6ddf8debe1 on Nov 6, 2017
  59. RPC: addmultisigaddress / createmultisig: parameterize _createmultisig_redeemScript to allow sorting of public keys (BIP67) 
    addmultisig/createmultisig RPC documentation: Remove stray quotes from fSort parameter
    5a2da09e4f
  60. script: GetScriptForRawPubKey: More efficient key-encoding loop ee9110a46d
  61. doc/bips: Add BIP 67 fcbf6cca5c
  62. Ensure whilst sorting only compressed public keys are used 6fbcf50e99
  63. Add more tests to sort_multisigs.py / wallet-accounts.py 
    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
    e11cb50a09
  64. afk11 force-pushed on Dec 2, 2017
  65. afk11 commented at 4:18 pm on December 2, 2017: contributor

    @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?

  66. luke-jr commented at 10:59 pm on March 1, 2018: member

    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.

  67. in src/wallet/rpcwallet.cpp:1153 in e11cb50a09 
    1147@@ -1148,21 +1148,24 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
1148         return NullUniValue;
1149     }
1150 
1151-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 3)
1152+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
1153     {
1154-        std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" )\n"
1155+        std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" ) ( sort )\n"
    luke-jr commented at 11:01 pm on March 1, 2018:
    Please switch back to an options object for this.
  68. in src/wallet/rpcwallet.cpp:1155 in e11cb50a09 
    1152+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
1153     {
1154-        std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" )\n"
1155+        std::string msg = "addmultisigaddress nrequired [\"key\",...] ( \"account\" ) ( sort )\n"
1156             "\nAdd a nrequired-to-sign multisignature address to the wallet. Requires a new wallet backup.\n"
1157+
    luke-jr commented at 11:01 pm on March 1, 2018:
    This blank line won’t be in the actual help, so has no purpose here.
  69. MarcoFalke added the label Needs rebase on Jun 6, 2018
  70. laanwj commented at 11:13 am on August 31, 2018: member
    Closing and putting “up for grabs” label
  71. laanwj closed this on Aug 31, 2018
  72. laanwj added the label Up for grabs on Aug 31, 2018
  73. laanwj removed the label Needs rebase on Oct 24, 2019
  74. DrahtBot locked this on Dec 16, 2021


afk11 dcousens laanwj MarcoFalke ryanofsky luke-jr jnewbery TheBlueMatt

Labels
RPC/REST/ZMQ Up for grabs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me