interpreter: Use the same type for SignatureHash in the definition

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.

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

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.

DrahtBot added the label Consensus on Nov 25, 2024

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?

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.

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.

theuni approved

theuni commented at 5:57 pm on November 25, 2024: member

Obvious fix ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f.

Linker did its job here :)

l0rinc commented at 6:23 pm on November 25, 2024: contributor

ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f

BrandonOdiwuor approved

BrandonOdiwuor commented at 7:01 pm on November 25, 2024: contributor

Code Review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f

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==

achow101 commented at 6:32 pm on November 26, 2024: member

ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f

achow101 merged this on Nov 26, 2024

achow101 closed this on Nov 26, 2024

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

Code Coverage & Benchmarks

Reviews