Risczero Fit

[ludete]

compile:

CXXFLAGS="-DRISCZERO" CXX=/opt/riscv-new/bin/riscv32-unknown-elf-g++ cargo build -p host

[apoelstra]

Neat!

Unfortunately I don't see any way to accept this. Modifying the upstream serialize.h to differ from upstream, in a semantically meaningful way, is not something we can accept. And upstream has sunsetted the library so they won't accept changes either.

Why do you need these serialization impls?

[ludete]

@apoelstra

The compiler(riscv32-unknown-elf-g++) treats them(int, int32_t) as different types in template instantiation. When you compile the bitcoin source code will get the error

blockbody-guest:   cargo:warning=In file included from depend/bitcoin/src/hash.h:14,
blockbody-guest:   cargo:warning=                 from depend/bitcoin/src/script/interpreter.h:9,
blockbody-guest:   cargo:warning=                 from depend/bitcoin/src/script/interpreter.cpp:6:
blockbody-guest:   cargo:warning=depend/bitcoin/src/serialize.h: In instantiation of 'void Serialize(Stream&, const T&) [with Stream = HashWriter; T = int]':
blockbody-guest:   cargo:warning=depend/bitcoin/src/hash.h:144:20:   required from 'HashWriter& HashWriter::operator<<(const T&) [with T = int]'
blockbody-guest:   cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1613:12:   required from 'uint256 SignatureHash(const CScript&, const T&, unsigned int, int, const CAmount&, SigVersion, const PrecomputedTransactionData*) [with T = CTransaction; CAmount = long long int]'
blockbody-guest:   cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1664:36:   required from 'bool GenericTransactionSignatureChecker<T>::CheckECDSASignature(const std::vector<unsigned char>&, const std::vector<unsigned char>&, const CScript&, SigVersion) const [with T = CTransaction]'
blockbody-guest:   cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1785:16:   required from here
blockbody-guest:   cargo:warning=depend/bitcoin/src/serialize.h:776:7: error: request for member 'Serialize' in 'a', which is of non-class type 'const int'
blockbody-guest:   cargo:warning=  776 |     a.Serialize(os);

This error is caused by the logic in the SignatureHash function within src/script/interpreter.cpp. The function serializes the data to obtain a hash value, but it fails to find a serialization method for the int type.

// Sighash type
ss << nHashType;

This behavior is particularly strict in environments like RISC-V, where the ABI (Application Binary Interface) might enforce more rigid type distinctions, leading to errors during compilation.

[apoelstra]

Makes sense. I think the correct fix though is to use int32_t everywhere that a 32-bit integer is expected rather than adding serialization logic for int (which isn't guaranteed to work, e.g. if int is 16 bits wide) (and it's nice that the existing code causes compilation failures when int size assumptions are wrong, instead of silently doing integer promotions).

If this isn't done upstream already, could you also PR there?

I'd be willing to carry an extra patch here which did that, since it'd be a clear bugfix and the new code would be clearly correct. Though it would require some extra changes to our vendoring scripts and could be a long while before it got in since this project is in near-maintenance-mode due to the upstream libbitcoinconsensus deprecation.

[ludete]

Thank you, I will submit a PR to upstream, and your proposed modification makes more sense.

However, I have a question: in the current version, when running on 32-bit and 64-bit platforms, why doesn't ss << nHashType cause serialization inconsistencies due to int being 32-bit or 64-bit?

[maflcko]

See my reply here: bitcoin/bitcoin#30747 (comment)

Specifically, it would be interesting if you compiled assumptions.h, as well as static_assert(std::is_same_v<int, int32_t>); on your system.

[apoelstra]

However, I have a question: in the current version, when running on 32-bit and 64-bit platforms, why doesn't ss << nHashType cause serialization inconsistencies due to int being 32-bit or 64-bit?

I suspect that there is an inconsistency but it's just not hit on any target platforms? I think when the 64-bit transition happened almost all C compilers (and presumably C++) kept their ints at 32 bits to avoid breaking the tons of code that assume this.

But the standard only says that int must be "at least 16 bits" so unless there is some template magic protecting them, I think this is a possible failure mode.

[ludete]

Has create a pr to bitcoin repo for the issue: bitcoin/bitcoin#30794.

Which will explicitly use int32_t instead of the int type.