[wallet] Shuffle transaction inputs before signing #12699

instagibbs · 2018-03-15T18:51:06Z

Currently inputs are ordered based on COutPoint ordering, which while doesn't leak additional internal wallet state, likely further fingerprints the wallet as a Core wallet to observers.

Note: This slightly changed behavior of fundrawtransaction in that the newly-appended inputs will now be shuffled rather than in outpoint-order. This does not break API compatibility.

Simple shuffling of the coins being returned will hopefully allow the wallet to blend in a bit more, in lieu of additional data to find what other wallets are doing, or another standard, ala @gmaxwell's suggested of ordering via scriptPubKey.

laanwj · 2018-03-15T18:56:00Z

Concept ACK - haven't reviewed yet but this looks like a lot of changes for just an additional input shuffle step?

instagibbs · 2018-03-15T19:00:21Z

Ok, I can make this smaller. ~~Just realized that the inner-use of the inputs doesn't actually require proper ordering for transaction size estimation, so it should just be a single shuffle before signing.~~

A bit annoying due to inner-loop behavior, but I'll see what I can do.

ryanofsky · 2018-03-15T19:01:42Z

Related discussion: https://botbot.me/freenode/bitcoin-core-dev/msg/97851598/

Sjors · 2018-03-15T19:04:54Z

Maybe just use the same ordering as BIP-69?

instagibbs · 2018-03-15T19:16:56Z

@Sjors I'd prefer to something slightly less bad now, and converge on a standard later. I have some quibbles with BIP69, which I think are out of scope for this thread.

Sjors · 2018-03-15T19:22:13Z

TIL the Core wallet doesn't use BIP-69 (which is a separate discussion), so that does weaken the case for using it here. (and TIL coin selection function also determines the order, not just the set)

See also #12457

instagibbs · 2018-03-15T20:19:55Z

Smaller changeset now, had to move the final input creation further output to avoid storing shuffle ordering.

instagibbs · 2018-03-16T18:09:20Z

Turns out that we're ~this far away(in master) from having the input part compliant with BIP69:

struct bip69_compare {
bool operator() (const uint256& lhs, const uint256& rhs) const
{
uint256 lhs_hash = rhs;
uint256 rhs_hash = lhs;
std::reverse(lhs_hash.begin(), lhs_hash.end());
std::reverse(rhs_hash.begin(), rhs_hash.end());
return rhs_hash < lhs_hash;
}
};`

The inputs are being sorted by BE rather than LE as per the BIP.

We also currently aren't really shuffling outputs randomly(except the case of 2 outputs, 1 destination and 1 change. This is fixed by a single shuffle line. BIP69 compliance would be another comparison struct.

I still prefer this changeset for the time being, due to ease of implementation and complementary nature with #12709

luke-jr · 2018-03-16T21:44:35Z

BIP 69 contradicts SIGHASH_SINGLE usage, and its concerns seem unrealistic (if you use malicious closed source wallet software, you're already compromised regardless). Randomising seems fine.

donaloconnor · 2018-03-17T11:54:04Z

utACK 8712bbd

instagibbs · 2018-03-21T19:04:41Z

rebased onto #12742 to avoid code churn

sipa · 2018-03-21T23:39:28Z

utACK 2fb9c1e

maflcko · 2018-03-22T17:31:00Z

If you want to remove the erroneous diff from the GitHub web view:

EDITOR=true git commit --allow-empty -m empty && git push [email protected]:instagibbs/bitcoin.git shuffleinputs && git reset --hard 2fb9c1e && git push [email protected]:instagibbs/bitcoin.git shuffleinputs -f

maflcko · 2018-03-22T17:54:58Z

utACK 2fb9c1e

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

promag · 2018-03-24T12:13:30Z

utACK 2fb9c1e, looks good.

This slightly changed behavior of fundrawtransaction in that the newly-appended inputs will now be shuffled rather than in outpoint-order.

At least the test framework doesn't rely on the order.

sipa · 2018-03-24T20:03:52Z

PR title seems outdated now (the shuffle happens after SelectCoins returns).

instagibbs · 2018-03-26T13:42:53Z

updated title

jamesob · 2018-03-26T14:55:48Z

utACK 2fb9c1e

@gmaxwell

2fb9c1e shuffle selected coins before transaction finalization (Gregory Sanders) Pull request description: Currently inputs are ordered based on COutPoint ordering, which while doesn't leak additional internal wallet state, likely further fingerprints the wallet as a Core wallet to observers. Note: This slightly changed behavior of `fundrawtransaction` in that the newly-appended inputs will now be shuffled rather than in outpoint-order. This does not break API compatibility. Simple shuffling of the coins being returned will hopefully allow the wallet to blend in a bit more, in lieu of additional data to find what other wallets are doing, or another standard, ala @gmaxwell's suggested of ordering via scriptPubKey. Tree-SHA512: 70689a6eccf9fa7fc6e3d884f2eba4b482446a1e6128beff7a98f446d0c60f7966c5a6c55e9b0b3d73a9b539ce54889a26c7efe78ab7f34af386d5e4f3fa6df2

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

instagibbs changed the title ~~Shuffle transaction inputs before returning from SelectCoins~~ [wallet] Shuffle transaction inputs before returning from SelectCoins Mar 15, 2018

laanwj added the Wallet label Mar 15, 2018

instagibbs force-pushed the shuffleinputs branch from 263389f to 8712bbd Compare March 15, 2018 20:19

fanquake requested a review from achow101 March 15, 2018 22:50

instagibbs mentioned this pull request Mar 16, 2018

[wallet] shuffle sendmany recipients ordering #12709

Merged

shuffle selected coins before transaction finalization

2fb9c1e

instagibbs force-pushed the shuffleinputs branch from 8712bbd to 2fb9c1e Compare March 21, 2018 19:03

instagibbs force-pushed the shuffleinputs branch from f652665 to 2fb9c1e Compare March 22, 2018 17:36

instagibbs changed the title ~~[wallet] Shuffle transaction inputs before returning from SelectCoins~~ [wallet] Shuffle transaction inputs before signing Mar 26, 2018

laanwj merged commit 2fb9c1e into bitcoin:master Mar 26, 2018

HashUnlimited mentioned this pull request Jul 31, 2018

[wallet] shuffle sendmany recipients ordering chaincoin/chaincoin#87

Merged

This was referenced Jan 7, 2020

randomizing the order of inputs and outputs (per BIP69 guidelines) horizontalsystems/unstoppable-wallet-android#1773

Closed

randomizing the order of inputs and outputs (per BIP69 guidelines) horizontalsystems/unstoppable-wallet-ios#1477

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[wallet] Shuffle transaction inputs before signing #12699

[wallet] Shuffle transaction inputs before signing #12699

Conversation

instagibbs commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 15, 2018

Uh oh!

Sjors commented Mar 15, 2018

Uh oh!

instagibbs commented Mar 15, 2018

Uh oh!

Sjors commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Mar 15, 2018

Uh oh!

instagibbs commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Mar 16, 2018

Uh oh!

donaloconnor commented Mar 17, 2018

Uh oh!

instagibbs commented Mar 21, 2018

Uh oh!

sipa commented Mar 21, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

promag commented Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Mar 24, 2018

Uh oh!

instagibbs commented Mar 26, 2018

Uh oh!

jamesob commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

instagibbs commented Mar 15, 2018 •

edited

Loading

laanwj commented Mar 15, 2018 •

edited

Loading

instagibbs commented Mar 15, 2018 •

edited

Loading

Sjors commented Mar 15, 2018 •

edited

Loading

instagibbs commented Mar 16, 2018 •

edited

Loading

promag commented Mar 24, 2018 •

edited

Loading