interpreter: use int32_t instead of int type for risczero compile

ludete commented at 2:40 am on September 3, 2024: none

When compile bitcoin by the toolchain(riscv32-unknown-elf-g++) from risc0 , the compiler argument is -march=rv32i, -mabi=ilp32, which will get the error which due to not serialize the value of type int .

0blockbody-guest:   cargo:warning=In file included from depend/bitcoin/src/hash.h:14,
1blockbody-guest:   cargo:warning=                 from depend/bitcoin/src/script/interpreter.h:9,
2blockbody-guest:   cargo:warning=                 from depend/bitcoin/src/script/interpreter.cpp:6:
3blockbody-guest:   cargo:warning=depend/bitcoin/src/serialize.h: In instantiation of 'void Serialize(Stream&, const T&) [with Stream = HashWriter; T = int]':
4blockbody-guest:   cargo:warning=depend/bitcoin/src/hash.h:144:20:   required from 'HashWriter& HashWriter::operator<<(const T&) [with T = int]'
5blockbody-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]'
6blockbody-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]'
7blockbody-guest:   cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1785:16:   required from here
8blockbody-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'
9blockbody-guest:   cargo:warning=  776 |     a.Serialize(os);

Reason

“The toolchain from RISC Zero defines int and int32_t as different types, although they have the same width. This means that src/compat/assumptions.h compiles fine; however, the templated serialization code cannot accept values of type int. Fix the compilation on RISC Zero by serializing int32_t instead of int values.

This patch will explicitly use the int32_t type instead of int to avoid errors when compiling with the risc0 toolchain. Additionally, this patch will not change any behavior on platforms where compilation was previously successful.

Fixes #30747

fix use int32_t instead of int type for risczero compile with (-march=rv32i, -mabi=ilp32) bc52cda1f3

DrahtBot commented at 2:40 am on September 3, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, 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 renamed this:
~~fix use int32_t instead of int type for risczero compile~~
fix use int32_t instead of int type for risczero compile
on Sep 3, 2024

maflcko commented at 6:08 am on September 3, 2024: member

Can you explain why this is correct and why it is needed in the pull request description? Otherwise, current and future reviewers of the change will have a hard time following it.

Maybe something along the lines:

Some platforms may define int and int32_t to be different types of the same width. This means that src/compat/assumptions.h compiles fine, however the templated serialization code can not accept values of type int.

Fix compilation on those platforms by serializing int32_t instead of int values.

This patch does not change behavior on platforms where a compilation was previously possible.

Also, instead of Issue ... you can use Fixes ....

fanquake renamed this:
~~fix use int32_t instead of int type for risczero compile~~
interpreter: use int32_t instead of int type for risczero compile
on Sep 3, 2024

DrahtBot added the label Consensus on Sep 3, 2024

TheCharlatan commented at 9:16 am on September 3, 2024: contributor

Concept ACK

This can be tested by cloning https://github.com/riscv-collab/riscv-gnu-toolchain, then:

0./configure --prefix=/opt/riscv-ilp32 --with-arch=rv32gc --with-abi=ilp32
1make

And then from the root directory:

0/opt/riscv-ilp32/bin/riscv32-unknown-elf-g++ -std=c++20 -march=rv32i -mabi=ilp32 -I /path/to/bitcoin/src -c src/script/interpreter.cpp -o script.o

Without this patch:

0src/script/interpreter.cpp:1306:24: error: call of overloaded 'Serialize(HashWriter&, int)' is ambiguous
1 1306 |             ::Serialize(s, int{0});
2      |             ~~~~~~~~~~~^~~~~~~~~~~

With this patch: Compiles successfully

If this patch is accepted, should this be backported to the 27 and 26 branches so libconsensus actually gets the patch while we figure out how to eventually support this platform through the kernel library?

TheCharlatan approved

TheCharlatan commented at 3:57 pm on September 5, 2024: contributor

ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017

maflcko commented at 8:24 am on September 6, 2024: member

review-only ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017

Changing int to int32_t in Bitcoin Core should always be fine, because they are assumed to be of the same bit-width and bit-representation. Generally, it is even preferred to use int32_t, especially in serialization code, because it documents the width explicitly.

However, I haven’t tested this or reviewed that the patch is complete. It would be nice if there was an easy way to highlight all places in the codebase where distinct types for int and int32_t will lead to a compile error. However, this can be done in a follow-up.

maflcko added the label DrahtBot Guix build requested on Sep 19, 2024

maflcko removed the label DrahtBot Guix build requested on Sep 19, 2024

achow101 commented at 5:37 pm on September 20, 2024: member

ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017

Did not test compilation, but int and int32_t are synonymous for us.

achow101 merged this on Sep 20, 2024

achow101 closed this on Sep 20, 2024

TheCharlatan referenced this in commit 8bb47d4c2c on Nov 2, 2024

ludete deleted the branch on Nov 19, 2024

interpreter: use int32_t instead of int type for risczero compile #30794

Reason

Code Coverage

Reviews