diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index c783f32fb7..3797e5881d 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -33,6 +33,10 @@ def get_cmake_args(build_type: str, extra_args: str) -> str: # Every config must declare 'minimal'. Minimal configs form the reduced matrix # built for pull requests by default; the full matrix adds the rest. Packaging # configs declare it too, but packaging is gated in the workflow, not by it. +# +# Configs may also opt into 'benchmark' to smoke-run the benchmarks. Note that +# the flag applies to every entry a config expands into, so only set it on +# configs that expand to a single combination. @dataclasses.dataclass @@ -43,6 +47,7 @@ class LinuxConfig: build_type: list[str] arch: list[str] minimal: bool + benchmark: bool = False # if true, smoke-run the benchmarks after testing sanitizers: list[str] = dataclasses.field(default_factory=list) suffix: str = "" extra_cmake_args: str = "" @@ -81,6 +86,7 @@ class PlatformConfig: build_type: list[str] minimal: bool build_only: bool = False # if true, skip tests (e.g. macos/Windows Debug) + benchmark: bool = False # if true, smoke-run the benchmarks after testing extra_cmake_args: str = "" def __post_init__(self) -> None: @@ -125,6 +131,7 @@ class MatrixEntry: cmake_args: str cmake_target: str build_only: bool + benchmark: bool build_type: str architecture: Architecture sanitizers: str @@ -193,6 +200,7 @@ def expand_linux_matrix(linux: LinuxFile, minimal: bool) -> list[MatrixEntry]: cmake_args=get_cmake_args(build_type, cfg.extra_cmake_args), cmake_target="all", build_only=False, + benchmark=cfg.benchmark, build_type=build_type, architecture=arch_info, sanitizers=sanitizer, @@ -245,6 +253,7 @@ def expand_platform_matrix(pf: PlatformFile, minimal: bool) -> list[MatrixEntry] cmake_args=get_cmake_args(build_type, cfg.extra_cmake_args), cmake_target="install" if is_windows else "all", build_only=cfg.build_only, + benchmark=cfg.benchmark, build_type=build_type, architecture=Architecture(platform=pf.platform, runner=pf.runner), sanitizers="", diff --git a/.github/scripts/strategy-matrix/linux.json b/.github/scripts/strategy-matrix/linux.json index 9510212344..159c76b6c2 100644 --- a/.github/scripts/strategy-matrix/linux.json +++ b/.github/scripts/strategy-matrix/linux.json @@ -14,7 +14,8 @@ "compiler": ["clang"], "build_type": ["Release"], "arch": ["amd64"], - "minimal": true + "minimal": true, + "benchmark": true }, { diff --git a/.github/workflows/on-pr.yml b/.github/workflows/on-pr.yml index 1cd97305da..0a4e4b1f49 100644 --- a/.github/workflows/on-pr.yml +++ b/.github/workflows/on-pr.yml @@ -82,6 +82,7 @@ jobs: .github/scripts/strategy-matrix/** .github/workflows/reusable-build-test-config.yml .github/workflows/reusable-build-test.yml + .github/workflows/reusable-check-autogen.yml .github/workflows/reusable-clang-tidy.yml .github/workflows/reusable-package.yml .github/workflows/reusable-strategy-matrix.yml @@ -126,6 +127,11 @@ jobs: outputs: go: ${{ steps.go.outputs.go == 'true' }} + check-autogen: + needs: should-run + if: ${{ needs.should-run.outputs.go == 'true' }} + uses: ./.github/workflows/reusable-check-autogen.yml + check-levelization: needs: should-run if: ${{ needs.should-run.outputs.go == 'true' }} @@ -200,6 +206,7 @@ jobs: passed: if: failure() || cancelled() needs: + - check-autogen - check-levelization - check-rename - clang-tidy diff --git a/.github/workflows/on-trigger.yml b/.github/workflows/on-trigger.yml index b8899cec72..73f918d528 100644 --- a/.github/workflows/on-trigger.yml +++ b/.github/workflows/on-trigger.yml @@ -20,6 +20,7 @@ on: - ".github/scripts/strategy-matrix/**" - ".github/workflows/reusable-build-test-config.yml" - ".github/workflows/reusable-build-test.yml" + - ".github/workflows/reusable-check-autogen.yml" - ".github/workflows/reusable-clang-tidy.yml" - ".github/workflows/reusable-package.yml" - ".github/workflows/reusable-strategy-matrix.yml" @@ -67,6 +68,9 @@ defaults: shell: bash jobs: + check-autogen: + uses: ./.github/workflows/reusable-check-autogen.yml + clang-tidy: uses: ./.github/workflows/reusable-clang-tidy.yml permissions: diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 3023f70cdf..548fde8b5e 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -3,6 +3,12 @@ name: Build and test configuration on: workflow_call: inputs: + benchmark: + description: "Whether to smoke-run the benchmarks after testing." + required: false + type: boolean + default: false + build_only: description: 'Whether to only build or to build and test the code ("true", "false").' required: true @@ -170,9 +176,9 @@ jobs: .. # Export the sanitizer options before any instrumented binary runs. The - # protocol code-gen and build steps below invoke instrumented dependency - # tools (protoc, grpc), so setting UBSAN_OPTIONS here lets the UBSan - # suppression list silence their diagnostics too, not just at test time. + # build step below invokes instrumented dependency tools (protoc, grpc), + # so setting UBSAN_OPTIONS here lets the UBSan suppression list silence + # their diagnostics too, not just at test time. # GITHUB_WORKSPACE (not the github.workspace context) is used so the path # resolves correctly inside the container job. - name: Set sanitizer options @@ -190,32 +196,6 @@ jobs: echo "UBSAN_OPTIONS=include=${SUPP}/runtime-ubsan-options.txt:suppressions=${SUPP}/ubsan.supp" >>${GITHUB_ENV} echo "LSAN_OPTIONS=include=${SUPP}/runtime-lsan-options.txt:suppressions=${SUPP}/lsan.supp" >>${GITHUB_ENV} - - name: Check protocol autogen files are up-to-date - working-directory: ${{ env.BUILD_DIR }} - env: - MESSAGE: | - - The generated protocol wrapper classes are out of date. - - This typically happens when the macro files or generator scripts - have changed but the generated files were not regenerated. - - To fix this: - 1. Run: cmake --build . --target setup_code_gen - 2. Run: cmake --build . --target code_gen - 3. Commit and push the regenerated files - run: | - set -e - cmake --build . --target setup_code_gen - cmake --build . --target code_gen - DIFF=$(git -C .. status --porcelain -- include/xrpl/protocol_autogen src/tests/libxrpl/protocol_autogen) - if [ -n "${DIFF}" ]; then - echo "::error::Generated protocol files are out of date" - git -C .. diff -- include/xrpl/protocol_autogen src/tests/libxrpl/protocol_autogen - echo "${MESSAGE}" - exit 1 - fi - - name: Build the binary working-directory: ${{ env.BUILD_DIR }} env: @@ -328,11 +308,14 @@ jobs: # Smoke-run every benchmark module with a single repetition to confirm the # benchmarks still build and execute. This is a correctness check, not a - # performance measurement, so it is skipped for instrumented builds - # (sanitizers/coverage/voidstar), where it would be slow and meaningless, - # and on Windows, where the `install` target does not build them. + # performance measurement, so there is nothing to gain from repeating it + # across configurations: it is opted into by a single config in the + # strategy matrix (see the 'benchmark' flag in the JSON files), which + # keeps it off instrumented builds (sanitizers/coverage/voidstar), where + # it would be slow and meaningless, off Debug builds, where it is much + # slower, and off Windows, where the `install` target does not build them. - name: Run the benchmarks - if: ${{ !inputs.build_only && runner.os != 'Windows' && env.SANITIZERS_ENABLED == 'false' && env.COVERAGE_ENABLED != 'true' && env.VOIDSTAR_ENABLED != 'true' }} + if: ${{ inputs.benchmark }} working-directory: ${{ env.BUILD_DIR }} run: | rc=0 diff --git a/.github/workflows/reusable-build-test.yml b/.github/workflows/reusable-build-test.yml index 4b64c53521..5368274a16 100644 --- a/.github/workflows/reusable-build-test.yml +++ b/.github/workflows/reusable-build-test.yml @@ -40,6 +40,7 @@ jobs: fail-fast: ${{ github.event_name == 'merge_group' }} matrix: ${{ fromJson(needs.generate-matrix.outputs.matrix) }} with: + benchmark: ${{ matrix.benchmark }} build_only: ${{ matrix.build_only }} build_type: ${{ matrix.build_type }} ccache_enabled: ${{ inputs.ccache_enabled }} diff --git a/.github/workflows/reusable-check-autogen.yml b/.github/workflows/reusable-check-autogen.yml new file mode 100644 index 0000000000..bb77ea85a9 --- /dev/null +++ b/.github/workflows/reusable-check-autogen.yml @@ -0,0 +1,76 @@ +# This workflow checks that the generated protocol wrapper classes are +# up-to-date with the macro files and generator scripts they are produced from, +# see more info in include/xrpl/protocol_autogen/README.md. +name: Check autogen + +# This workflow can only be triggered by other workflows. +on: workflow_call + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }}-autogen + cancel-in-progress: true + +defaults: + run: + shell: bash + +env: + BUILD_DIR: build/codegen + +jobs: + autogen: + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.13" + + # Code generation is pure Python, so the standalone project below offers + # the same targets as the main build without needing its dependencies or + # a compiler, which keeps this job down to a few seconds. + - name: Configure CMake + run: cmake -S cmake/codegen -B "${BUILD_DIR}" + + - name: Install code generation dependencies + run: cmake --build "${BUILD_DIR}" --target setup_code_gen + + - name: Generate code + run: cmake --build "${BUILD_DIR}" --target code_gen + + - name: Check for differences + env: + MESSAGE: | + + The generated protocol wrapper classes are out of date. + + This typically happens when the macro files or generator scripts + have changed but the generated files were not regenerated. + + Run the following from the repository root, then commit and push + the regenerated files. This needs neither the dependencies nor a + compiler. See include/xrpl/protocol_autogen/README.md for more info. + + cmake -S cmake/codegen -B build/codegen + cmake --build build/codegen --target setup_code_gen + cmake --build build/codegen --target code_gen + + In an already configured build directory, the 'setup_code_gen' and + 'code_gen' targets do the same thing. + run: | + # Record untracked files in the index without staging their contents, + # so that classes generated for a newly added transaction or ledger + # entry type show up in the diff below rather than silently as an + # empty one. + git add --intent-to-add . + DIFF=$(git status --porcelain) + if [ -n "${DIFF}" ]; then + # Print the differences to give the contributor a hint about what to + # expect when running code generation on their own machine. + git diff + echo "${MESSAGE}" + exit 1 + fi diff --git a/BUILD.md b/BUILD.md index edc52fe3b7..238c10e17c 100644 --- a/BUILD.md +++ b/BUILD.md @@ -247,7 +247,17 @@ cmake --build . --target setup_code_gen # create venv and install dependencies cmake --build . --target code_gen # regenerate code ``` -The regenerated files should be committed alongside your changes. +The same targets are also available as a standalone project, which does not +need the dependencies to be configured first: + +``` +cmake -S cmake/codegen -B build/codegen +cmake --build build/codegen --target setup_code_gen +cmake --build build/codegen --target code_gen +``` + +The regenerated files should be committed alongside your changes. CI verifies +that they are up-to-date. ## Coverage report diff --git a/cmake/XrplProtocolAutogen.cmake b/cmake/XrplProtocolAutogen.cmake index dd9ef6a9a4..33af560113 100644 --- a/cmake/XrplProtocolAutogen.cmake +++ b/cmake/XrplProtocolAutogen.cmake @@ -2,21 +2,22 @@ Protocol Autogen - Code generation for protocol wrapper classes #]===================================================================] +# The repository root, derived from the location of this file rather than from +# the including project, so that the targets below can also be offered on their +# own by cmake/codegen/CMakeLists.txt. +get_filename_component(XRPL_ROOT "${CMAKE_CURRENT_LIST_DIR}/.." ABSOLUTE) + set(CODEGEN_VENV_DIR - "${CMAKE_CURRENT_SOURCE_DIR}/.venv" + "${XRPL_ROOT}/.venv" CACHE PATH "Path to a Python virtual environment for code generation. A venv will be created here by setup_code_gen and used to run generation scripts." ) # Directory paths -set(MACRO_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include/xrpl/protocol/detail") -set(AUTOGEN_HEADER_DIR - "${CMAKE_CURRENT_SOURCE_DIR}/include/xrpl/protocol_autogen" -) -set(AUTOGEN_TEST_DIR - "${CMAKE_CURRENT_SOURCE_DIR}/src/tests/libxrpl/protocol_autogen" -) -set(SCRIPTS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/cmake/scripts/codegen") +set(MACRO_DIR "${XRPL_ROOT}/include/xrpl/protocol/detail") +set(AUTOGEN_HEADER_DIR "${XRPL_ROOT}/include/xrpl/protocol_autogen") +set(AUTOGEN_TEST_DIR "${XRPL_ROOT}/src/tests/libxrpl/protocol_autogen") +set(SCRIPTS_DIR "${XRPL_ROOT}/cmake/scripts/codegen") # Input macro files set(TRANSACTIONS_MACRO "${MACRO_DIR}/transactions.macro") @@ -114,14 +115,14 @@ if(CODEGEN_VENV_DIR) setup_code_gen COMMAND ${Python3_EXECUTABLE} -m venv "${CODEGEN_VENV_DIR}" COMMAND ${CODEGEN_PYTHON} -m pip install -r "${REQUIREMENTS_FILE}" - WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" + WORKING_DIRECTORY "${XRPL_ROOT}" COMMENT "Creating venv and installing code generation dependencies..." ) else() add_custom_target( setup_code_gen COMMAND ${Python3_EXECUTABLE} -m pip install -r "${REQUIREMENTS_FILE}" - WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" + WORKING_DIRECTORY "${XRPL_ROOT}" COMMENT "Installing code generation dependencies..." ) endif() @@ -139,8 +140,8 @@ add_custom_target( -DSFIELDS_MACRO=${SFIELDS_MACRO} -DAUTOGEN_HEADER_DIR=${AUTOGEN_HEADER_DIR} -DAUTOGEN_TEST_DIR=${AUTOGEN_TEST_DIR} -P - "${CMAKE_CURRENT_SOURCE_DIR}/cmake/XrplProtocolAutogenRun.cmake" - WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" + "${CMAKE_CURRENT_LIST_DIR}/XrplProtocolAutogenRun.cmake" + WORKING_DIRECTORY "${XRPL_ROOT}" COMMENT "Running protocol code generation..." SOURCES ${ALL_INPUT_FILES} ) diff --git a/cmake/codegen/CMakeLists.txt b/cmake/codegen/CMakeLists.txt new file mode 100644 index 0000000000..f697d67dd8 --- /dev/null +++ b/cmake/codegen/CMakeLists.txt @@ -0,0 +1,21 @@ +#[===================================================================[ + Protocol Autogen - Standalone project + + Exposes the 'setup_code_gen' and 'code_gen' targets on their own, without + configuring the rest of xrpl. Code generation is pure Python, so this needs + neither the dependencies nor a compiler, which makes it usable in CI and by + contributors who only want to regenerate the protocol wrapper classes: + + cmake -S cmake/codegen -B build/codegen + cmake --build build/codegen --target setup_code_gen + cmake --build build/codegen --target code_gen + + The targets are identical to the ones offered by the top-level build, since + both come from cmake/XrplProtocolAutogen.cmake. +#]===================================================================] + +cmake_minimum_required(VERSION 3.16) + +project(xrpl_codegen LANGUAGES NONE) + +include("${CMAKE_CURRENT_LIST_DIR}/../XrplProtocolAutogen.cmake") diff --git a/include/xrpl/protocol_autogen/README.md b/include/xrpl/protocol_autogen/README.md index 608ffed085..ed649a05fc 100644 --- a/include/xrpl/protocol_autogen/README.md +++ b/include/xrpl/protocol_autogen/README.md @@ -23,6 +23,16 @@ By default, `CODEGEN_VENV_DIR` points to `.venv` in the project root. The `setup_code_gen` target creates a venv there and installs the required packages. The `code_gen` target then uses the venv's Python interpreter to run generation. +Generation is pure Python, so the same targets are also available as a +standalone project that needs neither the dependencies nor a compiler. This is +what CI uses, and it is handy if you only want to regenerate these files: + +```bash +cmake -S cmake/codegen -B build/codegen +cmake --build build/codegen --target setup_code_gen +cmake --build build/codegen --target code_gen +``` + ### Python Dependencies The code generation requires the following Python packages (installed by `setup_code_gen`): diff --git a/src/libxrpl/protocol/STIssue.cpp b/src/libxrpl/protocol/STIssue.cpp index 10403d2c50..ba32c1214c 100644 --- a/src/libxrpl/protocol/STIssue.cpp +++ b/src/libxrpl/protocol/STIssue.cpp @@ -11,6 +11,8 @@ #include #include +#include + #include #include #include @@ -45,6 +47,10 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name} { MPTID mptID; std::uint32_t sequence = sit.get32(); + // MPTID stores the sequence in canonical big-endian bytes. STIssue + // ledger bytes are the legacy LE-host encoding, so convert the + // native get32() value to LE bytes before copying into the MPTID. + sequence = boost::endian::native_to_little(sequence); static_assert(MPTID::size() == sizeof(sequence) + sizeof(currencyOrAccount)); memcpy(mptID.data(), &sequence, sizeof(sequence)); memcpy( @@ -100,6 +106,10 @@ STIssue::add(Serializer& s) const s.addBitString(noAccount()); std::uint32_t sequence = 0; memcpy(&sequence, issue.getMptID().data(), sizeof(sequence)); + // The MPTID bytes are canonical big-endian. Interpret those bytes + // as the legacy LE-host value so add32() writes the preserved + // STIssue wire bytes on every host endian. + sequence = boost::endian::little_to_native(sequence); s.add32(sequence); }); } diff --git a/src/test/protocol/STIssue_test.cpp b/src/test/protocol/STIssue_test.cpp index b7cc944e6b..41517b38f3 100644 --- a/src/test/protocol/STIssue_test.cpp +++ b/src/test/protocol/STIssue_test.cpp @@ -7,17 +7,22 @@ #include #include +#include #include #include #include #include +#include #include +#include #include #include #include #include #include +#include +#include #include namespace xrpl::test { @@ -273,6 +278,54 @@ public: } } + void + testMPTSerialization() + { + testcase("MPT serialization"); + using namespace jtx; + Account const alice{"alice"}; + + // 0x01020304 pins canonical MPTID bytes 01 02 03 04 and + // preserved STIssue wire bytes 04 03 02 01 on BE and LE. + auto const sequences = std::to_array({0x00000001, 0x01020304, 0xa1b2c3d4}); + + for (auto const vector : sequences) + { + MPTID const mptID = makeMptID(vector, alice); + MPTIssue const issue{mptID}; + STIssue const stIssue(sfAsset, Asset{issue}); + + Serializer actual; + stIssue.add(actual); + + // STIssue preserves the existing little-endian validator ledger bytes. + Serializer expected; + expected.addBitString(alice.id()); + expected.addBitString(noAccount()); + { + std::array const bytes{ + static_cast(vector), + static_cast(vector >> 8), + static_cast(vector >> 16), + static_cast(vector >> 24)}; + expected.addRaw(bytes.data(), bytes.size()); + } + + BEAST_EXPECTS(strHex(actual) == strHex(expected), strHex(actual)); + + // Decoding the preserved wire format must recover the canonical MPTID. + SerialIter iter(expected.slice()); + STIssue const decoded(iter, sfAsset); + BEAST_EXPECT(decoded.holds()); + BEAST_EXPECT(decoded.value().get().getMptID() == mptID); + + // A decoded ledger value must serialize back to the same bytes. + Serializer roundTrip; + decoded.add(roundTrip); + BEAST_EXPECTS(strHex(roundTrip) == strHex(expected), strHex(roundTrip)); + } + } + void run() override { @@ -283,6 +336,7 @@ public: testNoAccountIssuer(); testXrpAccountIssuerRpc(); testXrpAccountIssuer(); + testMPTSerialization(); } };