[wallet] Shuffle transaction inputs before signing #12699

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:shuffleinputs changing 1 files +23 −14

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

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.
instagibbs renamed this:
~~Shuffle transaction inputs before returning from SelectCoins~~
[wallet] Shuffle transaction inputs before returning from SelectCoins
on Mar 15, 2018
laanwj added the label Wallet on Mar 15, 2018
laanwj commented at 6:56 pm on March 15, 2018: member

Concept ACK - haven’t reviewed yet but this looks like a lot of changes for just an additional input shuffle step?
instagibbs commented at 7:00 pm on March 15, 2018: member

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 commented at 7:01 pm on March 15, 2018: member

Related discussion: https://botbot.me/freenode/bitcoin-core-dev/msg/97851598/
Sjors commented at 7:04 pm on March 15, 2018: member

Maybe just use the same ordering as BIP-69?
instagibbs commented at 7:16 pm on March 15, 2018: member

@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 commented at 7:22 pm on March 15, 2018: member

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 force-pushed on Mar 15, 2018
instagibbs commented at 8:19 pm on March 15, 2018: member

Smaller changeset now, had to move the final input creation further output to avoid storing shuffle ordering.
fanquake requested review from achow101 on Mar 15, 2018
instagibbs commented at 6:09 pm on March 16, 2018: member

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 commented at 9:44 pm on March 16, 2018: member

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 commented at 11:54 am on March 17, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12699/commits/8712bbd6f0f643359b5e6dd090cf247bee3dd087
shuffle selected coins before transaction finalization 2fb9c1e668
instagibbs force-pushed on Mar 21, 2018
instagibbs commented at 7:04 pm on March 21, 2018: member

rebased onto #12742 to avoid code churn
sipa commented at 11:39 pm on March 21, 2018: member

utACK 2fb9c1e6681370478e24a19172ed6d78d95d50d3

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

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

0EDITOR=true git commit --allow-empty -m empty && git push git@github.com:instagibbs/bitcoin.git shuffleinputs && git reset --hard 2fb9c1e && git push git@github.com:instagibbs/bitcoin.git shuffleinputs -f

instagibbs force-pushed on Mar 22, 2018
MarcoFalke commented at 5:54 pm on March 22, 2018: member

utACK 2fb9c1e6681370478e24a19172ed6d78d95d50d3
MarcoFalke referenced this in commit 02b7e8319a on Mar 23, 2018
promag commented at 12:13 pm on March 24, 2018: member

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 commented at 8:03 pm on March 24, 2018: member

PR title seems outdated now (the shuffle happens after SelectCoins returns).
instagibbs renamed this:
~~[wallet] Shuffle transaction inputs before returning from SelectCoins~~
[wallet] Shuffle transaction inputs before signing
on Mar 26, 2018
instagibbs commented at 1:42 pm on March 26, 2018: member

updated title
jamesob commented at 2:55 pm on March 26, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12699/commits/2fb9c1e6681370478e24a19172ed6d78d95d50d3
laanwj merged this on Mar 26, 2018
laanwj closed this on Mar 26, 2018
laanwj referenced this in commit c948dc8f42 on Mar 26, 2018
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
MarcoFalke locked this on Sep 8, 2021

Contributors
instagibbs laanwj ryanofsky Sjors luke-jr donaloconnor sipa MarcoFalke promag jamesob

Review Requested
achow101

Labels
Wallet