[wallet] shuffle sendmany recipients ordering #12709
Conversation
instagibbs
changed the title
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. |
|
@ryanofsky ok, I was operating under the same assumption as #11872 I believe the arguments to change the behavior is still strong. |
|
IMO the Maybe we should mention this in the release notes? utACK fdc2e316ddea8454cc329ff8f0694c0a1740f508 |
|
Yes this definitely needs mention in the release notes, since the caller can control the order as explained by @ryanofsky |
|
Also, I think for API clarity we should accept a list that allows duplicates regardless of json implementation. Similar to #11872 |
@ryanofsky why not? The same could be said in #11872? |
|
@promag raw transaction APIs are a completely different beast, where caller expects more control, and can do their own shuffling if required |
|
I will add release notes. |
|
@instagibbs got it. Agree with @MarcoFalke though in doing something similar to #11872 in a different PR.
It can be a pain in some cases, so I don't think we should focus on that before doing the same as #11872. |
|
utACK fdc2e316ddea8454cc329ff8f0694c0a1740f508. This merits a release notes update, though. |
|
@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. |
src/wallet/rpcwallet.cpp
Outdated
There was a problem hiding this comment.
Please rebase on top of #12742 and use std::shuffle from cpp11 to avoid future code churn.
instagibbs
force-pushed
the
shuffleoutputs
branch
from
fdc2e31 to
b4f15bd
Compare
|
rebased onto #12742 and added release note. |
instagibbs
changed the title
|
utACK 76cc7f9bfe530cce24800cd317e0c36b5610a1cd |
|
utACK 76cc7f9 |
|
utACK 76cc7f9. |
instagibbs
force-pushed
the
shuffleoutputs
branch
from
76cc7f9 to
6acb02d
Compare
|
had to rebase due to release notes conflict :( |
|
re-utACK 6acb02d |
|
utACK 6acb02d. |
6acb02d add release note for sendmany output shuffling (Gregory Sanders) cf6ef3c shuffle sendmany recipients ordering to shuffle tx outputs (Gregory Sanders) Pull request description: 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) related: #12699 Tree-SHA512: afdd990dde6a4a9e7eef7bb2e3342a46d11900d7fe6e6e4eb0cc6b5deed89df989fa7931a4bdcbf49b7c2d7a13c90169af3a166466e5760948bacabe3490f572
Summary: 6acb02d add release note for sendmany output shuffling (Gregory Sanders) cf6ef3c shuffle sendmany recipients ordering to shuffle tx outputs (Gregory Sanders) Pull request description: 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) related: bitcoin/bitcoin#12699 Tree-SHA512: afdd990dde6a4a9e7eef7bb2e3342a46d11900d7fe6e6e4eb0cc6b5deed89df989fa7931a4bdcbf49b7c2d7a13c90169af3a166466e5760948bacabe3490f572 Backport of Core PR12709 bitcoin/bitcoin#12709 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3897
9 participants
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
sendtoaddresssince there is only 1 or 2 outputs, and the change output is shuffled in.This will not effect
*rawbehavior 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)
related: #12699