rpc: Shuffle inputs and outputs after joining psbts #16512

achow101 · 2019-07-31T22:11:38Z

joinpsbts currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction.

promag

Concept ACK.

src/rpc/rawtransaction.cpp

laanwj · 2019-08-01T12:47:47Z

Concept ACK, not sure whether there's reasons someone would want to disable this, but in any case I think privacy-by-default behavior is good

practicalswift · 2019-08-01T16:44:55Z

Concept ACK

fanquake · 2019-08-14T09:37:23Z

Concept ACK - Can we get some sort of simple test case to ensure that there's some shuffling going on.

but in any case I think privacy-by-default behavior is good

Definitely agree.

I guess this should also be backported given the patch is fairly simple and it's a privacy improvement?

achow101 · 2019-08-14T18:31:16Z

I've added a test. It runs joinpsbts multiple times and checks whether at least one of the resulting psbts are different.

promag

ACK, but could add a release note regarding the behavior change and also improve help text.

test/functional/rpc_psbt.py

src/rpc/rawtransaction.cpp

achow101 · 2019-08-15T00:22:51Z

Added a release note.

DrahtBot · 2019-08-15T01:10:56Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16439 (RPC: support "@height" in place of blockhash for getblock etc by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr · 2019-08-20T00:55:54Z

Only bugfixes should be backported, not improvements...

JeremyRubin · 2019-08-20T01:37:00Z

Wouldn't it be better to deterministically sort them?

achow101 · 2019-08-20T02:33:00Z

Wouldn't it be better to deterministically sort them?

Not at all. That gives a different privacy leak as it will indicate that joinpsbts has been used on that transaction.

JeremyRubin · 2019-08-20T17:28:00Z

Hm gotcha. I was unclear of the status of BIP-69 and related standard adoption efforts for Core

instagibbs · 2019-08-27T14:47:34Z

Only bugfixes should be backported, not improvements...

Privacy leak is a bug imo. Everywhere else we(should be) shuffle.

instagibbs · 2019-08-27T14:49:52Z

test/functional/rpc_psbt.py

@@ -370,6 +370,16 @@ def test_psbt_input_keys(psbt_input, keys):
        joined_decoded = self.nodes[0].decodepsbt(joined)
        assert len(joined_decoded['inputs']) == 4 and len(joined_decoded['outputs']) == 2 and "final_scriptwitness" not in joined_decoded['inputs'][3] and "final_scriptSig" not in joined_decoded['inputs'][3]

+        # Check that joining shuffles the inputs and outputs
+        # 10 attempts should be enough to get a shuffled join
+        shuffled = False


I think it's simpler to just do an infinite loop and break IFF a difference is found, makes it less dependent on number of inputs and lowers false negatives

Do you mean just replacing the line below with while True:? If you mean to drop shuffled then what would you assert after the loop?

just saying remove the 10 times limit, maybe makes the logic simpler. hanging with implementation bug is better than false negative imo

I agree with you but it it isn't that informative. Suppose it hangs in travis, you would be clueless.

Wouldn't be clueless if it printed out what tests are still running, but those PRs have been unable to get merged, sad!

Could just bump it to some fairly ridiculous number that's unlikely to be hit even if the test itself is modified significantly. I think the easiest case to get a false negative is just two entries, 50/50 chance, so run that 128 times and we're fine :P

I think 10 is enough for now.

first test failure you're getting strung up :P

instagibbs · 2019-08-27T14:54:02Z

src/rpc/rawtransaction.cpp

@@ -1604,8 +1605,28 @@ UniValue joinpsbts(const JSONRPCRequest& request)
        merged_psbt.unknown.insert(psbt.unknown.begin(), psbt.unknown.end());
    }

+    // Generate list of shuffled indices for shuffling inputs and outputs of the merged PSBT
+    std::vector<int> input_indices(merged_psbt.inputs.size());
+    std::iota(input_indices.begin(), input_indices.end(), 0);


std::iota

not sure we should be adding IOTA support to Core

(this is a joke)

instagibbs · 2019-08-27T15:02:11Z

src/rpc/rawtransaction.cpp

@@ -1604,8 +1605,28 @@ UniValue joinpsbts(const JSONRPCRequest& request)
        merged_psbt.unknown.insert(psbt.unknown.begin(), psbt.unknown.end());
    }

+    // Generate list of shuffled indices for shuffling inputs and outputs of the merged PSBT


Forgive me, why are we not just shuffling the vector of inputs and outputs in merged_psbt using Shuffle directly?

There are two input vectors and output vectors. The on in the global tx, and the one in the psbt metadata. Both need to line up, so by shuffling the indicies, each input and output can be lined up with its respective metadata.

ah! Missed how the actual transaction was being reconstructed, got it.

jonatack

Concept ACK b1d769f. Code review, built, ran tests, verified the added test fails without the change. Code looks correct; a few suggestions follow for your perusal. Is testing joinpsbt inequality in rpc_psbt.py enough? Could the manual copy+shuffling be extracted and covered by unit tests? e.g. as a member function of PartiallySignedTransaction?

jonatack · 2019-09-04T10:32:21Z

test/functional/rpc_psbt.py

+        # Check that joining shuffles the inputs and outputs
+        # 10 attempts should be enough to get a shuffled join
+        shuffled = False
+        for i in range(0, 10):


Suggested change:

- for i in range(0, 10): + for _ in range(10):

meh. not important enough to change

jonatack · 2019-09-04T10:34:18Z

test/functional/rpc_psbt.py

@@ -370,6 +370,16 @@ def test_psbt_input_keys(psbt_input, keys):
        joined_decoded = self.nodes[0].decodepsbt(joined)
        assert len(joined_decoded['inputs']) == 4 and len(joined_decoded['outputs']) == 2 and "final_scriptwitness" not in joined_decoded['inputs'][3] and "final_scriptSig" not in joined_decoded['inputs'][3]

+        # Check that joining shuffles the inputs and outputs


Suggested change:

- # Check that joining shuffles the inputs and outputs - # 10 attempts should be enough to get a shuffled join + # Check that joining shuffles the inputs and outputs. + # Run up to 10 attempts to ensure seeing a shuffled join.

meh. not important enough to change

jonatack · 2019-09-04T10:42:43Z

src/rpc/rawtransaction.cpp

@@ -1604,8 +1605,28 @@ UniValue joinpsbts(const JSONRPCRequest& request)
        merged_psbt.unknown.insert(psbt.unknown.begin(), psbt.unknown.end());
    }

+    // Generate list of shuffled indices for shuffling inputs and outputs of the merged PSBT


Suggested change for git grepping, mainly the line "// Shuffle input and output indices lists" to have a comment beginning with "Shuffle" like:

src/wallet/rpcwallet.cpp:913: // Shuffle recipient list src/wallet/wallet.cpp:3211: // Shuffle selected coins and fill in final vin

// Generate lists of input and output indices of the merged PSBT // to be shuffled. std::vector<int> input_indices(merged_psbt.inputs.size()); std::iota(input_indices.begin(), input_indices.end(), 0); std::vector<int> output_indices(merged_psbt.outputs.size()); std::iota(output_indices.begin(), output_indices.end(), 0); // Shuffle input and output indices lists Shuffle(input_indices.begin(), input_indices.end(), FastRandomContext()); Shuffle(output_indices.begin(), output_indices.end(), FastRandomContext()); // Generate shuffled_psbt from shuffled indices lists PartiallySignedTransaction shuffled_psbt;

Note that the proposed change places the two Shuffle functions together. First generate the lists, then perform the shuffles.

doc/release-notes-16512.md

nopara73 · 2019-09-04T14:42:35Z

Concept ACK. Related: #12457

jonatack · 2019-09-05T21:03:17Z

src/rpc/rawtransaction.cpp

+    std::vector<int> output_indices(merged_psbt.outputs.size());
+    std::iota(output_indices.begin(), output_indices.end(), 0);
+
+    // Shuffle input and output indicies lists


nit (if you need to retouch this): s/indicies/indices/

jonatack · 2019-09-17T16:58:07Z

@achow101 what did you think of this question / suggestion above?

achow101 · 2019-09-17T17:07:58Z

Is testing joinpsbt inequality in rpc_psbt.py enough?

It should be.

Could the manual copy+shuffling be extracted and covered by unit tests? e.g. as a member function of PartiallySignedTransaction?

Maybe later? I would like this merged for 0.19 so I do want any further changes to this to be minimal.

instagibbs · 2019-09-17T17:08:13Z

utACK c0b5d97

jonatack · 2019-09-17T17:16:22Z

Could the manual copy+shuffling be extracted and covered by unit tests? e.g. as a member function of PartiallySignedTransaction?

Maybe later? I would like this merged for 0.19 so I do want any further changes to this to be minimal.

Fair enough. I might propose it later.

jonatack · 2019-09-18T12:58:13Z

ACK c0b5d97 modulo suggestions for later.

laanwj · 2019-09-18T14:19:44Z

ACK c0b5d97

Agree the shuffling should ideally be factored out to a function, but that can be done in a future PR

c0b5d97 Test that joinpsbts randomly shuffles the inputs (Andrew Chow) 6f405a1 Shuffle inputs and outputs after joining psbts (Andrew Chow) Pull request description: `joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction. ACKs for top commit: instagibbs: utACK c0b5d97 jonatack: ACK c0b5d97 modulo suggestions for later. Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8

Github-Pull: bitcoin#16512 Rebased-From: 6f405a1

Github-Pull: bitcoin#16512 Rebased-From: c0b5d97

fanquake · 2019-09-19T01:59:04Z

Added for backport in #16617.

c0b5d97 Test that joinpsbts randomly shuffles the inputs (Andrew Chow) 6f405a1 Shuffle inputs and outputs after joining psbts (Andrew Chow) Pull request description: `joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction. ACKs for top commit: instagibbs: utACK bitcoin@c0b5d97 jonatack: ACK c0b5d97 modulo suggestions for later. Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8

Github-Pull: bitcoin#16512 Rebased-From: 6f405a1

Github-Pull: bitcoin#16512 Rebased-From: c0b5d97

0b18ea6 util: Filter control characters out of log messages (Wladimir J. van der Laan) ac30fc4 build: Factor out qt translations from build system (Wladimir J. van der Laan) 3b8af5f build: update boost macros to latest upstream (fanquake) b12defc Test that joinpsbts randomly shuffles the inputs (Andrew Chow) eb07d22 Shuffle inputs and outputs after joining psbts (Andrew Chow) 1175410 addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. (practicalswift) c52dd12 Handle the result of posix_fallocate system call (Luca Venturini) f792b25 torcontrol: Use the default/standard network port for Tor hidden services, even if the internal port is set differently (Luke Dashjr) 9fe8d28 Bugfix: QA: Run tests with UPnP disabled (Luke Dashjr) 1d12e52 Add vertical spacer (Josu Goñi) d764141 depends: add patch to common dependencies (fanquake) 56815e9 Give QApplication dummy arguments (Andrew Chow) 9d389d0 util: No translation of `Bitcoin Core` in the copyright (MarcoFalke) 87908e9 scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke) a44e18f build: Stop translating PACKAGE_NAME (MarcoFalke) 7bd8f4e rpc: Fix getblocktemplate CLI example (#16594) (Emil Engler) 1cc06a1 doc: Fix typos in COPYRIGHT (Chuf) Pull request description: Backports some commits to the `0.18` branch: * #16596 - rpc: Fix getblocktemplate CLI example * #16615 - doc: Fix typos in COPYRIGHT * #16291 - gui: Stop translating PACKAGE_NAME (without the `make translate` commit) * #16578 - Do not pass in command line arguments to QApplication * #16051 - depends: add patch to common dependencies * #16090 - Add vertical spacer * #15651 - torcontrol: Use the default/standard network port for Tor hidden services, even if the internal port is set differently * #15650 - Handle the result of posix_fallocate system call * #16646 - Bugfix: QA: Run tests with UPnP disabled * #16212 - addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. * #16512 - rpc: Shuffle inputs and outputs after joining psbts * #16870 - build: update boost macros to latest upstream for improved error reporting * #16982 - build: Factor out qt translations from build system * #17095 - util: Filter control characters out of log messages ACKs for top commit: laanwj: ACK 0b18ea6 Tree-SHA512: 37f0e5afc20975f4d1506e8662eda2ae0125f2f424a852818b5af2c3b8db78fc1c365b83571aa80ca63c885ca314302190b891a50ff3851fda9b9238455a5627

Summary: Pull request description: `joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction. --- Backport of Core [[bitcoin/bitcoin#16512 | PR16512]] Test Plan: ninja ./test/functional/test_runner.py rpc_psbt Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6665

c0b5d97 Test that joinpsbts randomly shuffles the inputs (Andrew Chow) 6f405a1 Shuffle inputs and outputs after joining psbts (Andrew Chow) Pull request description: `joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction. ACKs for top commit: instagibbs: utACK bitcoin@c0b5d97 jonatack: ACK c0b5d97 modulo suggestions for later. Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8

fanquake added the RPC/REST/ZMQ label Jul 31, 2019

promag reviewed Jul 31, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

fanquake added the Privacy label Aug 1, 2019

achow101 force-pushed the joinpsbt-rand branch from a114e33 to ec0af57 Compare August 1, 2019 19:56

fanquake added the Needs backport label Aug 14, 2019

fanquake added this to the 0.18.2 milestone Aug 14, 2019

promag reviewed Aug 14, 2019

View reviewed changes

test/functional/rpc_psbt.py Show resolved Hide resolved

test/functional/rpc_psbt.py Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the joinpsbt-rand branch 2 times, most recently from 078b29f to b1d769f Compare August 15, 2019 00:22

instagibbs reviewed Aug 27, 2019

View reviewed changes

jonatack reviewed Sep 4, 2019

View reviewed changes

achow101 added 2 commits September 4, 2019 17:44

Shuffle inputs and outputs after joining psbts

6f405a1

Test that joinpsbts randomly shuffles the inputs

c0b5d97

achow101 force-pushed the joinpsbt-rand branch from b1d769f to c0b5d97 Compare September 4, 2019 21:44

jonatack reviewed Sep 5, 2019

View reviewed changes

maflcko modified the milestones: 0.18.2, 0.19.0 Sep 17, 2019

fanquake changed the title ~~Shuffle inputs and outputs after joining psbts~~ rpc: Shuffle inputs and outputs after joining psbts Sep 18, 2019

laanwj merged commit c0b5d97 into bitcoin:master Sep 18, 2019

ryanofsky mentioned this pull request Sep 18, 2019

Replace Connman and BanMan globals with NodeContext local #16839

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 19, 2019

Shuffle inputs and outputs after joining psbts

1c1ad3c

Github-Pull: bitcoin#16512 Rebased-From: 6f405a1

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 19, 2019

Test that joinpsbts randomly shuffles the inputs

2129941

Github-Pull: bitcoin#16512 Rebased-From: c0b5d97

fanquake removed the Needs backport label Sep 19, 2019

fanquake mentioned this pull request Sep 19, 2019

[0.18.2] Backports #16617

Merged

maflcko modified the milestones: 0.19.0, 0.18.2 Sep 23, 2019

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 23, 2019

Shuffle inputs and outputs after joining psbts

eb07d22

Github-Pull: bitcoin#16512 Rebased-From: 6f405a1

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 23, 2019

Test that joinpsbts randomly shuffles the inputs

b12defc

Github-Pull: bitcoin#16512 Rebased-From: c0b5d97

This was referenced Dec 6, 2021

Privacy Enhancement BTCGPU/BTCGPU#435

Open

Privacy Enhancement digibyte/digibyte#236

Open

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

rpc: Shuffle inputs and outputs after joining psbts #16512

rpc: Shuffle inputs and outputs after joining psbts #16512

Uh oh!

Conversation

achow101 commented Jul 31, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Aug 1, 2019

Uh oh!

practicalswift commented Aug 1, 2019

Uh oh!

fanquake commented Aug 14, 2019

Uh oh!

achow101 commented Aug 14, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Aug 15, 2019

Uh oh!

DrahtBot commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr commented Aug 20, 2019

Uh oh!

JeremyRubin commented Aug 20, 2019

Uh oh!

achow101 commented Aug 20, 2019

Uh oh!

JeremyRubin commented Aug 20, 2019

Uh oh!

instagibbs commented Aug 27, 2019

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!

instagibbs Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

instagibbs Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DrahtBot commented Aug 15, 2019 •

edited

Loading

instagibbs Aug 27, 2019 •

edited

Loading

instagibbs Aug 27, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

achow101 Sep 4, 2019 •

edited

Loading

achow101 Sep 4, 2019 •

edited

Loading

jonatack Sep 4, 2019 •

edited

Loading