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
ACK	naiyoma, yuvicc
Concept ACK	zaidmstrr
Stale ACK	1440000bytes

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 · 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 85c2168

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

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.

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)

naiyoma · 2025-05-04T20:27:53Z

src/rpc/rawtransaction.cpp

+
+    // Can't merge < 2 items
+    if (txs.size() < 2) {
+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");


You can ignore this as everything LGTM, but if you have to push any other changes, you can include this as well

nit: IMO, this could be a bit more descriptive

// Can't merge < 2 items if (txs.size() < 2) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions"); + throw JSONRPCError( + RPC_INVALID_PARAMETER, + txs.size() == 0 + ? "Missing transactions. At least two transactions required." + : "Insufficient transactions. At least two transactions required." + ); }

yuvicc

re-ACK 9d31e94

achow101 · 2025-06-27T21:11:53Z

src/rpc/rawtransaction.cpp

        if (!DecodeHexTx(txVariants[idx], txs[idx].get_str())) {
            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed for tx %d. Make sure the tx has at least one input.", idx));
        }
    }

-    if (txVariants.empty()) {
-        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
+    { // Test Tx relation for mergeability. This involves converting each tx to PSBT format (stripping sigscripts/witnesses) and ensuring txid is consistent.


This comment is incorrect, it's not doing anything related to PSBT. You can just say that the scriptSigs and scriptWitnesses are being stripped in order to do a txid comparison.

Thanks, I will update this comment

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

rpc: combinerawtransaction now rejects unmergeable transactions

9d31e94

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

DrahtBot requested review from yuvicc and 1440000bytes May 4, 2025 19:38

naiyoma reviewed May 4, 2025

View reviewed changes

yuvicc reviewed Jun 6, 2025

View reviewed changes

achow101 reviewed Jun 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: combinerawtransaction now rejects unmergeable transactions #31298

rpc: combinerawtransaction now rejects unmergeable transactions #31298

adamandrews1 commented Nov 15, 2024

Uh oh!

DrahtBot commented Nov 15, 2024 •

edited

Loading

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

Uh oh!

yuvicc left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zaidmstrr commented Feb 20, 2025

Uh oh!

naiyoma left a comment

Uh oh!

naiyoma May 4, 2025 •

edited

Loading

Uh oh!

yuvicc left a comment

Uh oh!

achow101 Jun 27, 2025

Uh oh!

adamandrews1 Jun 27, 2025

Uh oh!

Uh oh!

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!

naiyoma May 4, 2025 • 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!

yuvicc left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

adamandrews1 Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Nov 15, 2024 •

edited

Loading

naiyoma May 4, 2025 •

edited

Loading