interpreter: Use the same type for SignatureHash in the definition #31365

pull TheCharlatan wants to merge 1 commits into bitcoin:master from TheCharlatan:consensus_sighash_int_type changing 1 files +1 −1
  1. TheCharlatan commented at 2:42 pm on November 25, 2024: contributor

    This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.

    The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with:

    0/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
    1/home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
    

    With this patch it is possible to link against the static consensus library and produce a fully static executable.

  2. interpreter: Use the same type for SignatureHash in the definition
    This was missed during the original PR switching the nHashType argument
    to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.
    
    The problem was discovered after running into a linker error when
    attempting to link this code as a static library using the header as a
    declaration with a riscv32 bare metal toolchain. The compiler would
    error with:
    
    /opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
    /home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
    c288c790cd
  3. DrahtBot commented at 2:42 pm on November 25, 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/31365.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theuni, l0rinc, BrandonOdiwuor, maflcko, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. DrahtBot added the label Consensus on Nov 25, 2024
  5. hebasto commented at 2:50 pm on November 25, 2024: member
    Does it make sense to make the sighash types/flags enum explicitly typed as well?
  6. in src/script/interpreter.h:243 in c288c790cd
    239@@ -240,7 +240,7 @@ extern const HashWriter HASHER_TAPLEAF;    //!< Hasher with tag "TapLeaf" pre-fe
    240 extern const HashWriter HASHER_TAPBRANCH;  //!< Hasher with tag "TapBranch" pre-fed to it.
    241 
    242 template <class T>
    243-uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr);
    244+uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int32_t nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr);
    


    l0rinc commented at 2:56 pm on November 25, 2024:

    Would it make sense to do this for the remaining nHashType types, e.g. in CTransactionSignatureSerializer, ParseSighashString, SignTransaction, MutableTransactionSignatureCreator, SignatureHashOld, SignSignature, etc?

    And how can we avoid this error next time, can we enable (a clang-tidy?) check that compares the cpp and header types?


    TheCharlatan commented at 5:51 pm on November 25, 2024:

    Would it make sense to do this for the remaining nHashType types, e.g. in CTransactionSignatureSerializer, ParseSighashString, SignTransaction, MutableTransactionSignatureCreator, SignatureHashOld, SignSignature, etc?

    I don’t think it makes sense in this context, but might still be good to do this for consistency. I’d rather change the absolute minimum possible in these files though.

    And how can we avoid this error next time, can we enable (a clang-tidy?) check that compares the cpp and header types?

    Maybe we can add a clang-tidy plugin that checks that the names of the type of each of the parameters in the declaration and the definition are the same, but not sure how useful this actually is. Also a compiler, or rather linker, did actually catch this. Might still be a good exercise to write a plugin for such a check.

  7. TheCharlatan commented at 5:39 pm on November 25, 2024: contributor

    Does it make sense to make the sighash types/flags enum explicitly typed as well?

    At that point I feel like we’d have to do it for everything. If I understood the actual problem correctly, it is that we cannot serialize an int with our current serialization functions on that platform, so I’m not sure if that would be useful in the first place.

  8. theuni approved
  9. theuni commented at 5:57 pm on November 25, 2024: member

    Obvious fix ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f.

    Linker did its job here :)

  10. l0rinc commented at 6:23 pm on November 25, 2024: contributor
    ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
  11. BrandonOdiwuor approved
  12. BrandonOdiwuor commented at 7:01 pm on November 25, 2024: contributor
    Code Review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
  13. maflcko commented at 9:49 am on November 26, 2024: member

    review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f 🐺

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f 🐺
    3GIxlo6uGCjWopQeUkzfL/QhPCSbb2cEMtVJLC+2x8LhCLZiY/vzYSpuzMCK9wUmeu5mQfwggFbMkxXkyLqhODQ==
    
  14. achow101 commented at 6:32 pm on November 26, 2024: member
    ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
  15. achow101 merged this on Nov 26, 2024
  16. achow101 closed this on Nov 26, 2024


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