rpc: combinerawtransaction now rejects unmergeable transactions #31298

pull adamandrews1 wants to merge 1 commits into bitcoin:master from adamandrews1:combinerawtransaction-check changing 2 files +35 −4
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me