From 4173f7e499e3dd55900bf72b3c05f68577448060 Mon Sep 17 00:00:00 2001 From: Braedon Klock Date: Mon, 10 Aug 2026 21:30:06 +0000 Subject: [PATCH 1/9] fix: Validate account_lines peer field type (#7728) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- API-CHANGELOG.md | 1 + src/test/rpc/AccountLines_test.cpp | 47 +++++++++++++++++++ .../rpc/handlers/account/AccountLines.cpp | 5 ++ 3 files changed, 53 insertions(+) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 79fb8ff522..bc3672588e 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -54,6 +54,7 @@ This section contains changes targeting a future version. - `submit`: The `fail_hard` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) - `subscribe`: The `taker` field in the `books` array now returns `actMalformed` instead of `badIssuer` if the value is not a valid account. [#6529](https://github.com/XRPLF/rippled/pull/6529) - Fixed a bug in `Forwarded` HTTP header parsing where the extracted IP address could be incorrect when no comma or semicolon delimiter follows the address. This could cause the server to misidentify a client's IP address when operating behind a reverse proxy. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `account_lines`: The `peer` field now returns an error if the value is not a string. [#7728](https://github.com/XRPLF/rippled/pull/7728) ## XRP Ledger server version 3.1.0 diff --git a/src/test/rpc/AccountLines_test.cpp b/src/test/rpc/AccountLines_test.cpp index cb20de9bf5..3de2bdefa3 100644 --- a/src/test/rpc/AccountLines_test.cpp +++ b/src/test/rpc/AccountLines_test.cpp @@ -94,6 +94,24 @@ public: LedgerHeader const ledger3Info = env.closed()->header(); BEAST_EXPECT(ledger3Info.seq == 3); + { + // test peer non-string + auto testInvalidPeerParam = [&](auto const& param) { + json::Value params; + params[jss::account] = alice.human(); + params[jss::peer] = param; + auto jrr = env.rpc("json", "account_lines", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'peer'."); + }; + + testInvalidPeerParam(1); + testInvalidPeerParam(1.1); + testInvalidPeerParam(true); + testInvalidPeerParam(json::Value(json::ValueType::Null)); + testInvalidPeerParam(json::Value(json::ValueType::Object)); + testInvalidPeerParam(json::Value(json::ValueType::Array)); + } { // alice is funded but has no lines. An empty array is returned. json::Value params; @@ -775,6 +793,35 @@ public: LedgerHeader const ledger3Info = env.closed()->header(); BEAST_EXPECT(ledger3Info.seq == 3); + { + // test peer non-string + auto testInvalidPeerParam = [&](auto const& param) { + json::Value params; + params[jss::account] = alice.human(); + params[jss::peer] = param; + + json::Value request; + request[jss::method] = "account_lines"; + request[jss::jsonrpc] = "2.0"; + request[jss::ripplerpc] = "2.0"; + request[jss::id] = 5; + request[jss::params] = params; + + auto const lines = env.rpc("json2", to_string(request)); + BEAST_EXPECT(lines[jss::error][jss::error] == "invalidParams"); + BEAST_EXPECT(lines[jss::error][jss::message] == "Invalid field 'peer'."); + BEAST_EXPECT(lines.isMember(jss::jsonrpc) && lines[jss::jsonrpc] == "2.0"); + BEAST_EXPECT(lines.isMember(jss::ripplerpc) && lines[jss::ripplerpc] == "2.0"); + BEAST_EXPECT(lines.isMember(jss::id) && lines[jss::id] == 5); + }; + + testInvalidPeerParam(1); + testInvalidPeerParam(1.1); + testInvalidPeerParam(true); + testInvalidPeerParam(json::Value(json::ValueType::Null)); + testInvalidPeerParam(json::Value(json::ValueType::Object)); + testInvalidPeerParam(json::Value(json::ValueType::Array)); + } { // alice is funded but has no lines. An empty array is returned. json::Value params; diff --git a/src/xrpld/rpc/handlers/account/AccountLines.cpp b/src/xrpld/rpc/handlers/account/AccountLines.cpp index 4a6d22d5d8..ac98e271b6 100644 --- a/src/xrpld/rpc/handlers/account/AccountLines.cpp +++ b/src/xrpld/rpc/handlers/account/AccountLines.cpp @@ -107,7 +107,12 @@ doAccountLines(rpc::JsonContext& context) std::string strPeer; if (params.isMember(jss::peer)) + { + if (!params[jss::peer].isString()) + return rpc::invalidFieldError(jss::peer); + strPeer = params[jss::peer].asString(); + } auto const raPeerAccount = [&]() -> std::optional { return strPeer.empty() ? std::nullopt : parseBase58(strPeer); From 60291c3ed613a749f6aeced06d485b1d478d9843 Mon Sep 17 00:00:00 2001 From: Kassaking7 <96991820+Kassaking7@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:34:28 +0000 Subject: [PATCH 2/9] fix: Allow OverrideFreeze to bypass individual/deep freeze on AMM trust lines (#6959) --- include/xrpl/tx/invariants/FreezeInvariant.h | 6 +- src/libxrpl/tx/invariants/FreezeInvariant.cpp | 23 +- src/test/app/AMMClawback_test.cpp | 204 ++++++++++++++++++ 3 files changed, 222 insertions(+), 11 deletions(-) diff --git a/include/xrpl/tx/invariants/FreezeInvariant.h b/include/xrpl/tx/invariants/FreezeInvariant.h index 4b3e9beec4..c66e002872 100644 --- a/include/xrpl/tx/invariants/FreezeInvariant.h +++ b/include/xrpl/tx/invariants/FreezeInvariant.h @@ -69,7 +69,8 @@ private: IssuerChanges const& changes, STTx const& tx, beast::Journal const& j, - bool enforce); + bool enforce, + bool fixOverrideFreeze); static bool validateFrozenState( @@ -78,7 +79,8 @@ private: STTx const& tx, beast::Journal const& j, bool enforce, - bool globalFreeze); + bool globalFreeze, + bool fixOverrideFreeze); }; } // namespace xrpl diff --git a/src/libxrpl/tx/invariants/FreezeInvariant.cpp b/src/libxrpl/tx/invariants/FreezeInvariant.cpp index 0a604d4c39..c4340b9aec 100644 --- a/src/libxrpl/tx/invariants/FreezeInvariant.cpp +++ b/src/libxrpl/tx/invariants/FreezeInvariant.cpp @@ -73,6 +73,7 @@ TransfersNotFrozen::finalize( * view.rules().enabled(fixFreezeExploit); */ [[maybe_unused]] bool const enforce = view.rules().enabled(featureDeepFreeze); + bool const fixOverrideFreeze = view.rules().enabled(fixCleanup3_4_0); return std::ranges::all_of(balanceChanges_, [&](auto const& entry) { auto const& [issue, changes] = entry; @@ -90,7 +91,7 @@ TransfersNotFrozen::finalize( return !enforce; } - return validateIssuerChanges(issuerSle, changes, tx, j, enforce); + return validateIssuerChanges(issuerSle, changes, tx, j, enforce, fixOverrideFreeze); }); } @@ -199,7 +200,8 @@ TransfersNotFrozen::validateIssuerChanges( IssuerChanges const& changes, STTx const& tx, beast::Journal const& j, - bool enforce) + bool enforce, + bool fixOverrideFreeze) { if (!issuer) { @@ -225,7 +227,7 @@ TransfersNotFrozen::validateIssuerChanges( { bool const high = change.line->at(sfLowLimit).getIssuer() == issuer->at(sfAccount); - if (!validateFrozenState(change, high, tx, j, enforce, globalFreeze)) + if (!validateFrozenState(change, high, tx, j, enforce, globalFreeze, fixOverrideFreeze)) { return false; } @@ -241,26 +243,29 @@ TransfersNotFrozen::validateFrozenState( STTx const& tx, beast::Journal const& j, bool enforce, - bool globalFreeze) + bool globalFreeze, + bool fixOverrideFreeze) { bool const freeze = change.balanceChangeSign < 0 && change.line->isFlag(high ? lsfLowFreeze : lsfHighFreeze); bool const deepFreeze = change.line->isFlag(high ? lsfLowDeepFreeze : lsfHighDeepFreeze); bool const frozen = globalFreeze || deepFreeze || freeze; - bool const isAMMLine = change.line->isFlag(lsfAMMNode); - if (!frozen) { return true; } - // AMMClawbacks are allowed to override some freeze rules - if ((!isAMMLine || globalFreeze) && hasPrivilege(tx, OverrideFreeze)) + // Pre-fixCleanup3_4_0: the isAMMLine check incorrectly blocked clawback on + // individually-frozen or deep-frozen AMM trust lines. + // Post-fixCleanup3_4_0: AMMClawbacks are allowed to override all freeze types. + bool const isAMMLine = change.line->isFlag(lsfAMMNode); + if ((fixOverrideFreeze || !isAMMLine || globalFreeze) && hasPrivilege(tx, OverrideFreeze)) { JLOG(j.debug()) << "Invariant check allowing funds to be moved " << (change.balanceChangeSign > 0 ? "to" : "from") - << " a frozen trustline for AMMClawback " << tx.getTransactionID(); + << " a frozen trustline for a freeze privileged transaction " + << tx.getTransactionID(); return true; } diff --git a/src/test/app/AMMClawback_test.cpp b/src/test/app/AMMClawback_test.cpp index ba416d8192..90bface1fb 100644 --- a/src/test/app/AMMClawback_test.cpp +++ b/src/test/app/AMMClawback_test.cpp @@ -2155,6 +2155,209 @@ class AMMClawback_test : public beast::unit_test::Suite } BEAST_EXPECT(env.balance(carol, eur) == eur(7750)); } + + // gw (USD issuer) individually freezes the AMM-USD trust line. + // AMMClawback must still succeed because the freeze invariant + // short-circuits before reaching the AMM line check (no receivers in + // the USD issuer's change set). Behavior is identical with or without + // fixCleanup3_4_0. + { + Env env(*this, features); + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + Account const alice{"alice"}; + env.fund(XRP(1000000), gw, gw2, alice); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + env.require(Flags(gw, asfAllowTrustLineClawback)); + + auto const usd = gw["USD"]; + env.trust(usd(100000), alice); + env(pay(gw, alice, usd(3000))); + env.close(); + + auto const eur = gw2["EUR"]; + env.trust(eur(100000), alice); + env(pay(gw2, alice, eur(3000))); + env.close(); + + AMM const amm(env, alice, eur(1000), usd(2000), Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT( + amm.expectBalances(usd(2000), eur(1000), IOUAmount{1414213562373095, -12})); + + // gw individually freezes the AMM-USD trust line (AMM pseudo-account + // <-> gw), not alice's trust line. + env(trust(gw, STAmount{Issue{usd.currency, amm.ammAccount()}, 0}, tfSetFreeze)); + env.close(); + + env(amm::ammClawback(gw, alice, usd, eur, usd(1000)), Ter(tesSUCCESS)); + env.close(); + + env.require(Balance(alice, usd(1000))); + env.require(Balance(alice, eur(2500))); + BEAST_EXPECT(amm.expectBalances(usd(1000), eur(500), IOUAmount{7071067811865475, -13})); + BEAST_EXPECT(amm.expectLPTokens(alice, IOUAmount{7071067811865475, -13})); + } + + // gw2 (EUR issuer) individually freezes the AMM-EUR trust line. + // The EUR flow (AMM → alice) is a genuine P2P transfer checked by the + // freeze invariant. Pre-fixCleanup3_4_0 the isAMMNode guard incorrectly + // blocked AMMClawback's overrideFreeze privilege on that trust line. + { + Env env(*this, features); + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + Account const alice{"alice"}; + env.fund(XRP(1000000), gw, gw2, alice); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + env.require(Flags(gw, asfAllowTrustLineClawback)); + + auto const usd = gw["USD"]; + env.trust(usd(100000), alice); + env(pay(gw, alice, usd(3000))); + env.close(); + + auto const eur = gw2["EUR"]; + env.trust(eur(100000), alice); + env(pay(gw2, alice, eur(3000))); + env.close(); + + AMM const amm(env, alice, eur(1000), usd(2000), Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT( + amm.expectBalances(usd(2000), eur(1000), IOUAmount{1414213562373095, -12})); + + // gw2 individually freezes the AMM-EUR trust line. + env(trust(gw2, STAmount{Issue{eur.currency, amm.ammAccount()}, 0}, tfSetFreeze)); + env.close(); + + if (features[fixCleanup3_4_0]) + { + // Post-fixCleanup3_4_0: overrideFreeze privilege applies to + // all freeze types on AMM trust lines. + env(amm::ammClawback(gw, alice, usd, eur, usd(1000)), Ter(tesSUCCESS)); + env.close(); + + env.require(Balance(alice, usd(1000))); + env.require(Balance(alice, eur(2500))); + BEAST_EXPECT( + amm.expectBalances(usd(1000), eur(500), IOUAmount{7071067811865475, -13})); + BEAST_EXPECT(amm.expectLPTokens(alice, IOUAmount{7071067811865475, -13})); + } + else + { + // Pre-fixCleanup3_4_0: the isAMMNode guard prevents the + // overrideFreeze privilege from applying to individually-frozen + // AMM trust lines, so the invariant blocks the clawback. + env(amm::ammClawback(gw, alice, usd, eur, usd(1000)), Ter(tecINVARIANT_FAILED)); + } + } + + // gw2 (EUR issuer) globally freezes its issued assets. AMMClawback + // must still be able to return EUR from the AMM to alice. + { + Env env(*this, features); + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + Account const alice{"alice"}; + env.fund(XRP(1000000), gw, gw2, alice); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + env.require(Flags(gw, asfAllowTrustLineClawback)); + + auto const usd = gw["USD"]; + env.trust(usd(100000), alice); + env(pay(gw, alice, usd(3000))); + env.close(); + + auto const eur = gw2["EUR"]; + env.trust(eur(100000), alice); + env(pay(gw2, alice, eur(3000))); + env.close(); + + AMM const amm(env, alice, eur(1000), usd(2000), Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT( + amm.expectBalances(usd(2000), eur(1000), IOUAmount{1414213562373095, -12})); + + env(fset(gw2, asfGlobalFreeze)); + env.close(); + + env(amm::ammClawback(gw, alice, usd, eur, usd(1000)), Ter(tesSUCCESS)); + env.close(); + + env.require(Balance(alice, usd(1000))); + env.require(Balance(alice, eur(2500))); + BEAST_EXPECT(amm.expectBalances(usd(1000), eur(500), IOUAmount{7071067811865475, -13})); + BEAST_EXPECT(amm.expectLPTokens(alice, IOUAmount{7071067811865475, -13})); + } + + // Same as above but gw2 deep-freezes the AMM-EUR trust line. + if (features[featureDeepFreeze]) + { + Env env(*this, features); + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + Account const alice{"alice"}; + env.fund(XRP(1000000), gw, gw2, alice); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + env.require(Flags(gw, asfAllowTrustLineClawback)); + + auto const usd = gw["USD"]; + env.trust(usd(100000), alice); + env(pay(gw, alice, usd(3000))); + env.close(); + + auto const eur = gw2["EUR"]; + env.trust(eur(100000), alice); + env(pay(gw2, alice, eur(3000))); + env.close(); + + AMM const amm(env, alice, eur(1000), usd(2000), Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT( + amm.expectBalances(usd(2000), eur(1000), IOUAmount{1414213562373095, -12})); + + // gw2 deep-freezes the AMM-EUR trust line. + env(trust( + gw2, + STAmount{Issue{eur.currency, amm.ammAccount()}, 0}, + tfSetFreeze | tfSetDeepFreeze)); + env.close(); + + if (features[fixCleanup3_4_0]) + { + env(amm::ammClawback(gw, alice, usd, eur, usd(1000)), Ter(tesSUCCESS)); + env.close(); + + env.require(Balance(alice, usd(1000))); + env.require(Balance(alice, eur(2500))); + BEAST_EXPECT( + amm.expectBalances(usd(1000), eur(500), IOUAmount{7071067811865475, -13})); + BEAST_EXPECT(amm.expectLPTokens(alice, IOUAmount{7071067811865475, -13})); + } + else + { + // Pre-fixCleanup3_4_0: same isAMMNode guard issue blocks the + // clawback on deep-frozen AMM trust lines. + env(amm::ammClawback(gw, alice, usd, eur, usd(1000)), Ter(tecINVARIANT_FAILED)); + } + } } void @@ -2530,6 +2733,7 @@ class AMMClawback_test : public beast::unit_test::Suite // precision loss caught in transaction layer -> tecPRECISION_LOSS all - fixAMMClawbackRounding - featureMPTokensV2, all - featureMPTokensV2, + all - fixCleanup3_4_0, all}) { testAMMClawbackSpecificAmount(features); From 6f5de9067aedad3ae5f7bb555d102ca67a67fb60 Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:37:38 +0000 Subject: [PATCH 3/9] chore: Mark unreachable branches in Confidential Transfer with UNREACHABLE (#7903) --- src/libxrpl/protocol/ConfidentialTransfer.cpp | 91 ++++++++++++++++--- .../token/ConfidentialMPTClawback.cpp | 55 +++++++++-- .../token/ConfidentialMPTConvert.cpp | 53 +++++++++-- .../token/ConfidentialMPTConvertBack.cpp | 46 +++++++++- .../token/ConfidentialMPTMergeInbox.cpp | 35 ++++++- .../transactors/token/ConfidentialMPTSend.cpp | 59 ++++++++++-- 6 files changed, 298 insertions(+), 41 deletions(-) diff --git a/src/libxrpl/protocol/ConfidentialTransfer.cpp b/src/libxrpl/protocol/ConfidentialTransfer.cpp index fe8a08c2ef..ecd4832928 100644 --- a/src/libxrpl/protocol/ConfidentialTransfer.cpp +++ b/src/libxrpl/protocol/ConfidentialTransfer.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -124,7 +125,12 @@ std::optional makeEcPair(Slice const& buffer) { if (buffer.length() != 2 * kEcCiphertextComponentLength) - return std::nullopt; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::makeEcPair : callers must pre-validate ciphertext length"); + return std::nullopt; + // LCOV_EXCL_STOP + } auto parsePubKey = [](Slice const& slice, secp256k1_pubkey& out) { return secp256k1_ec_pubkey_parse(secp256k1Context(), &out, slice.data(), slice.length()); @@ -266,7 +272,13 @@ std::optional encryptCanonicalZeroAmount(Slice const& pubKeySlice, AccountID const& account, MPTID const& mptId) { if (pubKeySlice.size() != kEcPubKeyLength) - return std::nullopt; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::encryptCanonicalZeroAmount : callers must pre-validate public key length"); + return std::nullopt; + // LCOV_EXCL_STOP + } EcPair pair{}; secp256k1_pubkey pubKey; @@ -274,14 +286,24 @@ encryptCanonicalZeroAmount(Slice const& pubKeySlice, AccountID const& account, M secp256k1Context(), &pubKey, pubKeySlice.data(), kEcPubKeyLength); res != 1) { - return std::nullopt; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::encryptCanonicalZeroAmount : public key read from the ledger must already be " + "valid"); + return std::nullopt; + // LCOV_EXCL_STOP } if (auto res = generate_canonical_encrypted_zero( secp256k1Context(), &pair.c1, &pair.c2, &pubKey, account.data(), mptId.data()); res != 1) { - return std::nullopt; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::encryptCanonicalZeroAmount : canonical zero generation cannot fail for a " + "valid public key"); + return std::nullopt; + // LCOV_EXCL_STOP } return serializeEcPair(pair); @@ -301,7 +323,11 @@ verifyRevealedAmount( issuer.publicKey.size() != kEcPubKeyLength || issuer.encryptedAmount.size() != kEcGamalEncryptedTotalLength) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifyRevealedAmount : callers must pre-validate holder/issuer field lengths"); + return tecINTERNAL; + // LCOV_EXCL_STOP } auto const holderP = toParticipant(holder); @@ -313,7 +339,11 @@ verifyRevealedAmount( if (auditor->publicKey.size() != kEcPubKeyLength || auditor->encryptedAmount.size() != kEcGamalEncryptedTotalLength) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifyRevealedAmount : callers must pre-validate auditor field lengths"); + return tecINTERNAL; + // LCOV_EXCL_STOP } auditorP = toParticipant(*auditor); auditorPtr = &auditorP; @@ -337,7 +367,12 @@ checkEncryptedAmountFormat(STObject const& object) if (!object.isFieldPresent(sfHolderEncryptedAmount) || !object.isFieldPresent(sfIssuerEncryptedAmount)) { - return temMALFORMED; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::checkEncryptedAmountFormat : callers already enforce that these fields are " + "present"); + return temMALFORMED; + // LCOV_EXCL_STOP } if (object[sfHolderEncryptedAmount].length() != kEcGamalEncryptedTotalLength || @@ -366,7 +401,12 @@ TER verifySchnorrProof(Slice const& pubKeySlice, Slice const& proofSlice, uint256 const& contextHash) { if (proofSlice.size() != kEcSchnorrProofLength || pubKeySlice.size() != kEcPubKeyLength) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::verifySchnorrProof : callers must pre-validate proof/public key length"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } if (mpt_verify_convert_proof(proofSlice.data(), pubKeySlice.data(), contextHash.data()) != 0) return tecBAD_PROOF; @@ -385,7 +425,12 @@ verifyClawbackProof( if (ciphertext.size() != kEcGamalEncryptedTotalLength || pubKeySlice.size() != kEcPubKeyLength || proof.size() != kEcClawbackProofLength) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifyClawbackProof : callers must pre-validate ciphertext/public " + "key/proof length"); + return tecINTERNAL; + // LCOV_EXCL_STOP } if (mpt_verify_clawback_proof( @@ -420,7 +465,12 @@ verifySendProof( amountCommitment.size() != kEcPedersenCommitmentLength || balanceCommitment.size() != kEcPedersenCommitmentLength) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifySendProof : callers must pre-validate proof/participant/commitment " + "lengths"); + return tecINTERNAL; + // LCOV_EXCL_STOP } std::vector participants; @@ -433,12 +483,22 @@ verifySendProof( if (auditor->publicKey.size() != kEcPubKeyLength || auditor->encryptedAmount.size() != kEcGamalEncryptedTotalLength) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE("xrpl::verifySendProof : callers must pre-validate auditor field lengths"); + return tecINTERNAL; + // LCOV_EXCL_STOP } participants.push_back(toParticipant(*auditor)); } if (participants.size() != recipientCount) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifySendProof : participant count must match the requested recipient " + "count"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } if (mpt_verify_send_proof( proof.data(), @@ -468,7 +528,12 @@ verifyConvertBackProof( spendingBalance.size() != kEcGamalEncryptedTotalLength || balanceCommitment.size() != kEcPedersenCommitmentLength) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifyConvertBackProof : callers must pre-validate proof/public " + "key/balance/commitment lengths"); + return tecINTERNAL; + // LCOV_EXCL_STOP } if (mpt_verify_convert_back_proof( diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTClawback.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTClawback.cpp index 6366e99105..19ec99702a 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTClawback.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTClawback.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -70,7 +71,14 @@ ConfidentialMPTClawback::preclaim(PreclaimContext const& ctx) // Sanity check: account must be the same as issuer if (sleIssuance->getAccountID(sfIssuer) != account) - return tefINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTClawback::preclaim : preflight already validated the " + "submitter is the issuer"); + return tefINTERNAL; + // LCOV_EXCL_STOP + } // Check if issuance has issuer ElGamal public key if (!sleIssuance->isFieldPresent(sfIssuerEncryptionKey)) @@ -127,7 +135,14 @@ ConfidentialMPTClawback::doApply() auto sleHolderMPToken = view().peek(keylet::mptoken(mptIssuanceID, holder)); if (!sleIssuance || !sleHolderMPToken) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTClawback::doApply : preclaim already validated these " + "objects exist"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const clawAmount = ctx_.tx[sfMPTAmount]; @@ -137,11 +152,25 @@ ConfidentialMPTClawback::doApply() // After clawback, the balance should be encrypted zero. auto const encZeroForHolder = encryptCanonicalZeroAmount(holderPubKey, holder, mptIssuanceID); if (!encZeroForHolder) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTClawback::doApply : canonical zero encryption cannot fail " + "for an already-valid holder public key"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto encZeroForIssuer = encryptCanonicalZeroAmount(issuerPubKey, holder, mptIssuanceID); if (!encZeroForIssuer) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTClawback::doApply : canonical zero encryption cannot fail " + "for an already-valid issuer public key"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } // Set holder's confidential balances to encrypted zero (*sleHolderMPToken)[sfConfidentialBalanceInbox] = *encZeroForHolder; @@ -154,14 +183,28 @@ ConfidentialMPTClawback::doApply() // Sanity check: the issuance must have an auditor public key if // auditing is enabled. if (!sleIssuance->isFieldPresent(sfAuditorEncryptionKey)) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTClawback::doApply : the holder's auditor balance implies " + "the issuance has an auditor public key"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const auditorPubKey = (*sleIssuance)[sfAuditorEncryptionKey]; auto encZeroForAuditor = encryptCanonicalZeroAmount(auditorPubKey, holder, mptIssuanceID); if (!encZeroForAuditor) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTClawback::doApply : canonical zero encryption cannot " + "fail for an already-valid auditor public key"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } (*sleHolderMPToken)[sfAuditorEncryptedBalance] = std::move(*encZeroForAuditor); } diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp index 454eb39ead..5be3892151 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -89,7 +90,14 @@ ConfidentialMPTConvert::preclaim(PreclaimContext const& ctx) // already checked in preflight, but should also check that issuer on the // issuance isn't the account either if (sleIssuance->getAccountID(sfIssuer) == account) - return tefINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvert::preclaim : issuer derived from the MPT ID must " + "match the ledger's stored issuer"); + return tefINTERNAL; + // LCOV_EXCL_STOP + } bool const hasAuditor = ctx.tx.isFieldPresent(sfAuditorEncryptedAmount); bool const requiresAuditor = sleIssuance->isFieldPresent(sfAuditorEncryptionKey); @@ -207,11 +215,25 @@ ConfidentialMPTConvert::doApply() auto sleMptoken = view().peek(keylet::mptoken(mptIssuanceID, accountID_)); if (!sleMptoken) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvert::doApply : preclaim already validated the MPToken " + "exists"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto sleIssuance = view().peek(keylet::mptokenIssuance(mptIssuanceID)); if (!sleIssuance) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvert::doApply : preclaim already validated the issuance " + "exists"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const amtToConvert = ctx_.tx[sfMPTAmount]; auto const amt = (*sleMptoken)[~sfMPTAmount].valueOr(0); @@ -273,7 +295,14 @@ ConfidentialMPTConvert::doApply() if (auditorEc) { if (!sleMptoken->isFieldPresent(sfAuditorEncryptedBalance)) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvert::doApply : issuance-level auditing implies " + "the MPToken already carries an auditor balance"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto sum = homomorphicAdd(*auditorEc, (*sleMptoken)[sfAuditorEncryptedBalance]); if (!sum) @@ -308,7 +337,14 @@ ConfidentialMPTConvert::doApply() (*sleMptoken)[sfHolderEncryptionKey], accountID_, mptIssuanceID); if (!zeroBalance) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvert::doApply : canonical zero encryption cannot fail " + "for an already-valid holder public key"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } (*sleMptoken)[sfConfidentialBalanceSpending] = std::move(*zeroBalance); } @@ -316,7 +352,12 @@ ConfidentialMPTConvert::doApply() { // both sfIssuerEncryptedBalance and sfConfidentialBalanceInbox should // exist together - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvert::doApply : confidential balance fields must be all " + "present or all absent"); + return tecINTERNAL; + // LCOV_EXCL_STOP } view().update(sleIssuance); diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp index 87f9e476d6..1e3617ffbd 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -72,7 +73,14 @@ verifyProofs( std::shared_ptr const& mptoken) { if (!mptoken->isFieldPresent(sfHolderEncryptionKey)) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::verifyProofs : preclaim already validated the holder encryption key is " + "present"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const mptIssuanceID = tx[sfMPTokenIssuanceID]; auto const account = tx[sfAccount]; @@ -169,7 +177,14 @@ ConfidentialMPTConvertBack::preclaim(PreclaimContext const& ctx) // already checked in preflight, but should also check that issuer on // the issuance isn't the account either if (sleIssuance->getAccountID(sfIssuer) == account) - return tefINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvertBack::preclaim : issuer derived from the MPT ID must " + "match the ledger's stored issuer"); + return tefINTERNAL; + // LCOV_EXCL_STOP + } auto const sleMptoken = ctx.view.read(keylet::mptoken(mptIssuanceID, account)); if (!sleMptoken) @@ -185,7 +200,14 @@ ConfidentialMPTConvertBack::preclaim(PreclaimContext const& ctx) // Sanity check: holder's MPToken must have auditor balance field if auditing // is enabled if (requiresAuditor && !sleMptoken->isFieldPresent(sfAuditorEncryptedBalance)) - return tefINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvertBack::preclaim : issuance-level auditing implies the " + "MPToken already carries an auditor balance"); + return tefINTERNAL; + // LCOV_EXCL_STOP + } // if the total circulating confidential balance is smaller than what the // holder is trying to convert back, we know for sure this txn should @@ -215,11 +237,25 @@ ConfidentialMPTConvertBack::doApply() auto sleMptoken = view().peek(keylet::mptoken(mptIssuanceID, accountID_)); if (!sleMptoken) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvertBack::doApply : preclaim already validated the " + "MPToken exists"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto sleIssuance = view().peek(keylet::mptokenIssuance(mptIssuanceID)); if (!sleIssuance) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTConvertBack::doApply : preclaim already validated the " + "issuance exists"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const amtToConvertBack = ctx_.tx[sfMPTAmount]; auto const amt = (*sleMptoken)[~sfMPTAmount].valueOr(0); diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTMergeInbox.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTMergeInbox.cpp index 0b98382a61..6485578cb4 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTMergeInbox.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTMergeInbox.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -49,7 +50,14 @@ ConfidentialMPTMergeInbox::preclaim(PreclaimContext const& ctx) // already checked in preflight, but should also check that issuer on the // issuance isn't the account either if (sleIssuance->getAccountID(sfIssuer) == ctx.tx[sfAccount]) - return tefINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTMergeInbox::preclaim : issuer derived from the MPT ID must " + "match the ledger's stored issuer"); + return tefINTERNAL; + // LCOV_EXCL_STOP + } auto const sleMptoken = ctx.view.read(keylet::mptoken(ctx.tx[sfMPTokenIssuanceID], ctx.tx[sfAccount])); @@ -82,14 +90,26 @@ ConfidentialMPTMergeInbox::doApply() auto const mptIssuanceID = ctx_.tx[sfMPTokenIssuanceID]; auto sleMptoken = view().peek(keylet::mptoken(mptIssuanceID, accountID_)); if (!sleMptoken) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTMergeInbox::doApply : preclaim already validated the " + "MPToken exists"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } // sanity check if (!sleMptoken->isFieldPresent(sfConfidentialBalanceSpending) || !sleMptoken->isFieldPresent(sfConfidentialBalanceInbox) || !sleMptoken->isFieldPresent(sfHolderEncryptionKey)) { - return tecINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTMergeInbox::doApply : preclaim already validated these " + "fields are present"); + return tecINTERNAL; + // LCOV_EXCL_STOP } // Merge inbox into spending: spending = spending + inbox @@ -114,7 +134,14 @@ ConfidentialMPTMergeInbox::doApply() encryptCanonicalZeroAmount((*sleMptoken)[sfHolderEncryptionKey], accountID_, mptIssuanceID); if (!zeroEncryption) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTMergeInbox::doApply : canonical zero encryption cannot fail " + "for an already-valid holder public key"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } (*sleMptoken)[sfConfidentialBalanceInbox] = std::move(*zeroEncryption); diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp index f4c7b98c41..e713ae5029 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -105,7 +106,14 @@ verifySendProofs( { // Sanity check if (!sleSenderMPToken || !sleDestinationMPToken || !sleIssuance) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::detail::verifySendProofs : caller must pre-validate sender/destination/" + "issuance existence"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const hasAuditor = ctx.tx.isFieldPresent(sfAuditorEncryptedAmount); @@ -204,7 +212,14 @@ ConfidentialMPTSend::preclaim(PreclaimContext const& ctx) // Sanity check: issuer isn't the sender if (sleIssuance->getAccountID(sfIssuer) == ctx.tx[sfAccount]) - return tefINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTSend::preclaim : issuer derived from the MPT ID must match " + "the ledger's stored issuer"); + return tefINTERNAL; + // LCOV_EXCL_STOP + } // Check sender's MPToken existence auto const sleSenderMPToken = ctx.view.read(keylet::mptoken(mptIssuanceID, account)); @@ -238,7 +253,12 @@ ConfidentialMPTSend::preclaim(PreclaimContext const& ctx) (!sleSenderMPToken->isFieldPresent(sfAuditorEncryptedBalance) || !sleDestinationMPToken->isFieldPresent(sfAuditorEncryptedBalance))) { - return tefINTERNAL; // LCOV_EXCL_LINE + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTSend::preclaim : issuance-level auditing implies both " + "MPTokens already carry an auditor balance"); + return tefINTERNAL; + // LCOV_EXCL_STOP } // Check lock @@ -283,7 +303,14 @@ ConfidentialMPTSend::doApply() auto const sleDestAcct = view().read(keylet::account(destination)); if (!sleSenderMPToken || !sleDestinationMPToken || !sleIssuance || !sleDestAcct) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ConfidentialMPTSend::doApply : preclaim already validated these objects " + "exist"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } // Deposit preauth authorization was already verified in preclaim. // Remove any expired credentials. @@ -353,7 +380,13 @@ ConfidentialMPTSend::doApply() auto rerandomizedDestEc = rerandomizeCiphertext( destEc, (*sleDestinationMPToken)[sfHolderEncryptionKey], sendChallenge); if (!rerandomizedDestEc) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + JLOG(ctx_.journal.error()) + << "ConfidentialMPTSend failed to rerandomize destination inbox ciphertext."; + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const curInbox = (*sleDestinationMPToken)[sfConfidentialBalanceInbox]; auto newInbox = homomorphicAdd(curInbox, *rerandomizedDestEc); @@ -374,7 +407,13 @@ ConfidentialMPTSend::doApply() auto rerandomizedIssuerEc = rerandomizeCiphertext(issuerEc, (*sleIssuance)[sfIssuerEncryptionKey], sendChallenge); if (!rerandomizedIssuerEc) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + JLOG(ctx_.journal.error()) + << "ConfidentialMPTSend failed to rerandomize destination issuer ciphertext."; + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const curIssuerEnc = (*sleDestinationMPToken)[sfIssuerEncryptedBalance]; auto newIssuerEnc = homomorphicAdd(curIssuerEnc, *rerandomizedIssuerEc); @@ -396,7 +435,13 @@ ConfidentialMPTSend::doApply() auto rerandomizedAuditorEc = rerandomizeCiphertext( *auditorEc, (*sleIssuance)[sfAuditorEncryptionKey], sendChallenge); if (!rerandomizedAuditorEc) - return tecINTERNAL; // LCOV_EXCL_LINE + { + // LCOV_EXCL_START + JLOG(ctx_.journal.error()) + << "ConfidentialMPTSend failed to rerandomize destination auditor ciphertext."; + return tecINTERNAL; + // LCOV_EXCL_STOP + } auto const curAuditorEnc = (*sleDestinationMPToken)[sfAuditorEncryptedBalance]; auto newAuditorEnc = homomorphicAdd(curAuditorEnc, *rerandomizedAuditorEc); From 909cc5bba90879b6187595d46af743fa38df7c51 Mon Sep 17 00:00:00 2001 From: Bryan Date: Mon, 10 Aug 2026 21:37:53 +0000 Subject: [PATCH 4/9] fix: Prevent silent zero AMM clawbacks due to integer MPT rounding (#7704) Co-authored-by: Bart --- .../tx/transactors/dex/AMMClawback.cpp | 9 +- src/test/app/AMMClawbackMPT_test.cpp | 155 +++++++++++++++++- 2 files changed, 156 insertions(+), 8 deletions(-) diff --git a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp index c1ef9f875e..455b2ad5c5 100644 --- a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp @@ -256,7 +256,7 @@ AMMClawback::applyGuts(Sandbox& sb) } if (!isTesSuccess(result)) - return result; // LCOV_EXCL_LINE + return result; if (sb.rules().enabled(fixCleanup3_3_0) && sb.rules().enabled(fixAMMv1_3)) { @@ -353,6 +353,13 @@ AMMClawback::equalWithdrawMatchingOneAmount( auto amountRounded = getRoundedAsset(rules, amountBalance, frac, IsDeposit::No); + // The requested clawback amount is likely too small and results in + // one-sided pool withdrawal due to round off. Fail so the issuer can + // clawback a larger amount. + if (rules.enabled(fixCleanup3_4_0) && + (amountRounded == beast::kZero || amount2Rounded == beast::kZero)) + return {tecAMM_FAILED, STAmount{}, STAmount{}, STAmount{}}; + return AMMWithdraw::withdraw( sb, ammSle, diff --git a/src/test/app/AMMClawbackMPT_test.cpp b/src/test/app/AMMClawbackMPT_test.cpp index 6facafde4a..6c7aa99156 100644 --- a/src/test/app/AMMClawbackMPT_test.cpp +++ b/src/test/app/AMMClawbackMPT_test.cpp @@ -137,7 +137,6 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite AMM amm(env, gw, btc(100), usd(100)); env.close(); amm.deposit(alice, 1'000); - env.close(); // can not clawback when tfMPTCanClawback is not enabled env(amm::ammClawback(gw, alice, btc, usd, std::nullopt), Ter(tecNO_PERMISSION)); @@ -503,6 +502,150 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite } } + void + testAMMClawbackAmountRoundsToZero(FeatureBitset features) + { + // Ensure a clawback that rounds down to zero MPT fails with + // tecAMM_FAILED instead of silently burning the holder's LP. + testcase("test AMMClawback amount that rounds down to zero"); + using namespace jtx; + + Env env(*this, features); + Account const gw{"gateway"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + env.fund(XRP(10'000'000), gw, alice, bob); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + + // The clawed asset (amountRounded) rounds to zero while its XRP + // counterpart is always large. + { + MPTTester const mptBtc( + {.env = env, + .issuer = gw, + .holders = {alice, bob}, + .pay = 1'000, + .flags = tfMPTCanClawback | kMptDexFlags}); + MPT const btc = mptBtc; + + AMM amm(env, alice, btc(3), XRP(333'000)); + amm.deposit(bob, btc(3), XRP(333'000)); + + [[maybe_unused]] auto const [poolBtcBefore, poolXrpBefore, lptBefore] = amm.balances(); + BEAST_EXPECT(poolBtcBefore == btc(6)); + + auto const issuerOABefore = mptBtc.getBalance(gw); + auto const aliceLpBefore = amm.getLPTokensBalance(alice.id()); + auto const bobLpBefore = amm.getLPTokensBalance(bob.id()); + + // Attempt to clawback 1/6th of the BTC pool. When the zero-rounding + // guard is active (gated by fixCleanup3_4_0) the rounded amount + // drops to 0 and should trigger tecAMM_FAILED. + env(amm::ammClawback(gw, alice, btc, XRP, btc(1)), + Ter(features[fixCleanup3_4_0] ? TER{tecAMM_FAILED} : TER{tesSUCCESS})); + env.close(); + + [[maybe_unused]] auto const [poolBtcAfter, poolXrpAfter, lptAfter] = amm.balances(); + auto const issuerOAAfter = mptBtc.getBalance(gw); + auto const aliceLpAfter = amm.getLPTokensBalance(alice.id()); + auto const bobLpAfter = amm.getLPTokensBalance(bob.id()); + + if (features[fixCleanup3_4_0]) + { + // Post-fixCleanup3_4_0: Clawback fails because the BTC balance + // would round to zero. All balances must remain untouched. + BEAST_EXPECT(poolBtcAfter == poolBtcBefore); + BEAST_EXPECT(poolXrpAfter == poolXrpBefore); + BEAST_EXPECT(issuerOAAfter == issuerOABefore); + BEAST_EXPECT(aliceLpAfter == aliceLpBefore); + BEAST_EXPECT(bobLpAfter == bobLpBefore); + } + else + { + // Pre-fixCleanup3_4_0: BTC rounds to zero and the clawback + // silently burns alice's LP without clawing back any BTC. + BEAST_EXPECT(poolBtcAfter == poolBtcBefore); + BEAST_EXPECT(poolXrpAfter < poolXrpBefore); + BEAST_EXPECT(issuerOAAfter == issuerOABefore); + BEAST_EXPECT(aliceLpAfter < aliceLpBefore); + BEAST_EXPECT(bobLpAfter == bobLpBefore); + } + } + + // The pool above only ever rounds the clawed asset (amountRounded) to + // zero; its XRP counterpart is always large. Exercise the other operand + // of the guard (amount2Rounded == 0) with an MPT/MPT pool where the + // *paired* asset is the tiny integer that floors to zero while the + // clawed asset still rounds non-zero. + { + Account const carol{"carol"}; + Account const dan{"dan"}; + env.fund(XRP(10'000'000), carol, dan); + env.close(); + + MPTTester const mptBtc( + {.env = env, + .issuer = gw, + .holders = {carol, dan}, + .pay = 100'000, + .flags = tfMPTCanClawback | kMptDexFlags}); + MPT const btc = mptBtc; + + MPTTester const mptEth( + {.env = env, + .issuer = gw, + .holders = {carol, dan}, + .pay = 1'000, + .flags = tfMPTCanClawback | kMptDexFlags}); + MPT const eth = mptEth; + + // btc pool dwarfs the eth pool, so a ~1/12th claw withdraws a + // non-zero btc amount while the eth counterpart rounds to zero. + AMM amm(env, carol, btc(3'000), eth(3)); + amm.deposit(dan, btc(3'000), eth(3)); + + [[maybe_unused]] auto const [poolBtcBefore, poolEthBefore, lptBefore] = amm.balances(); + BEAST_EXPECT(poolBtcBefore == btc(6'000)); + BEAST_EXPECT(poolEthBefore == eth(6)); + + auto const carolLpBefore = amm.getLPTokensBalance(carol.id()); + auto const danLpBefore = amm.getLPTokensBalance(dan.id()); + + env(amm::ammClawback(gw, carol, btc, eth, btc(500)), + Ter(features[fixCleanup3_4_0] ? TER{tecAMM_FAILED} : TER{tesSUCCESS})); + env.close(); + + [[maybe_unused]] auto const [poolBtcAfter, poolEthAfter, lptAfter] = amm.balances(); + auto const carolLpAfter = amm.getLPTokensBalance(carol.id()); + auto const danLpAfter = amm.getLPTokensBalance(dan.id()); + + if (features[fixCleanup3_4_0]) + { + // Post-fixCleanup3_4_0: clawback fails because the ETH (Asset2) + // balance would round to zero (guard fires via + // amount2Rounded == 0). All balances must remain untouched. + BEAST_EXPECT(poolBtcAfter == poolBtcBefore); + BEAST_EXPECT(poolEthAfter == poolEthBefore); + BEAST_EXPECT(carolLpAfter == carolLpBefore); + BEAST_EXPECT(danLpAfter == danLpBefore); + } + else + { + // Pre-fixCleanup3_4_0: the asymmetric round-off goes through. + // btc is clawed (non-zero) but eth rounds to zero, so the eth + // pool is untouched while carol's LP is burned. This asymmetry + // proves amount2Rounded == 0 is the trigger. + BEAST_EXPECT(poolBtcAfter < poolBtcBefore); + BEAST_EXPECT(poolEthAfter == poolEthBefore); + BEAST_EXPECT(carolLpAfter < carolLpBefore); + BEAST_EXPECT(danLpAfter == danLpBefore); + } + } + } + void testAMMClawbackAll(FeatureBitset features) { @@ -543,7 +686,6 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite // gw clawback all BTC from alice amm.deposit(bob, btc(1'000'000000), usd(2000)); - env.close(); BEAST_EXPECT(amm.expectBalances(btc(3'000'000000), usd(3000), IOUAmount(3000000))); auto aliceBTC = env.balance(alice, btc); @@ -921,7 +1063,6 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite BEAST_EXPECT(amm.expectBalances(btc(2'000'000000), usd(8'000), IOUAmount(4'000'000))); amm.deposit(bob, btc(1'000'000000), usd(4'000)); - env.close(); BEAST_EXPECT(amm.expectBalances(btc(3'000'000000), usd(12'000), IOUAmount(6'000'000))); auto aliceBTC = env.balance(alice, btc); @@ -1361,7 +1502,6 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite env.close(); BEAST_EXPECT(amm.expectBalances(XRP(100), btc(400), IOUAmount(200000))); amm.deposit(alice, btc(400)); - env.close(); BEAST_EXPECT(amm.expectBalances(XRP(100), btc(800), IOUAmount{282842'712474619, -9})); auto aliceBTC = env.balance(alice, MPT(btc)); @@ -1407,7 +1547,6 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite env.close(); BEAST_EXPECT(amm.expectBalances(usd(100), btc(400), IOUAmount(200))); amm.deposit(alice, btc(400)); - env.close(); BEAST_EXPECT(amm.expectBalances(usd(100), btc(800), IOUAmount{282'842712474619, -12})); auto aliceBTC = env.balance(alice, MPT(btc)); @@ -1462,7 +1601,6 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite env.close(); BEAST_EXPECT(amm.expectBalances(usd(100), btc(400), IOUAmount(200))); amm.deposit(alice, btc(400)); - env.close(); BEAST_EXPECT(amm.expectBalances(usd(100), btc(800), IOUAmount{282'842712474619, -12})); auto aliceBTC = env.balance(alice, MPT(btc)); @@ -1669,7 +1807,7 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite env(amm::ammClawback(gw, alice, btc, usd, std::nullopt), Ter(tecNO_PERMISSION)); // Although USD is clawable with asfAllowTrustLineClawback. - // When tfClawTwoAssets is set, we will claw Asser2 as well. + // When tfClawTwoAssets is set, we will claw Asset2 as well. // But Asset2 is not clawable. tfMPTCanClawback was not set for BTC. env(amm::ammClawback(gw, alice, usd, btc, std::nullopt), Txflags(tfClawTwoAssets), @@ -1819,6 +1957,9 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite testInvalidRequest(all); testFeatureDisabled(all); testAMMClawbackAmount(all); + testAMMClawbackAmount(all - fixCleanup3_4_0); + testAMMClawbackAmountRoundsToZero(all); + testAMMClawbackAmountRoundsToZero(all - fixCleanup3_4_0); testAMMClawbackAll(all); testAMMClawbackAmountSameIssuer(all); testAMMClawbackAllSameIssuer(all); From 639943123cced8fe0aff2b6477527558d31b5f6b Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Tue, 11 Aug 2026 00:49:02 +0000 Subject: [PATCH 5/9] fix: Validate buy/sell flag in nft RPC input (#7725) --- src/test/app/NFToken_test.cpp | 82 +++++++++++++++++++ .../rpc/handlers/orderbook/NFTOffersHelpers.h | 11 +++ 2 files changed, 93 insertions(+) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index a7437eea7f..7fcd34640b 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -4790,6 +4790,87 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite checkOffers("nft_buy_offers", 501, 2, __LINE__); } + void + testNftXxxOffersMarkerWrongSide(FeatureBitset features) + { + // A pagination marker passed to nft_buy_offers / nft_sell_offers must + // reference an offer on the same side (buy vs. sell) as the directory + // being enumerated. A wrong-side marker is rejected with invalidParams. + // + // Note: the pre-fix code also returned invalidParams for a wrong-side + // marker, but only after scanning the entire target directory (an + // O(directory size) walk usable to burn CPU). The fix short-circuits + // that scan. The scan-avoidance is not observable from the RPC + // response, so this test locks the rejection contract (wrong-side -> + // error, same-side -> success) rather than the performance property. + testcase("nft_buy_offers and nft_sell_offers wrong-side marker"); + + using namespace test::jtx; + + Env env{*this, features}; + + Account const issuer{"issuer"}; + Account const buyer{"buyer"}; + + env.fund(XRP(10000), issuer, buyer); + env.close(); + + // Mint a transferable NFT. + uint256 const nftID{token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), Txflags(tfTransferable)); + env.close(); + + // Create one sell offer (from the issuer, who owns the NFT) and one + // buy offer (from the buyer) for the same NFT. + env(token::createOffer(issuer, nftID, XRP(100)), Txflags(tfSellNFToken)); + env(token::createOffer(buyer, nftID, XRP(50)), token::Owner(issuer)); + env.close(); + + // Grab the index of the single offer on each side from the RPC + // response so we can use it as a marker. + auto firstOfferIndex = [this, &env, &nftID](char const* request) { + json::Value params; + params[jss::nft_id] = to_string(nftID); + json::Value const result = env.rpc("json", request, to_string(params))[jss::result]; + BEAST_EXPECT(result.isMember(jss::offers) && result[jss::offers].size() == 1); + return result[jss::offers][0u][jss::nft_offer_index].asString(); + }; + + std::string const sellOfferIndex = firstOfferIndex("nft_sell_offers"); + std::string const buyOfferIndex = firstOfferIndex("nft_buy_offers"); + + auto queryWithMarker = [&env, &nftID](char const* request, std::string const& marker) { + json::Value params; + params[jss::nft_id] = to_string(nftID); + params[jss::marker] = marker; + return env.rpc("json", request, to_string(params))[jss::result]; + }; + + // A marker referencing an offer on the wrong side is rejected with + // invalidParams. + { + // Sell-side marker passed to nft_buy_offers. + json::Value const result = queryWithMarker("nft_buy_offers", sellOfferIndex); + BEAST_EXPECT(result[jss::error].asString() == "invalidParams"); + } + { + // Buy-side marker passed to nft_sell_offers. + json::Value const result = queryWithMarker("nft_sell_offers", buyOfferIndex); + BEAST_EXPECT(result[jss::error].asString() == "invalidParams"); + } + + // A same-side marker is still accepted. With a single offer on each + // side, resuming after it simply yields no further offers. + { + json::Value const result = queryWithMarker("nft_buy_offers", buyOfferIndex); + BEAST_EXPECT(!result.isMember(jss::error)); + } + { + json::Value const result = queryWithMarker("nft_sell_offers", sellOfferIndex); + BEAST_EXPECT(!result.isMember(jss::error)); + } + } + void testNFTokenNegOffer(FeatureBitset features) { @@ -7305,6 +7386,7 @@ protected: testNFTokenWithTickets(features); testNFTokenDeleteAccount(features); testNftXxxOffers(features); + testNftXxxOffersMarkerWrongSide(features); testNFTokenNegOffer(features); testIOUWithTransferFee(features); testBrokeredSaleToSelf(features); diff --git a/src/xrpld/rpc/handlers/orderbook/NFTOffersHelpers.h b/src/xrpld/rpc/handlers/orderbook/NFTOffersHelpers.h index e03830ae0d..21bf3f8be8 100644 --- a/src/xrpld/rpc/handlers/orderbook/NFTOffersHelpers.h +++ b/src/xrpld/rpc/handlers/orderbook/NFTOffersHelpers.h @@ -93,6 +93,17 @@ enumerateNFTOffers(rpc::JsonContext& context, uint256 const& nftId, Keylet const if (!sle || nftId != sle->getFieldH256(sfNFTokenID)) return rpcError(RpcInvalidParams); + // Reject a marker that references an offer on the opposite side + // (buy vs. sell) of the directory being enumerated. Without this + // check the marker's node hint points into the other directory, so + // forEachItemAfter never finds `startAfter` and instead scans every + // page of `directory` before returning invalidParams -- turning an + // O(1) rejection into an O(directory size) walk. + auto const offerDir = + sle->isFlag(lsfSellNFToken) ? keylet::nftSells(nftId) : keylet::nftBuys(nftId); + if (directory.key != offerDir.key) + return rpcError(RpcInvalidParams); + startHint = sle->getFieldU64(sfNFTokenOfferNode); appendNftOfferJson(context.app, sle, jsonOffers); offers.reserve(reserve); From 0a572833eae96c28a30e7f5dcc143fb26cfa33bd Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 11 Aug 2026 12:38:40 +0000 Subject: [PATCH 6/9] chore: Gtest migration followups second pass (#7888) --- .cspell.config.yaml | 1 + cmake/XrplCov.cmake | 1 + include/xrpl/basics/Buffer.h | 14 + src/benchmarks/libxrpl/nodestore/Backend.cpp | 19 +- .../libxrpl/nodestore/NodeStoreBench.h | 5 +- src/tests/libxrpl/basics/Buffer.cpp | 517 +++++++++++------- src/tests/libxrpl/basics/IntrusiveShared.cpp | 11 +- src/tests/libxrpl/basics/base_uint.cpp | 227 ++++---- src/tests/libxrpl/shamap/SHAMap.cpp | 3 +- 9 files changed, 446 insertions(+), 352 deletions(-) diff --git a/.cspell.config.yaml b/.cspell.config.yaml index 21b0145f43..bb763e9935 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -365,6 +365,7 @@ words: - xchain - ximinez - XMACRO + - xored - xrpkuwait - xrpl - xrpld diff --git a/cmake/XrplCov.cmake b/cmake/XrplCov.cmake index 86ba534a88..05d9ed3806 100644 --- a/cmake/XrplCov.cmake +++ b/cmake/XrplCov.cmake @@ -44,6 +44,7 @@ setup_target_for_coverage_gcovr( EXCLUDE "src/test" "src/tests" + "src/benchmarks" "include/xrpl/beast/test" "include/xrpl/beast/unit_test" "${CMAKE_BINARY_DIR}/pb-xrpl.libpb" diff --git a/include/xrpl/basics/Buffer.h b/include/xrpl/basics/Buffer.h index 705a5ef51a..00a6b7ecf9 100644 --- a/include/xrpl/basics/Buffer.h +++ b/include/xrpl/basics/Buffer.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -156,6 +157,19 @@ public: } /** @} */ + /** + * Set every byte in the buffer to the given value. + * + * The size is unchanged, and this is a no-op on an empty buffer. + * + * @param value the byte to write to every position. + */ + void + fill(std::uint8_t value) noexcept + { + std::fill_n(p_.get(), size_, value); + } + /** * Reset the buffer. * All memory is deallocated. The resulting size is 0. diff --git a/src/benchmarks/libxrpl/nodestore/Backend.cpp b/src/benchmarks/libxrpl/nodestore/Backend.cpp index cd3e15bd65..9d5937f869 100644 --- a/src/benchmarks/libxrpl/nodestore/Backend.cpp +++ b/src/benchmarks/libxrpl/nodestore/Backend.cpp @@ -41,10 +41,11 @@ struct RunState release() { harness.reset(); - Batch{}.swap(present); - Batch{}.swap(recent); - std::vector{}.swap(missing); - std::vector{}.swap(shuffle); + present = Batch{}; + recent = Batch{}; + missing = std::vector{}; + shuffle = std::vector{}; + avgPayload = 0; } }; @@ -239,9 +240,13 @@ registerWorkload(BackendConfig const& bc, Workload const& w) if (!w.pinToPool) { auto rs = std::make_shared(); - auto* b = benchmark::RegisterBenchmark(name, makeRunner(w, cfg, rs)); - b->RangeMultiplier(10)->Range(kPoolSizes.front(), kPoolSizes.back()); - b->Threads(1)->Threads(4)->Threads(8)->UseRealTime(); + benchmark::RegisterBenchmark(name, makeRunner(w, cfg, rs)) + ->RangeMultiplier(10) + ->Range(kPoolSizes.front(), kPoolSizes.back()) + ->Threads(1) + ->Threads(4) + ->Threads(8) + ->UseRealTime(); return; } diff --git a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h index debdc5d47a..6122dd2535 100644 --- a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h +++ b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h @@ -297,12 +297,11 @@ struct BackendConfig inline std::vector const& backendConfigs() { + // Use factory settings for each DB static std::vector const kConfigs = { {.name = "nudb", .config = "type=nudb"}, #if XRPL_ROCKSDB_AVAILABLE - {.name = "rocksdb", - .config = "type=rocksdb,open_files=2000,filter_bits=12,cache_mb=256," - "file_size_mb=8,file_size_mult=2"}, + {.name = "rocksdb", .config = "type=rocksdb"}, #endif }; return kConfigs; diff --git a/src/tests/libxrpl/basics/Buffer.cpp b/src/tests/libxrpl/basics/Buffer.cpp index 9cdf610282..a3f78e8bcf 100644 --- a/src/tests/libxrpl/basics/Buffer.cpp +++ b/src/tests/libxrpl/basics/Buffer.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -12,8 +13,18 @@ namespace xrpl::test { +static_assert(std::is_nothrow_move_constructible_v); +static_assert(std::is_nothrow_move_assignable_v); + struct BufferTest : public ::testing::Test { + static constexpr auto kRandomData = std::to_array( + {0xa8, 0xa1, 0x38, 0x45, 0x23, 0xec, 0xe4, 0x23, 0x71, 0x6d, 0x2a, + 0x18, 0xb4, 0x70, 0xcb, 0xf5, 0xac, 0x2d, 0x89, 0x4d, 0x19, 0x9c, + 0xf0, 0x2c, 0x15, 0xd1, 0xf9, 0x9b, 0x66, 0xd2, 0x30, 0xd3}); + + static constexpr std::size_t kHalf = kRandomData.size() / 2; + static bool sane(Buffer const& b) { @@ -22,239 +33,321 @@ struct BufferTest : public ::testing::Test return b.data() != nullptr; } + + /** + * Check the state Buffer documents for a moved-from buffer: "the other buffer is reset", i.e. + * empty and sane. + * + * Zeroing the size is not incidental tidiness. Moving the member unique_ptr nulls the data + * pointer whether Buffer wants it or not, so a moved-from buffer that kept its old size would + * lie about itself everywhere: alloc() would take its `n == size_` early-out and hand back a + * null pointer while still reporting the old size, fill() would run std::fill_n over a null + * pointer, and the Slice conversion would publish {nullptr, oldSize} to callers. A moved-from + * Buffer has to be a usable empty Buffer rather than a landmine, which is why the tests below + * assert this state instead of treating a moved-from buffer as untouchable. + */ + static void + checkEmptyAfterMove(Buffer const& buf) + { + EXPECT_TRUE(sane(buf)); + EXPECT_TRUE(buf.empty()); + } + + Buffer const emptyBuffer; + Buffer const firstHalf{kRandomData.data(), kHalf}; + Buffer const secondHalf{kRandomData.data() + kHalf, kHalf}; + Buffer const whole{kRandomData.data(), kRandomData.size()}; }; -TEST_F(BufferTest, buffer) +TEST_F(BufferTest, default_constructed_is_empty) { - std::uint8_t const data[] = {0xa8, 0xa1, 0x38, 0x45, 0x23, 0xec, 0xe4, 0x23, 0x71, 0x6d, 0x2a, - 0x18, 0xb4, 0x70, 0xcb, 0xf5, 0xac, 0x2d, 0x89, 0x4d, 0x19, 0x9c, - 0xf0, 0x2c, 0x15, 0xd1, 0xf9, 0x9b, 0x66, 0xd2, 0x30, 0xd3}; + Buffer const b; - Buffer const b0; - EXPECT_TRUE(sane(b0)); - EXPECT_TRUE(b0.empty()); + EXPECT_TRUE(sane(b)); + EXPECT_TRUE(b.empty()); + EXPECT_EQ(b.data(), nullptr); +} - Buffer b1{0}; - EXPECT_TRUE(sane(b1)); - EXPECT_TRUE(b1.empty()); - std::memcpy(b1.alloc(16), data, 16); - EXPECT_TRUE(sane(b1)); - EXPECT_FALSE(b1.empty()); - EXPECT_EQ(b1.size(), 16); +TEST_F(BufferTest, zero_sized_construction_is_empty) +{ + Buffer const b{0}; - Buffer b2{b1.size()}; - EXPECT_TRUE(sane(b2)); - EXPECT_FALSE(b2.empty()); - EXPECT_EQ(b2.size(), b1.size()); - std::memcpy(b2.data(), data + 16, 16); + EXPECT_TRUE(sane(b)); + EXPECT_TRUE(b.empty()); +} - Buffer b3{data, sizeof(data)}; - EXPECT_TRUE(sane(b3)); - EXPECT_FALSE(b3.empty()); - EXPECT_EQ(b3.size(), sizeof(data)); - EXPECT_EQ(std::memcmp(b3.data(), data, b3.size()), 0); +TEST_F(BufferTest, alloc_grows_an_empty_buffer) +{ + Buffer b{0}; + std::memcpy(b.alloc(kHalf), kRandomData.data(), kHalf); - // Check equality and inequality comparisons. - // For code readability, we want to use general - // EXPECT_TRUE instead of specific EXPECT_EQ etc. - EXPECT_TRUE(b0 == b0); - EXPECT_TRUE(b0 != b1); - EXPECT_TRUE(b1 == b1); - EXPECT_TRUE(b1 != b2); - EXPECT_TRUE(b2 != b3); + EXPECT_TRUE(sane(b)); + EXPECT_FALSE(b.empty()); + EXPECT_EQ(b.size(), kHalf); + EXPECT_EQ(b, firstHalf); +} - // Check copy constructors and copy assignments: - { - Buffer x{b0}; - EXPECT_EQ(x, b0); - EXPECT_TRUE(sane(x)); - Buffer y{b1}; - EXPECT_EQ(y, b1); - EXPECT_TRUE(sane(y)); - x = b2; - EXPECT_EQ(x, b2); - EXPECT_TRUE(sane(x)); - x = y; - EXPECT_EQ(x, y); - EXPECT_TRUE(sane(x)); - y = b3; - EXPECT_EQ(y, b3); - EXPECT_TRUE(sane(y)); - x = b0; - EXPECT_EQ(x, b0); - EXPECT_TRUE(sane(x)); +TEST_F(BufferTest, sized_construction_reserves_without_filling) +{ + Buffer b{kHalf}; + + EXPECT_TRUE(sane(b)); + EXPECT_FALSE(b.empty()); + EXPECT_EQ(b.size(), kHalf); + + std::memcpy(b.data(), kRandomData.data() + kHalf, kHalf); + EXPECT_EQ(b, secondHalf); +} + +TEST_F(BufferTest, construction_copies_raw_memory) +{ + Buffer const b{kRandomData.data(), kRandomData.size()}; + + EXPECT_TRUE(sane(b)); + EXPECT_FALSE(b.empty()); + EXPECT_EQ(b.size(), kRandomData.size()); + EXPECT_EQ(std::memcmp(b.data(), kRandomData.data(), b.size()), 0); +} + +TEST_F(BufferTest, equality_compares_contents) +{ + // Uses EXPECT_TRUE rather than EXPECT_EQ/EXPECT_NE because the operators are what is under test + // here. + EXPECT_TRUE(emptyBuffer == emptyBuffer); + EXPECT_TRUE(firstHalf == firstHalf); + + EXPECT_TRUE(emptyBuffer != firstHalf); + EXPECT_TRUE(firstHalf != secondHalf); + EXPECT_TRUE(secondHalf != whole); +} + +TEST_F(BufferTest, copy_construction) +{ + Buffer const fromEmpty{emptyBuffer}; + EXPECT_TRUE(sane(fromEmpty)); + EXPECT_EQ(fromEmpty, emptyBuffer); + + Buffer const fromNonEmpty{firstHalf}; + EXPECT_TRUE(sane(fromNonEmpty)); + EXPECT_EQ(fromNonEmpty, firstHalf); +} + +TEST_F(BufferTest, copy_assignment) +{ + Buffer b{emptyBuffer}; + + // empty <- non-empty + b = secondHalf; + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, secondHalf); + + // non-empty <- non-empty of a different size + b = whole; + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, whole); + + // non-empty <- empty + b = emptyBuffer; + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, emptyBuffer); +} + +TEST_F(BufferTest, self_assignment_preserves_contents) +{ #ifdef __clang__ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wself-assign-overloaded" #endif - x = x; - EXPECT_EQ(x, b0); - EXPECT_TRUE(sane(x)); - y = y; - EXPECT_EQ(y, b3); - EXPECT_TRUE(sane(y)); + Buffer emptyCopy{emptyBuffer}; + emptyCopy = emptyCopy; + EXPECT_TRUE(sane(emptyCopy)); + EXPECT_EQ(emptyCopy, emptyBuffer); + + Buffer wholeCopy{whole}; + wholeCopy = wholeCopy; + EXPECT_TRUE(sane(wholeCopy)); + EXPECT_EQ(wholeCopy, whole); #ifdef __clang__ #pragma clang diagnostic pop #endif - } +} - // Check move constructor & move assignments: +TEST_F(BufferTest, move_construct_from_empty) +{ + Buffer source; + Buffer const moved{std::move(source)}; + + checkEmptyAfterMove(source); // NOLINT(bugprone-use-after-move) + EXPECT_TRUE(sane(moved)); + EXPECT_TRUE(moved.empty()); +} + +TEST_F(BufferTest, move_construct_from_non_empty) +{ + Buffer source{firstHalf}; + Buffer const moved{std::move(source)}; + + checkEmptyAfterMove(source); // NOLINT(bugprone-use-after-move) + EXPECT_TRUE(sane(moved)); + EXPECT_EQ(moved, firstHalf); +} + +TEST_F(BufferTest, move_assign_empty_to_empty) +{ + Buffer target; + Buffer source; + + target = std::move(source); + + EXPECT_TRUE(sane(target)); + EXPECT_TRUE(target.empty()); + checkEmptyAfterMove(source); // NOLINT(bugprone-use-after-move) +} + +TEST_F(BufferTest, move_assign_non_empty_to_empty) +{ + Buffer target; + Buffer source{firstHalf}; + + target = std::move(source); + + EXPECT_TRUE(sane(target)); + EXPECT_EQ(target, firstHalf); + checkEmptyAfterMove(source); // NOLINT(bugprone-use-after-move) +} + +TEST_F(BufferTest, move_assign_empty_to_non_empty) +{ + Buffer target{firstHalf}; + Buffer source; + + target = std::move(source); + + EXPECT_TRUE(sane(target)); + EXPECT_TRUE(target.empty()); + checkEmptyAfterMove(source); // NOLINT(bugprone-use-after-move) +} + +TEST_F(BufferTest, move_assign_non_empty_to_non_empty) +{ + Buffer target{firstHalf}; + Buffer sameSize{secondHalf}; + Buffer largerSize{whole}; + + target = std::move(sameSize); + EXPECT_TRUE(sane(target)); + EXPECT_EQ(target, secondHalf); + checkEmptyAfterMove(sameSize); // NOLINT(bugprone-use-after-move) + + target = std::move(largerSize); + EXPECT_TRUE(sane(target)); + EXPECT_EQ(target, whole); + checkEmptyAfterMove(largerSize); // NOLINT(bugprone-use-after-move) +} + +TEST_F(BufferTest, construction_from_slice) +{ + Buffer const fromEmpty{static_cast(emptyBuffer)}; + EXPECT_TRUE(sane(fromEmpty)); + EXPECT_EQ(fromEmpty, emptyBuffer); + + Buffer const fromNonEmpty{static_cast(whole)}; + EXPECT_TRUE(sane(fromNonEmpty)); + EXPECT_EQ(fromNonEmpty, whole); +} + +TEST_F(BufferTest, assignment_from_slice) +{ + Buffer b; + + // empty <- empty slice + b = static_cast(emptyBuffer); + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, emptyBuffer); + + // empty <- non-empty slice + b = static_cast(firstHalf); + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, firstHalf); + + // non-empty <- non-empty slice + b = static_cast(secondHalf); + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, secondHalf); + + // non-empty <- empty slice + b = static_cast(emptyBuffer); + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b, emptyBuffer); +} + +TEST_F(BufferTest, resize_allocates_and_clear_releases) +{ + auto check = [](Buffer const& original, std::size_t size) { + SCOPED_TRACE(::testing::Message() << "size: " << size); + + Buffer b{original}; + + // Resizing to zero is equivalent to clearing. + b(size); + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b.size(), size); + EXPECT_EQ(b.data() == nullptr, size == 0); + + b(size + 1); + EXPECT_TRUE(sane(b)); + EXPECT_EQ(b.size(), size + 1); + EXPECT_NE(b.data(), nullptr); + + b.clear(); + EXPECT_TRUE(sane(b)); + EXPECT_TRUE(b.empty()); + EXPECT_EQ(b.data(), nullptr); + + // clear() is idempotent. + b.clear(); + EXPECT_TRUE(sane(b)); + EXPECT_TRUE(b.empty()); + EXPECT_EQ(b.data(), nullptr); + }; + + for (auto size = 0uz; size < kHalf; ++size) { - static_assert(std::is_nothrow_move_constructible_v); - static_assert(std::is_nothrow_move_assignable_v); - - { // Move-construct from empty buf - Buffer x; - Buffer const y{std::move(x)}; - EXPECT_TRUE(sane(x)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(x.empty()); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(sane(y)); - EXPECT_TRUE(y.empty()); - EXPECT_EQ(x, y); // NOLINT(bugprone-use-after-move) - } - - { // Move-construct from non-empty buf - Buffer x{b1}; - Buffer const y{std::move(x)}; - EXPECT_TRUE(sane(x)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(x.empty()); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(sane(y)); - EXPECT_EQ(y, b1); - } - - { // Move assign empty buf to empty buf - Buffer x; - Buffer y; - - x = std::move(y); - EXPECT_TRUE(sane(x)); - EXPECT_TRUE(x.empty()); - EXPECT_TRUE(sane(y)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(y.empty()); // NOLINT(bugprone-use-after-move) - } - - { // Move assign non-empty buf to empty buf - Buffer x; - Buffer y{b1}; - - x = std::move(y); - EXPECT_TRUE(sane(x)); - EXPECT_EQ(x, b1); - EXPECT_TRUE(sane(y)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(y.empty()); // NOLINT(bugprone-use-after-move) - } - - { // Move assign empty buf to non-empty buf - Buffer x{b1}; - Buffer y; - - x = std::move(y); - EXPECT_TRUE(sane(x)); - EXPECT_TRUE(x.empty()); - EXPECT_TRUE(sane(y)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(y.empty()); // NOLINT(bugprone-use-after-move) - } - - { // Move assign non-empty buf to non-empty buf - Buffer x{b1}; - Buffer y{b2}; - Buffer z{b3}; - - x = std::move(y); - EXPECT_TRUE(sane(x)); - EXPECT_FALSE(x.empty()); - EXPECT_TRUE(sane(y)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(y.empty()); // NOLINT(bugprone-use-after-move) - - x = std::move(z); - EXPECT_TRUE(sane(x)); - EXPECT_FALSE(x.empty()); - EXPECT_TRUE(sane(z)); // NOLINT(bugprone-use-after-move) - EXPECT_TRUE(z.empty()); // NOLINT(bugprone-use-after-move) - } - } - - { - Buffer w{static_cast(b0)}; - EXPECT_TRUE(sane(w)); - EXPECT_EQ(w, b0); - - Buffer x{static_cast(b1)}; - EXPECT_TRUE(sane(x)); - EXPECT_EQ(x, b1); - - Buffer y{static_cast(b2)}; - EXPECT_TRUE(sane(y)); - EXPECT_EQ(y, b2); - - Buffer z{static_cast(b3)}; - EXPECT_TRUE(sane(z)); - EXPECT_EQ(z, b3); - - // Assign empty slice to empty buffer - w = static_cast(b0); - EXPECT_TRUE(sane(w)); - EXPECT_EQ(w, b0); - - // Assign non-empty slice to empty buffer - w = static_cast(b1); - EXPECT_TRUE(sane(w)); - EXPECT_EQ(w, b1); - - // Assign non-empty slice to non-empty buffer - x = static_cast(b2); - EXPECT_TRUE(sane(x)); - EXPECT_EQ(x, b2); - - // Assign non-empty slice to non-empty buffer - y = static_cast(z); - EXPECT_TRUE(sane(y)); - EXPECT_EQ(y, z); - - // Assign empty slice to non-empty buffer: - z = static_cast(b0); - EXPECT_TRUE(sane(z)); - EXPECT_EQ(z, b0); - } - - { - auto test = [](Buffer const& b, std::size_t i) { - Buffer x{b}; - - // Try to allocate some number of bytes, possibly - // zero (which means clear) and sanity check - x(i); - EXPECT_TRUE(sane(x)); - EXPECT_EQ(x.size(), i); - EXPECT_EQ((x.data() == nullptr), (i == 0)); - - // Try to allocate some more data (always non-zero) - x(i + 1); - EXPECT_TRUE(sane(x)); - EXPECT_EQ(x.size(), i + 1); - EXPECT_NE(x.data(), nullptr); - - // Try to clear: - x.clear(); - EXPECT_TRUE(sane(x)); - EXPECT_TRUE(x.empty()); - EXPECT_EQ(x.data(), nullptr); - - // Try to clear again: - x.clear(); - EXPECT_TRUE(sane(x)); - EXPECT_TRUE(x.empty()); - EXPECT_EQ(x.data(), nullptr); - }; - - for (std::size_t i = 0; i < 16; ++i) - { - test(b0, i); - test(b1, i); - } + check(emptyBuffer, size); + check(firstHalf, size); } } +TEST_F(BufferTest, fill_sets_every_byte) +{ + Buffer b{4}; + b.fill(0xab); + + EXPECT_EQ(b.size(), 4); + for (auto const byte : Slice{b}) + EXPECT_EQ(byte, 0xab); +} + +TEST_F(BufferTest, fill_overwrites_and_keeps_size) +{ + Buffer b{4}; + b.fill(0xab); + b.fill(0x00); + + EXPECT_EQ(b.size(), 4); + for (auto const byte : Slice{b}) + EXPECT_EQ(byte, 0x00); +} + +TEST_F(BufferTest, fill_on_empty_buffer_is_a_noop) +{ + Buffer empty; + empty.fill(0xff); + + EXPECT_TRUE(empty.empty()); + EXPECT_EQ(empty.data(), nullptr); +} + } // namespace xrpl::test diff --git a/src/tests/libxrpl/basics/IntrusiveShared.cpp b/src/tests/libxrpl/basics/IntrusiveShared.cpp index b9f8930b7b..c6c9fcfef0 100644 --- a/src/tests/libxrpl/basics/IntrusiveShared.cpp +++ b/src/tests/libxrpl/basics/IntrusiveShared.cpp @@ -92,6 +92,7 @@ public: static constexpr std::size_t kMaxStates = 128; static std::array, kMaxStates> state; static std::atomic nextId; + static TrackedState getState(std::size_t id) { @@ -100,13 +101,12 @@ public: return state[id].load(std::memory_order_acquire); } + static void resetStates(bool resetCallback) { for (std::size_t i = 0; i < kMaxStates; ++i) - { state[i].store(TrackedState::Uninitialized, std::memory_order_release); - } nextId.store(0, std::memory_order_release); if (resetCallback) TIBase::tracingCallback = [](TrackedState, std::optional) {}; @@ -120,6 +120,7 @@ public: { TIBase::resetStates(resetCallback); } + ~ResetStatesGuard() { TIBase::resetStates(resetCallback); @@ -130,6 +131,7 @@ public: { state[id].store(TrackedState::Alive, std::memory_order_relaxed); } + ~TIBase() override { using enum TrackedState; @@ -218,9 +220,7 @@ TEST(IntrusiveSharedTest, basics) EXPECT_EQ(TIBase::getState(id), Alive); EXPECT_EQ(b->useCount(), 1); for (auto i = 0uz; i < 10; ++i) - { strong.push_back(b); - } b.reset(); EXPECT_EQ(TIBase::getState(id), Alive); strong.resize(strong.size() - 1); @@ -244,8 +244,7 @@ TEST(IntrusiveSharedTest, basics) EXPECT_EQ(TIBase::getState(id), PartiallyDeleted); while (!weak.empty()) { - weak.resize(weak.size() - 1); - if (!weak.empty()) + if (weak.resize(weak.size() - 1); !weak.empty()) { EXPECT_EQ(TIBase::getState(id), PartiallyDeleted); } diff --git a/src/tests/libxrpl/basics/base_uint.cpp b/src/tests/libxrpl/basics/base_uint.cpp index 10795f4563..969705b5b7 100644 --- a/src/tests/libxrpl/basics/base_uint.cpp +++ b/src/tests/libxrpl/basics/base_uint.cpp @@ -6,6 +6,7 @@ #include +#include #include #include @@ -205,125 +206,119 @@ TEST_F(BaseUintTest, base_uint) Blob const raw{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}; EXPECT_EQ(BaseUInt96::kBytes, raw.size()); - BaseUInt96 u = BaseUInt96::fromRaw(raw); - uset.insert(u); - EXPECT_EQ(raw.size(), u.size()); - EXPECT_EQ(to_string(u), "0102030405060708090A0B0C"); - EXPECT_EQ(toShortString(u), "01020304..."); - EXPECT_EQ(*u.data(), 1); - EXPECT_EQ(u.signum(), 1); - EXPECT_FALSE(!u); - EXPECT_FALSE(u.isZero()); - EXPECT_TRUE(u.isNonZero()); - unsigned char t = 0; - for (auto& d : u) - { - EXPECT_EQ(d, ++t); - } + BaseUInt96 ascending = BaseUInt96::fromRaw(raw); + uset.insert(ascending); + EXPECT_EQ(raw.size(), ascending.size()); + EXPECT_EQ(to_string(ascending), "0102030405060708090A0B0C"); + EXPECT_EQ(toShortString(ascending), "01020304..."); + EXPECT_EQ(*ascending.data(), 1); + EXPECT_EQ(ascending.signum(), 1); + EXPECT_FALSE(!ascending); + EXPECT_FALSE(ascending.isZero()); + EXPECT_TRUE(ascending.isNonZero()); + unsigned char expectedByte = 0; + for (auto& byte : ascending) + EXPECT_EQ(byte, ++expectedByte); - // Test hash_append by "hashing" with a no-op hasher (h) + // Test hash_append by "hashing" with a no-op hasher (hasher) // and then extracting the bytes that were written during hashing - // back into another base_uint (w) for comparison with the original - Nonhash<96> h{}; - hash_append(h, u); - BaseUInt96 const w = - BaseUInt96::fromRaw(std::vector(h.data.begin(), h.data.end())); - EXPECT_EQ(w, u); + // back into another base_uint (rehashed) for comparison with the original + Nonhash<96> hasher{}; + hash_append(hasher, ascending); + BaseUInt96 const rehashed = + BaseUInt96::fromRaw(std::vector(hasher.data.begin(), hasher.data.end())); + EXPECT_EQ(rehashed, ascending); - BaseUInt96 v{~u}; - uset.insert(v); - EXPECT_EQ(to_string(v), "FEFDFCFBFAF9F8F7F6F5F4F3"); - EXPECT_EQ(toShortString(v), "FEFDFCFB..."); - EXPECT_EQ(*v.data(), 0xfe); - EXPECT_EQ(v.signum(), 1); - EXPECT_FALSE(!v); - EXPECT_FALSE(v.isZero()); - EXPECT_TRUE(v.isNonZero()); + BaseUInt96 complement{~ascending}; + uset.insert(complement); + EXPECT_EQ(to_string(complement), "FEFDFCFBFAF9F8F7F6F5F4F3"); + EXPECT_EQ(toShortString(complement), "FEFDFCFB..."); + EXPECT_EQ(*complement.data(), 0xfe); + EXPECT_EQ(complement.signum(), 1); + EXPECT_FALSE(!complement); + EXPECT_FALSE(complement.isZero()); + EXPECT_TRUE(complement.isNonZero()); - t = 0xff; - for (auto& d : v) - { - EXPECT_EQ(d, --t); - } + expectedByte = 0xff; + for (auto& byte : complement) + EXPECT_EQ(byte, --expectedByte); - EXPECT_LT(u, v); - EXPECT_GT(v, u); + EXPECT_LT(ascending, complement); + EXPECT_GT(complement, ascending); - v = u; - EXPECT_EQ(v, u); + complement = ascending; + EXPECT_EQ(complement, ascending); - BaseUInt96 z{beast::kZero}; - uset.insert(z); - EXPECT_EQ(to_string(z), "000000000000000000000000"); - EXPECT_EQ(toShortString(z), "00000000..."); - EXPECT_EQ(*z.data(), 0); - EXPECT_EQ(*z.begin(), 0); - EXPECT_EQ(*std::prev(z.end(), 1), 0); - EXPECT_EQ(z.signum(), 0); - EXPECT_TRUE(!z); - EXPECT_TRUE(z.isZero()); - EXPECT_FALSE(z.isNonZero()); - for (auto& d : z) - { - EXPECT_EQ(d, 0); - } + BaseUInt96 zero{beast::kZero}; + uset.insert(zero); + EXPECT_EQ(to_string(zero), "000000000000000000000000"); + EXPECT_EQ(toShortString(zero), "00000000..."); + EXPECT_EQ(*zero.data(), 0); + EXPECT_EQ(*zero.begin(), 0); + EXPECT_EQ(*std::prev(zero.end(), 1), 0); + EXPECT_EQ(zero.signum(), 0); + EXPECT_TRUE(!zero); + EXPECT_TRUE(zero.isZero()); + EXPECT_FALSE(zero.isNonZero()); + for (auto& byte : zero) + EXPECT_EQ(byte, 0); { // There are several ways to create a zero. beast::kZero is tested above. Test some // others. - BaseUInt96 const z1; - EXPECT_EQ(z1, z) << to_string(z1); + BaseUInt96 const defaultZero; + EXPECT_EQ(defaultZero, zero) << to_string(defaultZero); - BaseUInt96 const z2{}; - EXPECT_EQ(z2, z) << to_string(z2); + BaseUInt96 const bracedZero{}; + EXPECT_EQ(bracedZero, zero) << to_string(bracedZero); - BaseUInt96 const z3{0u}; - EXPECT_EQ(z3, z) << to_string(z3); + BaseUInt96 const zeroFromUInt{0u}; + EXPECT_EQ(zeroFromUInt, zero) << to_string(zeroFromUInt); } - BaseUInt96 n{z}; - n++; - EXPECT_EQ(n, BaseUInt96(1)); - n--; - EXPECT_EQ(n, beast::kZero); - EXPECT_EQ(n, z); - n--; - EXPECT_EQ(to_string(n), "FFFFFFFFFFFFFFFFFFFFFFFF"); - EXPECT_EQ(toShortString(n), "FFFFFFFF..."); - n = beast::kZero; - EXPECT_EQ(n, z); + BaseUInt96 counter{zero}; + counter++; + EXPECT_EQ(counter, BaseUInt96(1)); + counter--; + EXPECT_EQ(counter, beast::kZero); + EXPECT_EQ(counter, zero); + counter--; + EXPECT_EQ(to_string(counter), "FFFFFFFFFFFFFFFFFFFFFFFF"); + EXPECT_EQ(toShortString(counter), "FFFFFFFF..."); + counter = beast::kZero; + EXPECT_EQ(counter, zero); - BaseUInt96 zp1{z}; - zp1++; - BaseUInt96 zm1{z}; - zm1--; - BaseUInt96 const x{zm1 ^ zp1}; - uset.insert(x); - EXPECT_EQ(to_string(x), "FFFFFFFFFFFFFFFFFFFFFFFE") << to_string(x); - EXPECT_EQ(toShortString(x), "FFFFFFFF...") << toShortString(x); + BaseUInt96 zeroPlusOne{zero}; + zeroPlusOne++; + BaseUInt96 zeroMinusOne{zero}; + zeroMinusOne--; + BaseUInt96 const xored{zeroMinusOne ^ zeroPlusOne}; + uset.insert(xored); + EXPECT_EQ(to_string(xored), "FFFFFFFFFFFFFFFFFFFFFFFE") << to_string(xored); + EXPECT_EQ(toShortString(xored), "FFFFFFFF...") << toShortString(xored); EXPECT_EQ(uset.size(), 4); - BaseUInt96 tmp; - EXPECT_TRUE(tmp.parseHex(to_string(u))); - EXPECT_EQ(tmp, u); - tmp = z; + BaseUInt96 parsed; + EXPECT_TRUE(parsed.parseHex(to_string(ascending))); + EXPECT_EQ(parsed, ascending); + parsed = zero; // fails with extra char - EXPECT_FALSE(tmp.parseHex("A" + to_string(u))); - tmp = z; + EXPECT_FALSE(parsed.parseHex("A" + to_string(ascending))); + parsed = zero; // fails with extra char at end - EXPECT_FALSE(tmp.parseHex(to_string(u) + "A")); + EXPECT_FALSE(parsed.parseHex(to_string(ascending) + "A")); // fails with a non-hex character at some point in the string: - tmp = z; + parsed = zero; for (std::size_t i = 0; i != 24; ++i) { - std::string x = to_string(z); - x[i] = ('G' + (i % 10)); - EXPECT_FALSE(tmp.parseHex(x)); + std::string xored = to_string(zero); + xored[i] = ('G' + (i % 10)); + EXPECT_FALSE(parsed.parseHex(xored)); } // Walking 1s: @@ -332,8 +327,8 @@ TEST_F(BaseUintTest, base_uint) std::string s1 = "000000000000000000000000"; s1[i] = '1'; - EXPECT_TRUE(tmp.parseHex(s1)); - EXPECT_EQ(to_string(tmp), s1); + EXPECT_TRUE(parsed.parseHex(s1)); + EXPECT_EQ(to_string(parsed), s1); } // Walking 0s: @@ -342,8 +337,8 @@ TEST_F(BaseUintTest, base_uint) std::string s1 = "111111111111111111111111"; s1[i] = '0'; - EXPECT_TRUE(tmp.parseHex(s1)); - EXPECT_EQ(to_string(tmp), s1); + EXPECT_TRUE(parsed.parseHex(s1)); + EXPECT_EQ(to_string(parsed), s1); } // Constexpr constructors @@ -357,39 +352,27 @@ TEST_F(BaseUintTest, base_uint) // Using the constexpr constructor in a non-constexpr context // with an error in the parsing throws an exception. { - // Invalid length for string. - bool caught = false; - try - { - // Try to prevent constant evaluation. - std::vector str(23, '7'); + // Invalid length for string. The vector keeps this out of a constant + // expression, so the constructor throws instead of failing to compile. + auto tooShort = [] { + std::vector const str(23, '7'); std::string_view const sView(str.data(), str.size()); [[maybe_unused]] BaseUInt96 const t96(sView); - } - catch (std::invalid_argument const& e) - { - EXPECT_EQ(e.what(), std::string("invalid length for hex string")); - caught = true; - } - EXPECT_TRUE(caught); + }; + EXPECT_THAT( + tooShort, + ::testing::ThrowsMessage("invalid length for hex string")); } { // Invalid character in string. - bool caught = false; - try - { - // Try to prevent constant evaluation. + auto badCharacter = [] { std::vector str(23, '7'); str.push_back('G'); std::string_view const sView(str.data(), str.size()); [[maybe_unused]] BaseUInt96 const t96(sView); - } - catch (std::range_error const& e) - { - EXPECT_EQ(e.what(), std::string("invalid hex character")); - caught = true; - } - EXPECT_TRUE(caught); + }; + EXPECT_THAT( + badCharacter, ::testing::ThrowsMessage("invalid hex character")); } // Verify that constexpr base_uints interpret a string the same @@ -412,11 +395,11 @@ TEST_F(BaseUintTest, base_uint) "fFfFfFfFfFfFfFfFfFfFfFfF", }); - for (StrBaseUInt const& t : kTestCases) + for (StrBaseUInt const& expectedByte : kTestCases) { BaseUInt96 t96; - EXPECT_TRUE(t96.parseHex(t.str)); - EXPECT_EQ(t96, t.tst); + EXPECT_TRUE(t96.parseHex(expectedByte.str)); + EXPECT_EQ(t96, expectedByte.tst); } } } diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index e662e16be4..c84cdf504f 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -16,7 +16,6 @@ #include #include -#include #include #include #include @@ -113,7 +112,7 @@ protected: intToVuc(std::uint8_t v) { Buffer vuc{32}; - std::fill_n(vuc.data(), vuc.size(), v); + vuc.fill(v); return vuc; } }; From c74724a7197da3db16cc1c7274543c7ba4ce2dbc Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 11 Aug 2026 12:44:01 +0000 Subject: [PATCH 7/9] build: Reimagine linker warnings in different scenarios (#7974) --- cmake/XrplCompiler.cmake | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/cmake/XrplCompiler.cmake b/cmake/XrplCompiler.cmake index e262acf1c9..21566add01 100644 --- a/cmake/XrplCompiler.cmake +++ b/cmake/XrplCompiler.cmake @@ -188,6 +188,32 @@ else() endif() endif() +# Linker warnings are errors where we control the toolchain and the dependencies: CI and the Nix dev shell. +# On non-Nix macOS we suppress the deployment target warning: an old Conan profile may not pin os.version. +if(is_macos OR is_linux) + if(is_ci OR is_nix_compiler) + if(is_macos) + set(fatal_warnings_flag "-Wl,-fatal_warnings") + else() + set(fatal_warnings_flag "-Wl,--fatal-warnings") + endif() + message( + STATUS + "Treating all linker warnings as errors (${fatal_warnings_flag})" + ) + target_link_options(common INTERFACE "${fatal_warnings_flag}") + unset(fatal_warnings_flag) + elseif(is_macos) + set(silence_flag "-Wl,-deployment_target_mismatches,suppress") + message( + STATUS + "Silencing macOS deployment target mismatch warnings (${silence_flag})" + ) + target_link_options(common INTERFACE "${silence_flag}") + unset(silence_flag) + endif() +endif() + # Antithesis instrumentation will only be built and deployed using machines running Linux. if(voidstar) if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug") From d43e5acaa70f8d45a1691ab522a9c84c4fc95006 Mon Sep 17 00:00:00 2001 From: luisfernandomendozav <109832400+luisfernandomendozav@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:23:07 +0000 Subject: [PATCH 8/9] fix: Validate account/ident type in gateway_balances (#7655) --- API-CHANGELOG.md | 1 + src/test/rpc/GatewayBalances_test.cpp | 40 +++++++++++++++++++ .../rpc/handlers/account/GatewayBalances.cpp | 6 +++ 3 files changed, 47 insertions(+) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index bc3672588e..c853cfb07c 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -54,6 +54,7 @@ This section contains changes targeting a future version. - `submit`: The `fail_hard` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) - `subscribe`: The `taker` field in the `books` array now returns `actMalformed` instead of `badIssuer` if the value is not a valid account. [#6529](https://github.com/XRPLF/rippled/pull/6529) - Fixed a bug in `Forwarded` HTTP header parsing where the extracted IP address could be incorrect when no comma or semicolon delimiter follows the address. This could cause the server to misidentify a client's IP address when operating behind a reverse proxy. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `gateway_balances`: The `account` and `ident` fields now return an `invalidParams` error if the value is not a string, instead of an `internal` error. [#7655](https://github.com/XRPLF/rippled/pull/7655) - `account_lines`: The `peer` field now returns an error if the value is not a string. [#7728](https://github.com/XRPLF/rippled/pull/7728) ## XRP Ledger server version 3.1.0 diff --git a/src/test/rpc/GatewayBalances_test.cpp b/src/test/rpc/GatewayBalances_test.cpp index 106b9b5f1a..91d9126f61 100644 --- a/src/test/rpc/GatewayBalances_test.cpp +++ b/src/test/rpc/GatewayBalances_test.cpp @@ -176,6 +176,45 @@ public: }); } + void + testGWBInvalidAccount(FeatureBitset features) + { + testcase("Gateway Balances with non-string account/ident"); + using namespace std::chrono_literals; + using namespace jtx; + Env env(*this, features); + + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env.close(); + + auto wsc = makeWSClient(env.app().config()); + + // A non-string "account" must be rejected cleanly with invalidParams + // rather than throwing a Json::LogicError that surfaces as internal. + json::Value qry; + qry[jss::account] = 42; + qry[jss::hotwallet] = alice.human(); + + forAllApiVersions([&, this](unsigned apiVersion) { + qry[jss::api_version] = apiVersion; + auto jv = wsc->invoke("gateway_balances", qry); + expect(jv[jss::status] == "error"); + BEAST_EXPECT(jv[jss::result][jss::error] == "invalidParams"); + }); + + // The same applies to a non-string "ident". + json::Value qry2; + qry2[jss::ident] = 42; + + forAllApiVersions([&, this](unsigned apiVersion) { + qry2[jss::api_version] = apiVersion; + auto jv = wsc->invoke("gateway_balances", qry2); + expect(jv[jss::status] == "error"); + BEAST_EXPECT(jv[jss::result][jss::error] == "invalidParams"); + }); + } + void testGWBOverflow() { @@ -280,6 +319,7 @@ public: { testGWB(feature); testGWBApiVersions(feature); + testGWBInvalidAccount(feature); } testGWBWithMPT(); testGWBOverflow(); diff --git a/src/xrpld/rpc/handlers/account/GatewayBalances.cpp b/src/xrpld/rpc/handlers/account/GatewayBalances.cpp index ff19d1d1e5..041e878a3f 100644 --- a/src/xrpld/rpc/handlers/account/GatewayBalances.cpp +++ b/src/xrpld/rpc/handlers/account/GatewayBalances.cpp @@ -63,6 +63,12 @@ doGatewayBalances(rpc::JsonContext& context) if (!(params.isMember(jss::account) || params.isMember(jss::ident))) return rpc::missingFieldError(jss::account); + if (params.isMember(jss::account) && !params[jss::account].isString()) + return rpc::invalidFieldError(jss::account); + + if (params.isMember(jss::ident) && !params[jss::ident].isString()) + return rpc::invalidFieldError(jss::ident); + std::string const strIdent( params.isMember(jss::account) ? params[jss::account].asString() : params[jss::ident].asString()); From a3147740f2f610f0810a6e4eb56aa7cbe4c5cc12 Mon Sep 17 00:00:00 2001 From: klemenfn <102049210+klemenfn@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:24:56 +0000 Subject: [PATCH 9/9] build: Fix GCC 14 compilation (#7981) Co-authored-by: Ayaz Salikhov --- include/xrpl/json/json_value.h | 2 ++ src/xrpld/rpc/detail/RPCLedgerHelpers.cpp | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/include/xrpl/json/json_value.h b/include/xrpl/json/json_value.h index be126d8b8e..57936a774f 100644 --- a/include/xrpl/json/json_value.h +++ b/include/xrpl/json/json_value.h @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -524,6 +525,7 @@ public: class ValueIteratorBase { public: + using iterator_category = std::bidirectional_iterator_tag; using size_t = unsigned int; using difference_type = int; using SelfType = ValueIteratorBase; diff --git a/src/xrpld/rpc/detail/RPCLedgerHelpers.cpp b/src/xrpld/rpc/detail/RPCLedgerHelpers.cpp index 52e68e87f1..19fe294924 100644 --- a/src/xrpld/rpc/detail/RPCLedgerHelpers.cpp +++ b/src/xrpld/rpc/detail/RPCLedgerHelpers.cpp @@ -331,6 +331,13 @@ getLedger<>(std::shared_ptr&, LedgerShortcut shortcut, Context c template Status getLedger<>(std::shared_ptr&, uint256 const&, Context const&); +// explicit instantiation of ledgerFromSpecifier +template Status +ledgerFromSpecifier<>( + std::shared_ptr&, + org::xrpl::rpc::v1::LedgerSpecifier const&, + Context const&); + // The previous version of the lookupLedger command would accept the // "ledger_index" argument as a string and silently treat it as a request to // return the current ledger which, while not strictly wrong, could cause a lot