From 82ed6fd6f3e49ea51782d8537ca810e749b9a958 Mon Sep 17 00:00:00 2001 From: TimothyBanks Date: Tue, 22 Sep 2026 18:18:53 -0400 Subject: [PATCH] fix: Address AI code review comments --- crates/README.md | 2 +- include/xrpl/ledger/helpers/EscrowHelpers.h | 15 ++- include/xrpl/protocol/PathAsset.h | 2 +- include/xrpl/tx/wasm/HostFunc.h | 120 +++++++++--------- include/xrpl/tx/wasm/HostFuncImpl.h | 4 +- src/benchmarks/libxrpl/wasm/README.md | 8 +- src/benchmarks/libxrpl/wasm/WasmBench.cpp | 2 +- .../tx/wasm/transactor/BytecodeRun.cpp | 26 +++- 8 files changed, 107 insertions(+), 72 deletions(-) diff --git a/crates/README.md b/crates/README.md index 19f2dcc044..a9114d12ca 100644 --- a/crates/README.md +++ b/crates/README.md @@ -6,7 +6,7 @@ bridged into C++ via `cxxbridge`/the `cxx` crate. The workspace is built unconditionally — `add_subdirectory(crates)` in the top-level `CMakeLists.txt` is not behind an option, and `xrpl_wasm_vm_ffi_cxxbridge` is a `PUBLIC` dependency of -`xrpl.libxrpl.ledger` (see `cmake/XrplCore.cmake`). The Rust toolchain pinned in +`xrpl.libxrpl.tx` (see `cmake/XrplCore.cmake`). The Rust toolchain pinned in [`rust-toolchain.toml`](../rust-toolchain.toml) is therefore required to build `libxrpl` at all; the Nix devshell provides it automatically. diff --git a/include/xrpl/ledger/helpers/EscrowHelpers.h b/include/xrpl/ledger/helpers/EscrowHelpers.h index 08b5956004..aeb24e3ac8 100644 --- a/include/xrpl/ledger/helpers/EscrowHelpers.h +++ b/include/xrpl/ledger/helpers/EscrowHelpers.h @@ -278,11 +278,20 @@ template static int32_t calculateAdditionalReserve(T const& finishFunction) { - if (!finishFunction) - return 1; // First 500 bytes included in the normal reserve // Each additional 500 bytes requires an additional reserve - return 1 + (finishFunction->size() / 500); + static auto constexpr kBytecodeReserveIncrement = 500; + + if (!finishFunction) + return 1; + + // Ceiling division answers 0 for an empty field, which would subtract less than + // the create added. + auto const size = finishFunction->size(); + if (size == 0) + return 1; + + return static_cast((size + kBytecodeReserveIncrement - 1) / kBytecodeReserveIncrement); } } // namespace xrpl diff --git a/include/xrpl/protocol/PathAsset.h b/include/xrpl/protocol/PathAsset.h index ebf6fb68a4..30940bbd14 100644 --- a/include/xrpl/protocol/PathAsset.h +++ b/include/xrpl/protocol/PathAsset.h @@ -79,7 +79,7 @@ PathAsset::holds() const } template -[[nodiscard]] [[nodiscard]] T const& +[[nodiscard]] T const& PathAsset::get() const { if (!holds()) diff --git a/include/xrpl/tx/wasm/HostFunc.h b/include/xrpl/tx/wasm/HostFunc.h index 83a6e50c45..5eca08c2b4 100644 --- a/include/xrpl/tx/wasm/HostFunc.h +++ b/include/xrpl/tx/wasm/HostFunc.h @@ -87,37 +87,37 @@ public: return true; } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getLedgerSqn() const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getParentLedgerTime() const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getParentLedgerHash() const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getBaseFee() const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected isAmendmentEnabled(uint256 const& amendmentId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected isAmendmentEnabled(std::string_view const& amendmentName) const { return std::unexpected(HostFunctionError::Unimplemented); @@ -129,73 +129,73 @@ public: return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getTxField(SField const& fname) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getCurrentLedgerObjField(SField const& fname) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getLedgerObjField(int32_t cacheIdx, SField const& fname) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getTxNestedField(FieldLocator const& locator) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getCurrentLedgerObjNestedField(FieldLocator const& locator) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getLedgerObjNestedField(int32_t cacheIdx, FieldLocator const& locator) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getTxArrayLen(SField const& fname) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getCurrentLedgerObjArrayLen(SField const& fname) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getLedgerObjArrayLen(int32_t cacheIdx, SField const& fname) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getTxNestedArrayLen(FieldLocator const& locator) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getCurrentLedgerObjNestedArrayLen(FieldLocator const& locator) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getLedgerObjNestedArrayLen(int32_t cacheIdx, FieldLocator const& locator) const { return std::unexpected(HostFunctionError::Unimplemented); @@ -207,184 +207,184 @@ public: return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected checkSignature(Slice const& message, Slice const& signature, Slice const& pubkey) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected computeSha512HalfHash(Slice const& data) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected accountKeylet(AccountID const& account) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected ammKeylet(Asset const& issue1, Asset const& issue2) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected checkKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected credentialKeylet(AccountID const& subject, AccountID const& issuer, Slice const& credentialType) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected didKeylet(AccountID const& account) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected delegateKeylet(AccountID const& account, AccountID const& authorize) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected depositPreauthKeylet(AccountID const& account, AccountID const& authorize) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected escrowKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected trustLineKeylet(AccountID const& account1, AccountID const& account2, Currency const& currency) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected mptokenIssuanceKeylet(AccountID const& issuer, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected mptokenKeylet(MPTID const& mptid, AccountID const& holder) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected nftokenOfferKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected offerKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected oracleKeylet(AccountID const& account, std::uint32_t docId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected paychannelKeylet(AccountID const& account, AccountID const& destination, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected permissionedDomainKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected signerListKeylet(AccountID const& account) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected ticketKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected vaultKeylet(AccountID const& account, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected sponsorshipKeylet(AccountID const& sponsor, AccountID const& sponsee) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected loanBrokerKeylet(AccountID const& owner, std::uint32_t seq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected loanKeylet(uint256 const& loanBrokerID, std::uint32_t loanSeq) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getNFT(AccountID const& account, uint256 const& nftId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getNFTIssuer(uint256 const& nftId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getNFTTaxon(uint256 const& nftId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getNFTFlags(uint256 const& nftId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getNFTTransferFee(uint256 const& nftId) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected getNFTSequence(uint256 const& nftId) const { return std::unexpected(HostFunctionError::Unimplemented); @@ -397,43 +397,43 @@ public: { } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatFromInt(int64_t x, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatFromUint(uint64_t x, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatFromSTAmount(STAmount const& x, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatFromSTNumber(STNumber const& x, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatToInt(Slice const& x, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatToMantExp(Slice const& x) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatFromMantExp(int64_t mantissa, int32_t exponent, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); @@ -445,31 +445,31 @@ public: return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatAdd(Slice const& x, Slice const& y, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatSubtract(Slice const& x, Slice const& y, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatMultiply(Slice const& x, Slice const& y, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatDivide(Slice const& x, Slice const& y, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatPower(Slice const& x, int32_t n, int32_t mode) const { return std::unexpected(HostFunctionError::Unimplemented); diff --git a/include/xrpl/tx/wasm/HostFuncImpl.h b/include/xrpl/tx/wasm/HostFuncImpl.h index e6cbb20187..8769ef2ef7 100644 --- a/include/xrpl/tx/wasm/HostFuncImpl.h +++ b/include/xrpl/tx/wasm/HostFuncImpl.h @@ -51,9 +51,9 @@ public: std::expected normalizeCacheIndex(int32_t cacheIdx) const { - --cacheIdx; - if (cacheIdx < 0 || cacheIdx >= maxCache) + if (cacheIdx <= 0 || cacheIdx > maxCache) return std::unexpected(HostFunctionError::SlotOutRange); + --cacheIdx; if (!cache_[cacheIdx]) return std::unexpected(HostFunctionError::EmptySlot); return cacheIdx; diff --git a/src/benchmarks/libxrpl/wasm/README.md b/src/benchmarks/libxrpl/wasm/README.md index 7f714ff621..ce1abbb6fe 100644 --- a/src/benchmarks/libxrpl/wasm/README.md +++ b/src/benchmarks/libxrpl/wasm/README.md @@ -35,10 +35,16 @@ contract can buy too cheaply, a denial-of-service vector rather than a rounding jq -r '.benchmarks[] | select(.price_ratio) | [.price_ratio, .name] | @tsv' | sort -n ``` -`unreliable=1` when `rel_error` exceeds 25%, or when `suggested_gas` falls below the crossing floor +`unreliable=1` when `rel_error` exceeds 25%, or when `implied_gas` falls below the crossing floor — a call whose own cost is small next to the crossing is read off the difference of two nearly equal numbers. +The two arms catch different failures, and the second is the one that fires in practice. `rel_error` +is about precision; the floor comparison is about how much of `suggested_gas` was measured for this +case at all. On a quiet Release machine the floor is ~36 gas, and the cheap `Impl` cases sit near 3 — +so the floor is over 90% of their price while `rel_error` reads a comfortable 2%. Expect roughly a +quarter of the priced rows to carry `unreliable=1`, all of them `Impl`. + ### With `--benchmark_repetitions` Adds `_mean` / `_median` / `_stddev` / `_cv` rows. One trap worth knowing: diff --git a/src/benchmarks/libxrpl/wasm/WasmBench.cpp b/src/benchmarks/libxrpl/wasm/WasmBench.cpp index e5975f2403..81bc6f5db7 100644 --- a/src/benchmarks/libxrpl/wasm/WasmBench.cpp +++ b/src/benchmarks/libxrpl/wasm/WasmBench.cpp @@ -391,7 +391,7 @@ report( // equal numbers, so its `suggested_gas` is scatter rather than signal. auto const floor = calibration.crossingFloorGas(); state.counters["unreliable"] = - (totalErr > kMaxRelativeSpread || (floor > 0.0 && suggested < floor)) ? 1 : 0; + (totalErr > kMaxRelativeSpread || (floor > 0.0 && implied < floor)) ? 1 : 0; } } // namespace xrpl::test::bench diff --git a/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp b/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp index d58d20aaac..83f41caa34 100644 --- a/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp +++ b/src/tests/libxrpl/tx/wasm/transactor/BytecodeRun.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -17,6 +18,7 @@ #include #include +#include #include #include @@ -143,9 +145,9 @@ TEST_F(BytecodeRun, TheBytecodeReserveIsHeldWhileTheEscrowLivesAndReleasedWhenIt auto const wasm = assembleWat(gatedOnLedgerSqn(threshold)); auto const created = createEscrow(wasm); - // `calculateAdditionalReserve`: one increment for the escrow, plus one per 500 bytes. - auto const expected = 1U + static_cast(wasm.size() / 500); - EXPECT_EQ(env.getOwnerCount(alice), expected); + auto const held = env.getOwnerCount(alice); + ASSERT_GT(held, 0U); + EXPECT_EQ(held, static_cast(calculateAdditionalReserve(std::optional{wasm}))); while (currentSeq() < threshold) { @@ -156,6 +158,24 @@ TEST_F(BytecodeRun, TheBytecodeReserveIsHeldWhileTheEscrowLivesAndReleasedWhenIt EXPECT_EQ(env.getOwnerCount(alice), 0U); } +TEST(BytecodeReserve, TheBytecodeReserveIsCeilingDivision) +{ + auto const reserveFor = [](std::size_t size) { + return calculateAdditionalReserve(std::optional{Bytes(size, 0x00)}); + }; + + EXPECT_EQ(calculateAdditionalReserve(std::optional{}), 1); + EXPECT_EQ(reserveFor(0), 1); + EXPECT_EQ(reserveFor(1), 1); + EXPECT_EQ(reserveFor(499), 1); + EXPECT_EQ(reserveFor(500), 1); + EXPECT_EQ(reserveFor(501), 2); + EXPECT_EQ(reserveFor(1000), 2); + EXPECT_EQ(reserveFor(1001), 3); + EXPECT_EQ(reserveFor(1500), 3); + EXPECT_EQ(reserveFor(200'000), 400); // kMaxBytecodeSizeLimit +} + TEST_F(BytecodeRun, CreatingChargesTheAmountAndTheFee) { auto const before = env.getXrpBalance(alice);