rpc: combinerawtransaction now rejects unmergeable transactions

adamandrews1 commented at 9:24 pm on November 15, 2024: none

Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.

fixes #25980

DrahtBot commented at 9:24 pm on November 15, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31298.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	zaidmstrr
Stale ACK	1440000bytes, i-am-yuvi

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Nov 15, 2024

adamandrews1 force-pushed on Nov 15, 2024

DrahtBot added the label CI failed on Nov 15, 2024

DrahtBot commented at 9:37 pm on November 15, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33065613881

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

adamandrews1 marked this as a draft on Nov 15, 2024

adamandrews1 force-pushed on Nov 15, 2024

adamandrews1 force-pushed on Nov 20, 2024

adamandrews1 marked this as ready for review on Nov 20, 2024

adamandrews1 commented at 2:08 am on November 20, 2024: none

It seems like, with #31249, I should migrate these test changes as well. WIP

DrahtBot removed the label CI failed on Nov 20, 2024

1440000bytes commented at 9:31 pm on November 20, 2024: none

Can this use the same approach and error message as combinepsbt?

adamandrews1 commented at 10:32 pm on November 20, 2024: none

Can this use the same approach and error message as combinepsbt?

This is the same functional approach and error message already. Creating a PSBT involves stripping the scriptSig and scriptWitnesses. Later, the hashes are compared. That’s exactly what this change is doing too but the syntax is different. I can align the syntax to make it more clear it is doing the same thing.

adamandrews1 force-pushed on Nov 20, 2024

1440000bytes commented at 7:52 am on November 21, 2024: none

Later, the hashes are compared. That’s exactly what this change is doing too but the syntax is different.

0bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
1{
2    // Prohibited to merge two PSBTs over different transactions
3    if (tx->GetHash() != psbt.tx->GetHash()) {
4        return false;
5    }

Right. Only the error message was different which looks same now,

1440000bytes approved

1440000bytes commented at 7:52 am on November 21, 2024: none

utACK https://github.com/bitcoin/bitcoin/pull/31298/commits/85c216871a01aab003e1b1aba6246b4178afc929

i-am-yuvi approved

i-am-yuvi commented at 7:26 pm on December 18, 2024: contributor

Tested ACK 85c216871a01aab003e1b1aba6246b4178afc929

Below is my full test setup:

Tested on Regtest mode

master@d2136d32bb4764e4035fb892d979c27f037fad93

When checked with only 1 raw txn, returns only the txn (no error for missing txns)
When checked for mergeability:
- returns only the first txn when unrelated txns were provided
- doesn’t show descriptive error for any mergeability issue

combinerawtransaction-check@85c216871a01aab003e1b1aba6246b4178afc929

When only 1 raw txn provided it returns missing inputs
checking for mergeability:
- shows descriptive error for mergeability issue for e.g.

0error code: -8
1error message:
2Transaction 1 not compatible (different transactions)

Gives combine hex when same txns are provided

0./build/src/bitcoin-cli combinerawtransaction '["0200000001a0d2e799d40d404da2daf128fc678fdd15f245bdd92c6e1ad15b45f7258a47b90000000000fdffffff01008c86470000000016001483f259c98962117d75154f35cb55cd60b88a257e00000000","0200000001a0d2e799d40d404da2daf128fc678fdd15f245bdd92c6e1ad15b45f7258a47b90000000000fdffffff01008c86470000000016001483f259c98962117d75154f35cb55cd60b88a257e00000000"]'
10200000001a0d2e799d40d404da2daf128fc678fdd15f245bdd92c6e1ad15b45f7258a47b90000000000fdffffff01008c86470000000016001483f259c98962117d75154f35cb55cd60b88a257e00000000

for inputs already spent:

0error code: -25
1error message:
2Input not found or already spent

Note: newly created wallet and txns were used during the test using createrawtransaction rpc in regtest mode

Also, the functional test LGTM:

0Temporary test directory at /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/test_runner_₿_🏃_20241219_004940
1Remaining jobs: [rpc_createmultisig.py]
21/1 - rpc_createmultisig.py passed, Duration: 3 s
3
4TEST                  | STATUS    | DURATION
5
6rpc_createmultisig.py | ✓ Passed  | 3 s
7
8ALL                   | ✓ Passed  | 3 s (accumulated) 
9Runtime: 3 s

in src/rpc/rawtransaction.cpp:663 in 85c216871a outdated

672+        Txid txid{};
673+        for (unsigned int k{0}; k < txVariantsCopy.size(); k++) {
674+            // Remove all scriptSigs and scriptWitnesses from inputs
675+            for (CTxIn& input : txVariantsCopy[k].vin) {
676+                input.scriptSig.clear();
677+                input.scriptWitness.SetNull();

danielabrozzoni commented at 6:17 pm on December 27, 2024:

I don’t think it’s needed to clear the script witness, as it’s not taken into account when calculating the txid

adamandrews1 commented at 11:00 pm on January 13, 2025:

This is done to align the method with PSBT. It could be removed without breaking the change. But it also is just clearing data which should take minimal time. I think it’s worth keeping to align with PSBT format.

brunoerg commented at 11:29 pm on January 14, 2025:

No objections on it but maybe you could update the documentation, it says: “This involves stripping sigscripts from each tx and ensuring txid is consistent.” but you’re also stripping the script witness. It could sound weird for those who doesn’t know you’re just keeping alignment with PSBT.

adamandrews1 commented at 10:17 pm on February 15, 2025:

@brunoerg I think you are right, I will update the description here to make it explicit we are tracking PSBT here 👍🏻

adamandrews1 force-pushed on Feb 16, 2025

adamandrews1 requested review from brunoerg on Feb 16, 2025

adamandrews1 requested review from danielabrozzoni on Feb 16, 2025

in src/rpc/rawtransaction.cpp:650 in 85d798db1a outdated

646+    }
647+
648     std::vector<CMutableTransaction> txVariants(txs.size());
649 
650-    for (unsigned int idx = 0; idx < txs.size(); idx++) {
651+    for (unsigned int idx {0}; idx < txs.size(); idx++) {

zaidmstrr commented at 11:06 am on February 19, 2025:

0    for (unsigned int idx{0}; idx < txs.size(); idx++) {

nit: don’t need to add the extra space

i-am-yuvi commented at 7:35 pm on February 20, 2025:

It might be helpful to consider giving a Concept ACK or NACK before the review. I came across this idea here.

zaidmstrr commented at 8:49 pm on February 20, 2025:

Done

adamandrews1 commented at 9:31 pm on February 20, 2025:

Fixed nit

in src/rpc/rawtransaction.cpp:659 in 85d798db1a outdated

657-    if (txVariants.empty()) {
658-        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
659+    { // Test Tx relation for mergeability. This involves converting each tx to PSBT format (stripping sigscripts/witnesses) and ensuring txid is consistent.
660+        std::vector<CMutableTransaction> txVariantsCopy(txVariants);
661+        Txid txid{};
662+        for (unsigned int k{0}; k < txVariantsCopy.size(); k++) {

zaidmstrr commented at 11:07 am on February 19, 2025:

0        for (unsigned int k{0}; k < txVariantsCopy.size(); ++k) {

nit: ++k should be given more prefrence then k++

adamandrews1 commented at 9:31 pm on February 20, 2025:

Fixed nit

in src/rpc/rawtransaction.cpp:702 in 85d798db1a outdated

698@@ -678,7 +699,7 @@ static RPCHelpMan combinerawtransaction()
699     // transaction to avoid rehashing.
700     const CTransaction txConst(mergedTx);
701     // Sign what we can:
702-    for (unsigned int i = 0; i < mergedTx.vin.size(); i++) {
703+    for (unsigned int i{0}; i < mergedTx.vin.size(); i++) {

zaidmstrr commented at 11:15 am on February 19, 2025:

0    for (unsigned int i{0}; i < mergedTx.vin.size(); ++i) {

nit: same for here ++i should be prioritised.

adamandrews1 commented at 9:31 pm on February 20, 2025:

Fixed nit

zaidmstrr commented at 8:47 pm on February 20, 2025: none

Concept ACK

rpc: combinerawtransaction now rejects unmergeable transactions

Previously, combinerawtransaction would silently return the first tx when
asked to combine unrelated txs. Now, it will check tx mergeability and
throws a descriptive error if tx cannot be merged.

9d31e94bf5

adamandrews1 force-pushed on Feb 20, 2025

rpc: combinerawtransaction now rejects unmergeable transactions #31298

Code Coverage & Benchmarks

Reviews

Conflicts

Tested on Regtest mode

master@d2136d32bb4764e4035fb892d979c27f037fad93

combinerawtransaction-check@85c216871a01aab003e1b1aba6246b4178afc929