[rpc] createrawtransaction: Accept sorted outputs #11872

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:Mf1712-rpcCreateRawSortedOuts changing 6 files +111 −42
  1. MarcoFalke commented at 1:23 am on December 12, 2017: member

    The second parameter of the createrawtransaction is a dictionary of the outputs. This comes with at least two drawbacks:

    • In case of duplicate keys, either of them might silently disappear, with no user feedback at all. A user needs to make other mistakes, but this could eventually lead to abnormal tx fees.
    • A dictionary does not guarantee that keys are sorted. Again, a user needs to keep this in mind, as it could eventually lead to excessive tx fees.

    Even though my scenario of loss-of-funds is unlikely to happen, I see it as a inconvenience that should be fixed.

  2. MarcoFalke added the label Needs release notes on Dec 12, 2017
  3. MarcoFalke added the label RPC/REST/ZMQ on Dec 12, 2017
  4. sipa commented at 5:12 am on December 12, 2017: member
    Vague concept ACK that accepting a list instead of a dict should be supported. I have little opinion on how to do that in a backward compatible way.
  5. jonasschnelli commented at 6:10 am on December 12, 2017: contributor
    Concept ACK. I think the backward compatibility is solved in a good way… not sure if it should be mentioned somewhere (at least in some comments)
  6. promag commented at 7:22 am on December 12, 2017: member
    Should use deprecated flag to enable backward compatibility?
  7. in src/rpc/rawtransaction.cpp:376 in fa961b9bca outdated 
    373     UniValue inputs = request.params[0].get_array();
374-    UniValue sendTo = request.params[1].get_obj();
375+    const bool outputs_is_array = request.params[1].isArray();
376+    UniValue outputs = outputs_is_array ?
377+                           request.params[1].get_array() :
378+                           request.params[1].get_obj();
    promag commented at 11:16 am on December 12, 2017:

    This will raise JSON value is not an object as expected which doesn’t match with the new signature. I suggest to switch to isObject() above so that get_array throws. In that case, it is not necessary to callget_obj():

    0... = outputs_is_object ? request.params[1] : request.params[1].get_array();
    MarcoFalke commented at 0:31 am on December 13, 2017:
    Can you provide code to reproduce that throw, please. When I tested it a day ago in the gui, it didn’t throw.
    MarcoFalke commented at 8:27 pm on December 13, 2017:
    @promag Heh, now I see what you mean. Makes sense.
    promag commented at 8:51 pm on December 13, 2017:
    Cool, I forgot to reply! :)
  8. laanwj commented at 11:16 am on December 12, 2017: member

    Concept ACK, I’ve frequently wondered why this was a dictionary.

    Should use deprecated flag to enable backward compatibility?

    I think both ways should be supported for the foreseeable future, no need to deprecate anything right now. There’s no reason to break software for this.

  9. promag commented at 11:18 am on December 12, 2017: member

    In case of duplicate keys, either of them might silently disappear, with no user feedback at all

    Why? Invalid parameter, duplicated address is thrown.

    A dictionary does not guarantee that keys are sorted

    Why does it matters?

    Are these concerns because of the common lack support for duplicate keys in clients?

    And since this is an utility call, weren’t we moving enhancements to bitcoin-tx instead (although I’m ok with it)?

    Maybe the missing piece is an updaterawtransaction call, where inputs/outputs can be added/removed/updated (just a thought)?

  10. promag commented at 11:20 am on December 12, 2017: member
    @laanwj so just add a note that the parameter as object is deprecated?
  11. laanwj commented at 11:27 am on December 12, 2017: member

    TODO: Needs tests

    Something that I guess needs to be tested explicitly now: duplicate address causes failure (or is that an actual use-case? we don’t allow multiple outputs to the same address in the wallet, at least). Edit, a check and error for that already exists, #11877 adds a test.

  12. MarcoFalke commented at 9:16 pm on December 12, 2017: member
    Note that this didn’t break any functional test, so it should be compatible with the previous interface.
  13. jnewbery commented at 9:42 pm on December 12, 2017: member

    Concept ACK. I’ll review once there are tests.

    Have you seen luke-jr’s change to allow RPC Type Checking against more than one type. It’s in commit https://github.com/bitcoin/bitcoin/pull/11660/commits/8334875d8dcebab3841e42ab25e4e1d5247070a6 in #11660. Would it be useful here?

  14. MarcoFalke commented at 0:38 am on December 13, 2017: member

    Why? Invalid parameter, duplicated address is thrown.

    Not when the dict implementation eats duplicate keys. This is at least the case for python’s dict and javascript’s dict.

    Why does it matters?

    The outputs structure is a vector, i.e. sorted in the bitcoin protocol, thus one could falsely assume that creating a raw transaction via createrawtransaction populates that vector in the same order as the outputs are typed.

    Have you seen luke-jr’s change to allow RPC Type Checking against more than one type. It’s in commit 8334875 in #11660. Would it be useful here?

    Yes, that could be useful. Though out of scope for this pull, imo. I am calling either get_array or get_obj, both of which throw on invalid type. So I am pretty sure I don’t need to pull in further features.

    C.f. https://dev.visucore.com/bitcoin/doxygen/univalue__get_8cpp_source.html#l00141

  15. laanwj commented at 5:49 am on December 13, 2017: member

    Not when the dict implementation eats duplicate keys. This is at least the case for python’s dict and javascript’s dict.

    Normally whatever dict/object implementation is on the client-side will eat it. To be able pass duplicate keys, the clients need to some way emit an array as a JSON object. That’s absolutely against the JSON spec, just like expecting object attributes to be emitted in a certain order. To quote myself from IRC:

    <wumpus> FWIW in principle it is already possible to specify the order of outputs, but only if the client-side JSON library can emit ordered dictionaries. Python’s can do this when you use Collections.OrderedDict() instead of a dictionary. The server-side (univalue) won’t reorder dictionaries. But using an array is conforming to the spec instead of ‘it happens to work’.

    Note that this didn’t break any functional test, so it should be compatible with the previous interface.

    It seems to break Travis, any idea why (or is this another fluke?).

  16. jnewbery commented at 12:48 pm on December 13, 2017: member

    It seems to break Travis, any idea why (or is this another fluke?).

    No, not a random Travis failure. This rpc unit test is broken:

    0BOOST_CHECK_THROW(CallRPC("createrawtransaction [] []"), std::runtime_error);

    (src/test/rpc_tests.cpp, L55)

    since createrawtransaction can now take an array as the second argument.

  17. MarcoFalke commented at 2:32 pm on December 13, 2017: member
    I will rebase and fixup all of this when the other createrawtransaction test are merged.
  18. instagibbs commented at 4:42 pm on December 13, 2017: member
    concept ACK
  19. MarcoFalke force-pushed on Dec 13, 2017
  20. MarcoFalke force-pushed on Dec 13, 2017
  21. MarcoFalke force-pushed on Dec 13, 2017
  22. MarcoFalke force-pushed on Dec 13, 2017
  23. MarcoFalke force-pushed on Dec 13, 2017
  24. MarcoFalke force-pushed on Dec 13, 2017
  25. MarcoFalke force-pushed on Dec 13, 2017
  26. promag commented at 2:25 am on December 14, 2017: member
    BTW, echoing @laanwj #8457 (comment). I understand that here it’s a different case though.
  27. MarcoFalke commented at 4:36 am on December 14, 2017: member
    @promag Can you elaborate a bit more on that? It is a different case and in fact we do overload already, IIRC for some bool/integer parameters.
  28. promag commented at 7:36 am on December 14, 2017: member
    Just wanted to link his concern regarding overloading. However here we want to fix a problem which happens to overload the RPC.
  29. in src/rpc/rawtransaction.cpp:432 in fa288ade78 outdated 
    430+        // Translate array of key-value pairs into dict
431+        UniValue outputs_dict = UniValue(UniValue::VOBJ);
432+        for (size_t i = 0; i < outputs.size(); ++i) {
433+            const UniValue& output = outputs[i];
434+            if (output.size() != 1) {
435+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter: Key-value pair must contain exactly one key");
    promag commented at 1:48 pm on January 10, 2018:
    Current format: Invalid parameter, key-value pair ....
  30. in src/rpc/rawtransaction.cpp:435 in fa288ade78 outdated 
    428-
429+    if (!outputs_is_obj) {
430+        // Translate array of key-value pairs into dict
431+        UniValue outputs_dict = UniValue(UniValue::VOBJ);
432+        for (size_t i = 0; i < outputs.size(); ++i) {
433+            const UniValue& output = outputs[i];
    promag commented at 1:49 pm on January 10, 2018:
    Validate output is an object.
    MarcoFalke commented at 6:25 pm on January 10, 2018:
    Done. Also added a test for this
  31. MarcoFalke force-pushed on Jan 10, 2018
  32. MarcoFalke force-pushed on Jan 10, 2018
  33. in src/rpc/rawtransaction.cpp:436 in fac70767e7 outdated 
    429+    if (!outputs_is_obj) {
430+        // Translate array of key-value pairs into dict
431+        UniValue outputs_dict = UniValue(UniValue::VOBJ);
432+        for (size_t i = 0; i < outputs.size(); ++i) {
433+            const UniValue& output = outputs[i];
434+            if (!output.isObject()) {
    promag commented at 6:02 pm on January 18, 2018:
    Use RPCTypeCheckArgument instead?
    MarcoFalke commented at 10:06 pm on January 18, 2018:
    Would make the error message less specific
    promag commented at 10:14 pm on January 18, 2018:
    Right.
  34. in test/functional/rawtransactions.py:305 in fac70767e7 outdated 
    301@@ -263,7 +302,7 @@ def run_test(self):
302         encrawtx = "01000000010000000000000072c1a6a246ae63f74f931e8365e15a089c68d61900000000000000000000ffffffff0100e1f505000000000000000000"
303         decrawtx = self.nodes[0].decoderawtransaction(encrawtx, False) # decode as non-witness transaction
304         assert_equal(decrawtx['vout'][0]['value'], Decimal('1.00000000'))
305-        
306+
    promag commented at 6:03 pm on January 18, 2018:
    👀
    MarcoFalke commented at 10:07 pm on January 18, 2018:
    Removing trailing white space should be fine now, since adding trailing white space is forbidden. So we converge to a single “code style”.
  35. in src/rpc/server.cpp:69 in fac70767e7 outdated 
    65@@ -67,10 +66,10 @@ void RPCTypeCheck(const UniValue& params,
66     }
67 }
68 
69-void RPCTypeCheckArgument(const UniValue& value, UniValue::VType typeExpected)
70+void RPCTypeCheckArgument(const UniValue& value, const UniValueType& typeExpected)
    promag commented at 6:05 pm on January 18, 2018:
    Move to different commit?
    MarcoFalke commented at 10:06 pm on January 18, 2018:
    Thx, done.
  36. promag commented at 6:07 pm on January 18, 2018: member
    As a follow up, should the test framework be updated to use this variant?
  37. MarcoFalke force-pushed on Jan 18, 2018
  38. MarcoFalke commented at 10:09 pm on January 18, 2018: member

    should the test framework be updated to use this variant?

    Depends on the use case and your personal preference. In most cases there is no functional difference.

  39. promag commented at 10:13 pm on January 18, 2018: member

    should the test framework be updated to use this variant?

    Depends on the use case and your personal preference. In most cases there is no functional difference.

    Just saying, because IMO we should encourage this variant. Maybe tag for or add release note.

    BTW, I think you can remove the TODO in the description.

    utACK fa2e390.

  40. MarcoFalke commented at 10:27 pm on January 18, 2018: member

    add release note

    Will do immediately after merge, as I am not sure which branch this will end up on.

  41. promag commented at 10:53 pm on January 18, 2018: member
    Sorry already had the tag.
  42. MarcoFalke force-pushed on Feb 7, 2018
  43. MarcoFalke force-pushed on Feb 7, 2018
  44. MarcoFalke commented at 3:42 am on February 7, 2018: member
    Rebased
  45. MarcoFalke force-pushed on Feb 22, 2018
  46. rpc: Allow typeAny in RPCTypeCheck 8acd25d854
  47. MarcoFalke force-pushed on Feb 22, 2018
  48. MarcoFalke commented at 11:09 pm on February 22, 2018: member
    Added release notes.
  49. luke-jr commented at 4:59 pm on February 26, 2018: member
    I don’t see the use case… it might make sense for error checking, but this doesn’t add a check for duplicate keys either…?
  50. MarcoFalke commented at 5:23 pm on February 26, 2018: member
    @luke-jr The check for duplicate keys is already there. See “Invalid parameter, duplicated address” in the functional tests.
  51. in src/rpc/server.h:31 in fa7a11a4c7 outdated 
    29@@ -30,7 +30,7 @@ namespace RPCServer
30 /** Wrapper for UniValue::VType, which includes typeAny:
31  * Used to denote don't care type. Only used by RPCTypeCheckObj */
    jnewbery commented at 2:05 pm on March 6, 2018:
    Comment could be updated since this object is now used by RPCTypeCheck and RPCTypeCheckArgument
  52. in src/test/rpc_tests.cpp:55 in fa7a11a4c7 outdated 
    51@@ -52,7 +52,6 @@ BOOST_AUTO_TEST_CASE(rpc_rawparams)
52     BOOST_CHECK_THROW(CallRPC("createrawtransaction"), std::runtime_error);
53     BOOST_CHECK_THROW(CallRPC("createrawtransaction null null"), std::runtime_error);
54     BOOST_CHECK_THROW(CallRPC("createrawtransaction not_array"), std::runtime_error);
55-    BOOST_CHECK_THROW(CallRPC("createrawtransaction [] []"), std::runtime_error);
    jnewbery commented at 3:18 pm on March 6, 2018:
    Why not just change from BOOST_CHECK_THROW() to BOOST_CHECK_NO_THROW() instead of removing entirely? Passing two arrays is now permitted.
    MarcoFalke commented at 6:01 pm on March 7, 2018:
    Thx. Moved the check to the functional tests
    jnewbery commented at 6:45 pm on March 7, 2018:
    Why not change to BOOST_CHECK_NO_THROW()?
    MarcoFalke commented at 7:01 pm on March 7, 2018:
    @jnewbery I think we deprecated the existing rpc unit tests in favor of the functional tests which test the rpc interface. If you wanted reasonable coverage, you’d have to duplicate all the rpc unit tests based on the functional rpc tests. So yeah, I am not convinced that the unit tests add additional coverage…
    jnewbery commented at 7:06 pm on March 7, 2018:

    If you really think the unit tests are deprecated, they should be removed in a separate PR. It doesn’t make sense to me to remove just this one case in this PR.

    Anyway, as I said - I don’t think this should hold up merge. I’m just puzzled that you’d chose a larger diff that reduces test coverage (the fact that this unit test failed in the first iteration of this PR tells me that it was testing something).

    MarcoFalke commented at 9:03 pm on March 7, 2018:
    Yes. Moving all those tests to the functional test suite makes sense.
  53. in doc/release-notes.md:65 in fa7a11a4c7 outdated 
    60@@ -61,7 +61,8 @@ RPC changes
61 
62 ### Low-level changes
63 
64-- The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
65+- The `fundrawtransaction` RPC will reject the previously deprecated `reserveChangeKey` option.
66+- The `createrawtransaction` RPC will now accept an array or dictionary (kept for compatibility) for the `outputs` parameter. This makes the result of the call deterministic.
    jnewbery commented at 3:34 pm on March 6, 2018:
    Could use simpler language and be more explicit makes the result of the call deterministic -> means the order of transaction outputs can be specified by the client or similar
  54. jnewbery commented at 3:41 pm on March 6, 2018: member

    utACK fa7a11a4c798eacc984762a379c206b0c7da8723

    A few nits inline, but none should delay merge.

  55. in src/rpc/rawtransaction.cpp:340 in fa7a11a4c7 outdated 
    335@@ -335,12 +336,17 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
336             "       } \n"
337             "       ,...\n"
338             "     ]\n"
339-            "2. \"outputs\"               (object, required) a json object with outputs\n"
340+            "2. \"outputs\"               (array, required) a json array with outputs (key-value pairs)\n"
341+            "   [\n"
    promag commented at 5:03 pm on March 6, 2018:
    Nit, the indentation could be fixed.
    promag commented at 5:04 pm on March 6, 2018:

    The alignment too:

     01. "inputs"                (array, required) A json array of json objects
 1     [
 2       {
 3         "txid":"id",    (string, required) The transaction id
 4         "vout":n,         (numeric, required) The output number
 5         "sequence":n      (numeric, optional) The sequence number
 6       }
 7       ,...
 8     ]
 92. "outputs"               (array, required) a json array with outputs (key-value pairs)
10   [
11    {
12      "address": x.xxx,    (obj, optional) A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC
13    },
14    {
15      "data": "hex"      (obj, optional) A key-value pair. The key must be "data", the value is hex encoded data
16    }
17    ,...                     More key-value pairs of the above form. For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also
18                             accepted as second parameter.
19   ]
    MarcoFalke commented at 6:00 pm on March 7, 2018:
    Fixed alignment
  56. promag commented at 5:19 pm on March 6, 2018: member
    Is there a use case to create a raw transaction without outputs?
  57. [rpc] createrawtransaction: Accept sorted outputs fa06dfce0f
  58. MarcoFalke force-pushed on Mar 7, 2018
  59. MarcoFalke commented at 6:01 pm on March 7, 2018: member
    Force pushed the documentation changes.
  60. jnewbery commented at 6:47 pm on March 7, 2018: member

    Tested ACK fa06dfce0f1f8e5e34f416c556590fa56b721788.

    I’d still prefer to modify the unit test rather than dropping it, but this is ready for merge.

  61. promag commented at 7:06 pm on March 7, 2018: member

    Other than the 1 space indentation nit:

    02. "outputs"               (array, required) a json array with outputs (key-value pairs)
1   [
2    {

    utACK fa06dfc.

  62. in src/rpc/rawtransaction.cpp:358 in fa06dfce0f outdated 
    357@@ -352,14 +358,25 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
358             + HelpExampleCli("createrawtransaction", "\"[{\\\"txid\\\":\\\"myid\\\",\\\"vout\\\":0}]\" \"{\\\"data\\\":\\\"00010203\\\"}\"")
    conscott commented at 1:43 am on March 11, 2018:

    Need to add an example for the sorted format?

    Right now it only shows examples with the ‘backwards compatible" format

  63. conscott commented at 1:47 am on March 11, 2018: contributor

    Tested ACK fa06dfce0f1f8e5e34f416c556590fa56b721788

    Just left small inline comment

  64. rpc: Update createrawtransaction examples fac70134a9
  65. MarcoFalke commented at 8:49 pm on March 11, 2018: member
    Added a commit to address the documentation nit. This seems ready for merge.
  66. laanwj merged this on Mar 13, 2018
  67. laanwj closed this on Mar 13, 2018
  68. laanwj referenced this in commit 702e8b70bd on Mar 13, 2018
  69. MarcoFalke deleted the branch on Mar 13, 2018
  70. fanquake removed the label Needs release note on Mar 20, 2019
  71. PastaPastaPasta referenced this in commit 5a5b52f00e on Jun 10, 2020
  72. PastaPastaPasta referenced this in commit 59229a87dd on Jun 13, 2020
  73. PastaPastaPasta referenced this in commit 754e918d7b on Jun 13, 2020
  74. PastaPastaPasta referenced this in commit 87798aba80 on Jun 13, 2020
  75. PastaPastaPasta referenced this in commit 871e79a5a5 on Jun 17, 2020
  76. DrahtBot locked this on Dec 16, 2021


MarcoFalke sipa jonasschnelli promag laanwj jnewbery instagibbs luke-jr conscott

Labels
RPC/REST/ZMQ

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 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me