rpc: combinerawtransaction now rejects unmergeable transactions

[adamandrews1]

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]

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 <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

[DrahtBot]

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

It seems like, with https://github.com/bitcoin/bitcoin/pull/31249, I should migrate these test changes as well. WIP

[1440000bytes]

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

[adamandrews1]

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]

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

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

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

[zaidmstrr]

Concept ACK

[achow101]

Are you still working on this?

[adamandrews1]

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

[mossein]

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]

Are you still working on this?

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

[adamandrews1]

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

[adamandrews1]

Overall looking quite good.

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

https://github.com/bitcoin/bitcoin/blob/56ce78d5f62c9eb9353d33eedd15495c1205465f/test/functional/rpc_createmultisig.py#L48-L51

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.