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

pull ludete wants to merge 1 commits into bitcoin:master from ludete:r0-fit changing 1 files +2 −2
  1. 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

  2. fix use int32_t instead of int type for risczero compile with (-march=rv32i, -mabi=ilp32) bc52cda1f3
  3. 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.

  4. 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
  5. 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 ....

  6. 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
  7. DrahtBot added the label Consensus on Sep 3, 2024
  8. 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?

  9. TheCharlatan approved
  10. TheCharlatan commented at 3:57 pm on September 5, 2024: contributor
    ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017
  11. 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.

  12. maflcko added the label DrahtBot Guix build requested on Sep 19, 2024
  13. maflcko removed the label DrahtBot Guix build requested on Sep 19, 2024
  14. 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.

  15. achow101 merged this on Sep 20, 2024
  16. achow101 closed this on Sep 20, 2024

  17. TheCharlatan referenced this in commit 8bb47d4c2c on Nov 2, 2024
  18. ludete deleted the branch on Nov 19, 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-11-23 06:12 UTC

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