[wallet] shuffle sendmany recipients ordering

instagibbs commented at 6:36 pm on March 16, 2018: member

Unless there is something important I’m missing, we’re just possible leaking information by preserving whatever ordering json object ordering is giving us (no guarantees at all).

This is unneeded for sendtoaddress since there is only 1 or 2 outputs, and the change output is shuffled in.

This will not effect *raw behavior by design, since users generally want full control using those apis. Further PRs could add optional args to over-ride that behavior.

Alternative ideas would be to sort the outputs by some deterministic ordering. (this would require more refactoring since change outputs are created and handled by caller)

instagibbs renamed this:
~~shuffle sendmany recipients ordering, since caller cannot control~~
[wallet] shuffle sendmany recipients ordering, since caller cannot control
on Mar 16, 2018

ryanofsky commented at 6:57 pm on March 16, 2018: member

since caller cannot control

It’s not really true that the caller can’t control this. But maybe we don’t want to allow them to. Whether the caller can control the order just depends on how they are generating their request, and what JSON library they are using if any. The python json library and our c++ UniValue library do allow control over ordering.

instagibbs commented at 7:01 pm on March 16, 2018: member

@ryanofsky ok, I was operating under the same assumption as #11872

I believe the arguments to change the behavior is still strong.

meshcollider added the label RPC/REST/ZMQ on Mar 16, 2018

donaloconnor approved

donaloconnor commented at 11:57 am on March 17, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12709/commits/fdc2e316ddea8454cc329ff8f0694c0a1740f508

jonasschnelli commented at 5:15 am on March 18, 2018: contributor

IMO the sendmany API has no order (per JSON specs) due to the use of an associated array (object) ( {\"address\":amount,...}).

Maybe we should mention this in the release notes?

utACK fdc2e316ddea8454cc329ff8f0694c0a1740f508

MarcoFalke commented at 10:57 am on March 18, 2018: member

Yes this definitely needs mention in the release notes, since the caller can control the order as explained by @ryanofsky

MarcoFalke commented at 11:08 am on March 18, 2018: member

Also, I think for API clarity we should accept a list that allows duplicates regardless of json implementation. Similar to #11872

MarcoFalke added the label Needs release notes on Mar 19, 2018

promag commented at 11:28 pm on March 20, 2018: member

But maybe we don’t want to allow them to @ryanofsky why not? The same could be said in #11872?

instagibbs commented at 11:32 pm on March 20, 2018: member

@promag raw transaction APIs are a completely different beast, where caller expects more control, and can do their own shuffling if required

instagibbs commented at 11:33 pm on March 20, 2018: member

I will add release notes.

promag commented at 0:15 am on March 21, 2018: member

@instagibbs got it. Agree with @MarcoFalke though in doing something similar to #11872 in a different PR.

the caller can control the order

It can be a pain in some cases, so I don’t think we should focus on that before doing the same as #11872.

sipa commented at 1:41 am on March 21, 2018: member

utACK fdc2e316ddea8454cc329ff8f0694c0a1740f508. This merits a release notes update, though.

sipa commented at 2:13 am on March 21, 2018: member

@instagibbs Have a look at #12742; that should permit a faster implementation that doesn’t need a call to OpenSSL for every output in the list.

in src/wallet/rpcwallet.cpp:1149 in fdc2e316dd outdated

1144@@ -1145,6 +1145,9 @@ UniValue sendmany(const JSONRPCRequest& request)
1145     if (totalAmount > nBalance)
1146         throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds");
1147 
1148+    // Shuffle recipient list
1149+    std::random_shuffle(vecSend.begin(), vecSend.end(), GetRandInt);

MarcoFalke commented at 12:17 pm on March 21, 2018:

Please rebase on top of #12742 and use std::shuffle from cpp11 to avoid future code churn.

instagibbs force-pushed on Mar 21, 2018

instagibbs commented at 6:51 pm on March 21, 2018: member

rebased onto #12742 and added release note.

instagibbs renamed this:
~~[wallet] shuffle sendmany recipients ordering, since caller cannot control~~
[wallet] shuffle sendmany recipients ordering
on Mar 21, 2018

sipa commented at 0:07 am on March 22, 2018: member

utACK 76cc7f9bfe530cce24800cd317e0c36b5610a1cd

MarcoFalke force-pushed on Mar 22, 2018

MarcoFalke commented at 5:27 pm on March 22, 2018: member

utACK 76cc7f9

promag commented at 5:56 pm on March 22, 2018: member

utACK 76cc7f9.

shuffle sendmany recipients ordering to shuffle tx outputs cf6ef3c139

add release note for sendmany output shuffling 6acb02d635

instagibbs force-pushed on Mar 23, 2018

instagibbs commented at 12:57 pm on March 23, 2018: member

had to rebase due to release notes conflict :(

MarcoFalke commented at 2:44 pm on March 23, 2018: member

re-utACK 6acb02d635da68c0506b49a8dd27c2c3ded4addf

promag commented at 3:36 pm on March 23, 2018: member

utACK 6acb02d.

MarcoFalke merged this on Mar 23, 2018

MarcoFalke closed this on Mar 23, 2018

MarcoFalke referenced this in commit 02b7e8319a on Mar 23, 2018

fanquake removed the label Needs release note on Mar 20, 2019

Fabcien referenced this in commit 56a2d76149 on Aug 30, 2019

PastaPastaPasta referenced this in commit 6c405f3f8c on Jun 10, 2020

PastaPastaPasta referenced this in commit 4d90470d16 on Jun 13, 2020

PastaPastaPasta referenced this in commit 7876f19cac on Jun 13, 2020

PastaPastaPasta referenced this in commit ef92f3c114 on Jun 13, 2020

DrahtBot locked this on Dec 16, 2021

[wallet] shuffle sendmany recipients ordering #12709