psbt: fix PSBTInput::Merge ignoring sighash_type field #35217

pull adrianrozadagarcia wants to merge 2 commits into bitcoin:master from adrianrozadagarcia:fix/psbt-merge-sighash-type changing 2 files +53 −1
  1. adrianrozadagarcia commented at 9:22 PM on May 5, 2026: none

    PSBTInput::Merge (used by CombinePSBTs / combinepsbt RPC) did not handle the sighash_type (PSBT_IN_SIGHASH) field at all:

    • When only the incoming PSBT had sighash_type set, the field was silently dropped from the merged result.
    • When both PSBTs had conflicting sighash_type values, Merge returned true instead of false, violating BIP 174 combiner rule: 'If two PSBTs contain the same key and different values, the Combiner MUST reject the combined PSBT.'

    Fix by copying the field when the receiving PSBT lacks it, and returning false on conflict.

    Add six unit tests covering: both absent, copy from other, preserve existing, same value, and two conflict cases.

  2. psbt: fix PSBTInput::Merge ignoring sighash_type field
    PSBTInput::Merge (used by CombinePSBTs / combinepsbt RPC) did not
    handle the sighash_type (PSBT_IN_SIGHASH) field at all:
    
    - When only the incoming PSBT had sighash_type set, the field was
      silently dropped from the merged result.
    - When both PSBTs had conflicting sighash_type values, Merge returned
      true instead of false, violating BIP 174 combiner rule: 'If two
      PSBTs contain the same key and different values, the Combiner MUST
      reject the combined PSBT.'
    
    Fix by copying the field when the receiving PSBT lacks it, and
    returning false on conflict.
    
    Add six unit tests covering: both absent, copy from other, preserve
    existing, same value, and two conflict cases.
    aad99b865e
  3. DrahtBot added the label PSBT on May 5, 2026
  4. DrahtBot commented at 9:22 PM on May 5, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35217.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. achow101 commented at 7:09 AM on May 6, 2026: member

    We prefer to test PSBT things with functional tests rather than unit tests.

  6. psbt: move sighash_type merge tests to functional test
    Per reviewer request, replace the unit tests for PSBTInput::Merge
    sighash_type handling with a functional test in rpc_psbt.py that
    exercises the same cases through the combinepsbt RPC.
    8dc85dbfbb
  7. adrianrozadagarcia commented at 10:03 AM on May 6, 2026: none

    We prefer to test PSBT things with functional tests rather than unit tests.

    I moved it to a funcional test in test/funcional/rpc_psbt.py (test_combine_sighash_type)

  8. achow101 commented at 10:25 AM on May 7, 2026: member

    Thanks, but LLM output is not accepted if the author can not properly explain, test or could have written the change themselves.

    The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on.

    Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR.

    I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output.

    Please reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested.

    I'll close this for now.

  9. achow101 closed this on May 7, 2026


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: 2026-05-07 15:12 UTC

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