Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rpc: Shuffle inputs and outputs after joining psbts #16512

Merged
merged 2 commits into from
Sep 18, 2019
Merged

rpc: Shuffle inputs and outputs after joining psbts #16512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f405a1
Shuffle inputs and outputs after joining psbts
achow101 Jul 31, 2019
c0b5d97
Test that joinpsbts randomly shuffles the inputs
achow101 Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/release-notes-16512.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
RPC changes
-----------
The RPC `joinpsbts` will shuffle the order of the inputs and outputs of the resulting joined psbt.
fanquake marked this conversation as resolved.
Show resolved Hide resolved
Previously inputs and outputs were added in the order that the PSBTs were provided which makes correlating inputs to outputs extremely easy.
25 changes: 24 additions & 1 deletion src/rpc/rawtransaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <policy/rbf.h>
#include <primitives/transaction.h>
#include <psbt.h>
#include <random.h>
#include <rpc/rawtransaction_util.h>
#include <rpc/server.h>
#include <rpc/util.h>
Expand Down Expand Up @@ -1604,8 +1605,30 @@ 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
Copy link
Member

@instagibbs instagibbs Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jonatack jonatack Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

std::vector<int> input_indices(merged_psbt.inputs.size());
std::iota(input_indices.begin(), input_indices.end(), 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::iota

not sure we should be adding IOTA support to Core

(this is a joke)

std::vector<int> output_indices(merged_psbt.outputs.size());
std::iota(output_indices.begin(), output_indices.end(), 0);

// Shuffle input and output indicies lists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Shuffle(input_indices.begin(), input_indices.end(), FastRandomContext());
Shuffle(output_indices.begin(), output_indices.end(), FastRandomContext());

PartiallySignedTransaction shuffled_psbt;
shuffled_psbt.tx = CMutableTransaction();
shuffled_psbt.tx->nVersion = merged_psbt.tx->nVersion;
shuffled_psbt.tx->nLockTime = merged_psbt.tx->nLockTime;
for (int i : input_indices) {
shuffled_psbt.AddInput(merged_psbt.tx->vin[i], merged_psbt.inputs[i]);
}
for (int i : output_indices) {
shuffled_psbt.AddOutput(merged_psbt.tx->vout[i], merged_psbt.outputs[i]);
}
shuffled_psbt.unknown.insert(merged_psbt.unknown.begin(), merged_psbt.unknown.end());

CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << merged_psbt;
ssTx << shuffled_psbt;
return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());
}

Expand Down
10 changes: 10 additions & 0 deletions test/functional/rpc_psbt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
fanquake marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@achow101 achow101 Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh. not important enough to change

# 10 attempts should be enough to get a shuffled join
shuffled = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@instagibbs instagibbs Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 10 is enough for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first test failure you're getting strung up :P

for i in range(0, 10):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change:

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

Copy link
Member Author

@achow101 achow101 Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh. not important enough to change

shuffled_joined = self.nodes[0].joinpsbts([psbt, psbt2])
shuffled |= joined != shuffled_joined
fanquake marked this conversation as resolved.
Show resolved Hide resolved
if shuffled:
break
assert shuffled

# Newly created PSBT needs UTXOs and updating
addr = self.nodes[1].getnewaddress("", "p2sh-segwit")
txid = self.nodes[0].sendtoaddress(addr, 7)
Expand Down