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

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.

85c216871a

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

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: