rpc: combinerawtransaction now rejects unmergeable transactions #31298

adamandrews1 · 2024-11-15T21:24:12Z

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 · 2024-11-15T21:24:15Z

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, theStack, rkrux
Stale ACK	1440000bytes, naiyoma, yuvicc, mossein

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

DrahtBot · 2024-11-15T21:37:59Z

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

Hints

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 · 2024-11-20T02:08:21Z

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

adamandrews1 · 2024-11-20T22:32:54Z

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.

1440000bytes

utACK 85c2168

yuvicc

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.

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

Gives combine hex when same txns are provided

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

for inputs already spent:

error code: -25
error message:
Input 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:

Temporary test directory at /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/test_runner_₿_🏃_20241219_004940
Remaining jobs: [rpc_createmultisig.py]
1/1 - rpc_createmultisig.py passed, Duration: 3 s

TEST                  | STATUS    | DURATION

rpc_createmultisig.py | ✓ Passed  | 3 s

ALL                   | ✓ Passed  | 3 s (accumulated) 
Runtime: 3 s

src/rpc/rawtransaction.cpp

zaidmstrr · 2025-02-20T20:47:39Z

Concept ACK

naiyoma

Tested ACK 9d31e94

I have tested the changes on regtest and got expected output

bitcoin-cli -regtest combinerawtransaction "[\"$RAW_TX1\", \"$RAW_TX2\"]"
error code: -8
error message:
Transaction 1 not compatible (different transactions)

src/rpc/rawtransaction.cpp

yuvicc

re-ACK 9d31e94bf584e12269683b74b1c96c7ac883a760

src/rpc/rawtransaction.cpp

achow101 · 2025-09-15T22:35:12Z

Are you still working on this?

adamandrews1 · 2025-09-16T03:35:33Z

@achow101 I've rebased this and updated the comment. No other changes.

theStack

Concept ACK

src/rpc/rawtransaction.cpp

test/functional/rpc_createmultisig.py

rkrux

Concept ACK a26e9fd5b5908853d37b81915f5e12a5ad822f86

Thanks for adding the fix.

src/rpc/rawtransaction.cpp

test/functional/rpc_createmultisig.py

src/rpc/rawtransaction.cpp

mossein · 2025-12-06T22:28:10Z

Tested ACK a26e9fd5b5908853d37b81915f5e12a5ad822f86

Built on macOS, ran
rpc_createmultisig.py

passes. The approach of stripping scriptSigs/scriptWitnesses and comparing txids is clean.

Agree with the minor nits from other reviewers (style changes, error message verbosity).

adamandrews1 · 2025-12-07T14:24:15Z

Are you still working on this?

Yes, I will produce an additional patch addressing all comments in the coming days.

adamandrews1 · 2025-12-08T15:04:24Z

I've addressed all nits and rebased. Thank you to each of you who left thoughtful commentary.

test/functional/rpc_createmultisig.py

rkrux

Overall looking quite good.

I noticed that these new cases are added within the do_multisig function that is called several times here:

bitcoin/test/functional/rpc_createmultisig.py

Lines 48 to 51 in 56ce78d

    
           m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)] 
        
           for (sigs, keys) in m_of_n: 
        
               for output_type in ["bech32", "p2sh-segwit", "legacy"]: 
        
                   self.do_multisig(keys, sigs, output_type)

Given the nature of the bug regarding the un-mergeable transactions, I feel testing this once should be sufficient in which case we can consider adding these cases in a separate function that should help in avoiding testing the fix multiple times. WDYT? (I have not formed a strong opinion on this though.)

test/functional/rpc_createmultisig.py

src/rpc/rawtransaction.cpp

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.

adamandrews1 · 2025-12-29T17:31:05Z

Overall looking quite good.

I noticed that these new cases are added within the do_multisig function that is called several times here:

bitcoin/test/functional/rpc_createmultisig.py

Lines 48 to 51 in 56ce78d

m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)]

for (sigs, keys) in m_of_n:

for output_type in ["bech32", "p2sh-segwit", "legacy"]:

self.do_multisig(keys, sigs, output_type)

Given the nature of the bug regarding the un-mergeable transactions, I feel testing this once should be sufficient in which case we can consider adding these cases in a separate function that should help in avoiding testing the fix multiple times. WDYT? (I have not formed a strong opinion on this though.)

The tx mergeability constraints are invariant across output_type and n_sig etc, it should be sufficient to test them one time explicitly and disable during all other runs. Probably not a huge runtime cost as-is but there is definitely a fan-out that would adds additional overhead to all callers of do_multisig so got rid of that. Also integrated all other feedback.

rkrux

The tx mergeability constraints are invariant across output_type and n_sig etc, it should be sufficient to test them one time explicitly and disable during all other runs. Probably not a huge runtime cost as-is but there is definitely a fan-out that would adds additional overhead to all callers of do_multisig so got rid of that. Also integrated all other feedback.

Thanks for addressing all the feedback! I had a different approach in mind that creates a separate function for these checks but the latest implementation also looks fine to me.

Code is lgtm e3069ec

Shared 2 nits that could be addressed at the moment I believe.

rkrux · 2025-12-30T13:04:43Z

src/rpc/rawtransaction.cpp

+        Txid txid{};
+        for (unsigned int k{0}; k < tx_variants_copy.size(); ++k) {
+            // Remove all scriptSigs and scriptWitnesses from inputs
+            for (CTxIn& input : tx_variants_copy[k].vin) {
+                input.scriptSig.clear();
+                input.scriptWitness.SetNull();
+            }
+            if (k == 0) {
+                txid = tx_variants_copy[k].GetHash();
+            } else if (txid != tx_variants_copy[k].GetHash()) {


Nit - this is set only once for the first transaction:

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 1de4d200e8..d0038f25ad 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -622,7 +622,7 @@ static RPCHelpMan combinerawtransaction() { // Test Tx relation for mergeability. Strip scriptSigs and scriptWitnesses to facilitate txId comparison std::vector<CMutableTransaction> tx_variants_copy(txVariants); - Txid txid{}; + Txid first_txid{}; for (unsigned int k{0}; k < tx_variants_copy.size(); ++k) { // Remove all scriptSigs and scriptWitnesses from inputs for (CTxIn& input : tx_variants_copy[k].vin) { @@ -630,8 +630,8 @@ static RPCHelpMan combinerawtransaction() input.scriptWitness.SetNull(); } if (k == 0) { - txid = tx_variants_copy[k].GetHash(); - } else if (txid != tx_variants_copy[k].GetHash()) { + first_txid = tx_variants_copy[k].GetHash(); + } else if (first_txid != tx_variants_copy[k].GetHash()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Transaction number %d not compatible with first transaction", k+1)); } }

rkrux · 2025-12-30T13:06:34Z

test/functional/rpc_createmultisig.py

+            out_addr2 = getnewdestination('bech32')[2]
+            unmergeable_transaction_args = [
+                ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval * 2}]], {}), # similar transaction but with different amount to send
+                ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}]], {"locktime": 0, "replaceable": True, "version": 1}), # similar transaction but with different version


Because non-positional arguments are used here for version, rest 2 (locktime and replaceable) are not required:

diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index fa0f668922..199193cbbd 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -162,7 +162,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): out_addr2 = getnewdestination('bech32')[2] unmergeable_transaction_args = [ ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval * 2}]], {}), # similar transaction but with different amount to send - ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}]], {"locktime": 0, "replaceable": True, "version": 1}), # similar transaction but with different version + ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}]], {"version": 1}), # similar transaction but with different version ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}]], {"locktime": 1}), # similar transaction but with different locktime ([[{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr2: outval}]], {}), # similar transaction but with different output address (scriptPubKey) ([[{"txid": tx["txid"], "vout": tx["sent_vout"], "sequence": 1}], [{out_addr: outval}]], {}), # similar transaction but with different sequence number (END)

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

adamandrews1 force-pushed the combinerawtransaction-check branch from c0d23a0 to 8515fc6 Compare November 15, 2024 21:37

DrahtBot added the CI failed label Nov 15, 2024

adamandrews1 marked this pull request as draft November 15, 2024 22:03

adamandrews1 force-pushed the combinerawtransaction-check branch 2 times, most recently from 9b24f38 to 192a50b Compare November 15, 2024 22:46

DrahtBot mentioned this pull request Nov 16, 2024

wallet: Disable creating and loading legacy wallets #31250

Merged

adamandrews1 force-pushed the combinerawtransaction-check branch from 192a50b to f19a815 Compare November 20, 2024 02:02

adamandrews1 marked this pull request as ready for review November 20, 2024 02:03

DrahtBot removed the CI failed label Nov 20, 2024

This comment was marked as abuse.

Sign in to view

adamandrews1 force-pushed the combinerawtransaction-check branch from f19a815 to 85c2168 Compare November 20, 2024 23:44

This comment was marked as abuse.

Sign in to view

1440000bytes approved these changes Nov 21, 2024

View reviewed changes

yuvicc approved these changes Dec 18, 2024

View reviewed changes

danielabrozzoni reviewed Dec 27, 2024

View reviewed changes

src/rpc/rawtransaction.cpp Show resolved Hide resolved

adamandrews1 force-pushed the combinerawtransaction-check branch from 85c2168 to 85d798d Compare February 16, 2025 02:39

adamandrews1 requested review from brunoerg and danielabrozzoni February 16, 2025 02:39

zaidmstrr reviewed Feb 19, 2025

View reviewed changes

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

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

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

adamandrews1 force-pushed the combinerawtransaction-check branch from 85d798d to 9d31e94 Compare February 20, 2025 21:31

adamandrews1 mentioned this pull request Mar 6, 2025

rpc: Support v3 raw transactions creation #31936

Closed

adamandrews1 requested a review from zaidmstrr March 6, 2025 22:58

adamandrews1 mentioned this pull request May 1, 2025

combinerawtransaction confusing with distinct transactions #25980

Open

naiyoma reviewed May 4, 2025

View reviewed changes

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

yuvicc reviewed Jun 6, 2025

View reviewed changes

achow101 reviewed Jun 27, 2025

View reviewed changes

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

adamandrews1 force-pushed the combinerawtransaction-check branch from 4e31bbe to a26e9fd Compare September 16, 2025 03:32

adamandrews1 requested a review from achow101 September 16, 2025 03:35

achow101 mentioned this pull request Sep 17, 2025

Fix #25980: Validate transactions in combinerawtransaction #33361

Closed

theStack reviewed Sep 24, 2025

View reviewed changes

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

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved

rkrux reviewed Nov 7, 2025

View reviewed changes

src/rpc/rawtransaction.cpp Show resolved Hide resolved

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

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved

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

achow101 reviewed Nov 18, 2025

View reviewed changes

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

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

DrahtBot requested review from naiyoma, rkrux, theStack and yuvicc December 6, 2025 22:28

adamandrews1 force-pushed the combinerawtransaction-check branch from a26e9fd to a516fe8 Compare December 8, 2025 15:00

adamandrews1 requested a review from achow101 December 8, 2025 15:02

maflcko reviewed Dec 9, 2025

View reviewed changes

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved

rkrux suggested changes Dec 10, 2025

View reviewed changes

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved

test/functional/rpc_createmultisig.py Outdated Show resolved Hide resolved

achow101 reviewed Dec 24, 2025

View reviewed changes

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

rpc: combinerawtransaction now rejects unmergeable transactions

e3069ec

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.

adamandrews1 force-pushed the combinerawtransaction-check branch from a516fe8 to e3069ec Compare December 29, 2025 18:15

adamandrews1 requested review from achow101, maflcko and rkrux December 29, 2025 18:42

rkrux reviewed Dec 30, 2025

View reviewed changes

	m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)]
	for (sigs, keys) in m_of_n:
	for output_type in ["bech32", "p2sh-segwit", "legacy"]:
	self.do_multisig(keys, sigs, output_type)

rpc: combinerawtransaction now rejects unmergeable transactions #31298

Are you sure you want to change the base?

rpc: combinerawtransaction now rejects unmergeable transactions #31298

Conversation

adamandrews1 commented Nov 15, 2024

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Nov 15, 2024

Uh oh!

adamandrews1 commented Nov 20, 2024

Uh oh!

This comment was marked as abuse.

adamandrews1 commented Nov 20, 2024

Uh oh!

This comment was marked as abuse.

1440000bytes left a comment

Choose a reason for hiding this comment

Uh oh!

yuvicc left a comment

Choose a reason for hiding this comment

Tested on Regtest mode

master@d2136d32bb4764e4035fb892d979c27f037fad93

combinerawtransaction-check@85c216871a01aab003e1b1aba6246b4178afc929

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zaidmstrr commented Feb 20, 2025

Uh oh!

naiyoma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuvicc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Sep 15, 2025

Uh oh!

adamandrews1 commented Sep 16, 2025

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mossein commented Dec 6, 2025

Uh oh!

adamandrews1 commented Dec 7, 2025

Uh oh!

adamandrews1 commented Dec 8, 2025

Uh oh!

Uh oh!

rkrux 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamandrews1 commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

DrahtBot commented Nov 15, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading

adamandrews1 commented Dec 29, 2025 •

edited

Loading

rkrux left a comment •

edited

Loading