[rpc] createrawtransaction: Accept sorted outputs #11872

maflcko · 2017-12-12T01:23:36Z

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.

sipa · 2017-12-12T05:12:50Z

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.

jonasschnelli · 2017-12-12T06:10:49Z

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)

promag · 2017-12-12T07:22:40Z

Should use deprecated flag to enable backward compatibility?

laanwj · 2017-12-12T11:16:12Z

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.

promag

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

promag · 2017-12-12T11:16:09Z

src/rpc/rawtransaction.cpp

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():

... = outputs_is_object ? request.params[1] : request.params[1].get_array();

Can you provide code to reproduce that throw, please. When I tested it a day ago in the gui, it didn't throw.

@promag Heh, now I see what you mean. Makes sense.

Cool, I forgot to reply! :)

promag · 2017-12-12T11:20:46Z

@laanwj so just add a note that the parameter as object is deprecated?

laanwj · 2017-12-12T11:27:23Z

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.

maflcko · 2017-12-12T21:16:16Z

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

jnewbery · 2017-12-12T21:42:48Z

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 8334875 in #11660. Would it be useful here?

maflcko · 2017-12-13T00:38:53Z

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

laanwj · 2017-12-13T05:49:14Z

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?).

jnewbery · 2017-12-13T12:48:55Z

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:

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

(src/test/rpc_tests.cpp, L55)

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

maflcko · 2017-12-13T14:32:28Z

I will rebase and fixup all of this when the other createrawtransaction test are merged.

instagibbs · 2017-12-13T16:42:38Z

concept ACK

promag · 2017-12-14T02:25:08Z

BTW, echoing @laanwj #8457 (comment). I understand that here it's a different case though.

maflcko · 2017-12-14T04:36:41Z

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

promag · 2017-12-14T07:36:35Z

Just wanted to link his concern regarding overloading. However here we want to fix a problem which happens to overload the RPC.

promag · 2018-01-10T13:48:53Z

src/rpc/rawtransaction.cpp

Current format: Invalid parameter, key-value pair ....

promag · 2018-01-10T13:49:13Z

src/rpc/rawtransaction.cpp

Validate output is an object.

Done. Also added a test for this

maflcko · 2018-02-07T03:42:20Z

Rebased

maflcko · 2018-02-22T23:09:50Z

Added release notes.

luke-jr · 2018-02-26T16:59:09Z

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

maflcko · 2018-02-26T17:23:16Z

@luke-jr The check for duplicate keys is already there. See "Invalid parameter, duplicated address" in the functional tests.

jnewbery

utACK fa7a11a4c798eacc984762a379c206b0c7da8723

A few nits inline, but none should delay merge.

jnewbery · 2018-03-06T14:05:29Z

src/rpc/server.h

Comment could be updated since this object is now used by RPCTypeCheck and RPCTypeCheckArgument

jnewbery · 2018-03-06T15:18:49Z

src/test/rpc_tests.cpp

Why not just change from BOOST_CHECK_THROW() to BOOST_CHECK_NO_THROW() instead of removing entirely? Passing two arrays is now permitted.

Thx. Moved the check to the functional tests

Why not change to BOOST_CHECK_NO_THROW()?

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

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).

Yes. Moving all those tests to the functional test suite makes sense.

jnewbery · 2018-03-06T15:34:08Z

doc/release-notes.md

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

promag

Is there a use case to create a raw transaction without outputs?

promag · 2018-03-06T17:03:29Z

src/rpc/rawtransaction.cpp

Nit, the indentation could be fixed.

The alignment too:

1. "inputs" (array, required) A json array of json objects [ { "txid":"id", (string, required) The transaction id "vout":n, (numeric, required) The output number "sequence":n (numeric, optional) The sequence number } ,... ] 2. "outputs" (array, required) a json array with outputs (key-value pairs) [ { "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 }, { "data": "hex" (obj, optional) A key-value pair. The key must be "data", the value is hex encoded data } ,... More key-value pairs of the above form. For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also accepted as second parameter. ]

Fixed alignment

maflcko · 2018-03-07T18:01:39Z

Force pushed the documentation changes.

jnewbery · 2018-03-07T18:47:55Z

Tested ACK fa06dfc.

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

promag · 2018-03-07T19:06:09Z

Other than the 1 space indentation nit:

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

utACK fa06dfc.

conscott · 2018-03-11T01:43:21Z

src/rpc/rawtransaction.cpp


            "\nExamples:\n"
            + HelpExampleCli("createrawtransaction", "\"[{\\\"txid\\\":\\\"myid\\\",\\\"vout\\\":0}]\" \"{\\\"address\\\":0.01}\"")
            + HelpExampleCli("createrawtransaction", "\"[{\\\"txid\\\":\\\"myid\\\",\\\"vout\\\":0}]\" \"{\\\"data\\\":\\\"00010203\\\"}\"")


Need to add an example for the sorted format?

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

conscott · 2018-03-11T01:47:31Z

Tested ACK fa06dfc

Just left small inline comment

maflcko · 2018-03-11T20:49:42Z

Added a commit to address the documentation nit. This seems ready for merge.

fac7013 rpc: Update createrawtransaction examples (MarcoFalke) fa06dfc [rpc] createrawtransaction: Accept sorted outputs (MarcoFalke) 8acd25d rpc: Allow typeAny in RPCTypeCheck (MarcoFalke) Pull request description: 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. Tree-SHA512: cd562f34f7f9f79c7d3433805971325c388c2035611be283980f4049066a622df4f0afdc11d7ac96662260ec0115147cb65e1ab5268f5a1b063242f3fe425f77

maflcko added Needs release note RPC/REST/ZMQ labels Dec 12, 2017

promag reviewed Dec 12, 2017

View reviewed changes

promag mentioned this pull request Dec 12, 2017

Improve createrawtransaction functional tests #11877

Merged

maflcko force-pushed the Mf1712-rpcCreateRawSortedOuts branch 7 times, most recently from fafdc08 to fa288ad Compare December 13, 2017 21:46

ryanofsky mentioned this pull request Dec 14, 2017

Add 'subtractFeeFromAmount' option to 'fundrawtransaction'. #9222

Merged

maflcko mentioned this pull request Dec 26, 2017

[Docs] Fixed createrawtransaction help text. #11993

Closed

promag reviewed Jan 10, 2018

View reviewed changes

maflcko force-pushed the Mf1712-rpcCreateRawSortedOuts branch from fabf801 to fa2d1df Compare February 22, 2018 22:37

rpc: Allow typeAny in RPCTypeCheck

8acd25d

maflcko force-pushed the Mf1712-rpcCreateRawSortedOuts branch from fa2d1df to faf766d Compare February 22, 2018 22:40

jnewbery reviewed Mar 6, 2018

View reviewed changes

promag reviewed Mar 6, 2018

View reviewed changes

[rpc] createrawtransaction: Accept sorted outputs

fa06dfc

maflcko force-pushed the Mf1712-rpcCreateRawSortedOuts branch from fa7a11a to fa06dfc Compare March 7, 2018 17:58

conscott reviewed Mar 11, 2018

View reviewed changes

rpc: Update createrawtransaction examples

fac7013

laanwj merged commit fac7013 into bitcoin:master Mar 13, 2018

maflcko deleted the Mf1712-rpcCreateRawSortedOuts branch March 13, 2018 18:19

instagibbs mentioned this pull request Mar 16, 2018

[wallet] shuffle sendmany recipients ordering #12709

Merged

fanquake removed the Needs release note label Mar 20, 2019

elvinsophus mentioned this pull request May 10, 2019

Is the order of outputs preserved when calling 'createrawtransaction'? Bitcoin-ABC/bitcoin-abc#319

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[rpc] createrawtransaction: Accept sorted outputs #11872

[rpc] createrawtransaction: Accept sorted outputs #11872

Uh oh!

Conversation

maflcko commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Dec 12, 2017

Uh oh!

promag commented Dec 12, 2017

Uh oh!

laanwj commented Dec 12, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Dec 12, 2017

Uh oh!

laanwj commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 12, 2017

Uh oh!

jnewbery commented Dec 12, 2017

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

laanwj commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Dec 13, 2017

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

instagibbs commented Dec 13, 2017

Uh oh!

promag commented Dec 14, 2017

Uh oh!

maflcko commented Dec 14, 2017

Uh oh!

promag commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 7, 2018

Uh oh!

maflcko commented Feb 22, 2018

Uh oh!

luke-jr commented Feb 26, 2018

Uh oh!

maflcko commented Feb 26, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 12, 2017 •

edited

Loading

sipa commented Dec 12, 2017 •

edited

Loading

laanwj commented Dec 12, 2017 •

edited

Loading

laanwj commented Dec 13, 2017 •

edited

Loading