rpc: combinerawtransaction now rejects unmergeable transactions #31298

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

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

   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.

3. DrahtBot added the label RPC/REST/ZMQ on Nov 15, 2024
4. adamandrews1 force-pushed on Nov 15, 2024
5. DrahtBot added the label CI failed on Nov 15, 2024
6. 
   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.

7. adamandrews1 marked this as a draft on Nov 15, 2024
8. adamandrews1 force-pushed on Nov 15, 2024
9. adamandrews1 force-pushed on Nov 15, 2024
10. adamandrews1 force-pushed on Nov 20, 2024
11. adamandrews1 marked this as ready for review on Nov 20, 2024
12. 
    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
13. DrahtBot removed the label CI failed on Nov 20, 2024
14. 
    1440000bytes commented at 9:31 pm on November 20, 2024: none

    Can this use the same approach and error message as combinepsbt?
15. 
    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.

16. 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
17. adamandrews1 force-pushed on Nov 20, 2024
18. 
    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,

19. 1440000bytes approved
20. 
    1440000bytes commented at 7:52 am on November 21, 2024: none

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

Contributors
adamandrews1 DrahtBot 1440000bytes

Labels
RPC/REST/ZMQ