From a72c237373f88be17aaf47ff93080867ac48ecca Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 3 Sep 2025 18:04:05 -0400 Subject: [PATCH] Review feedback from @Bronek - Remove unnecessary #include - Explanatory comments - Make the MPT InvariantCheck related to EscrowFinish amendment safe - Convert SField maps to unordered_maps - Make jtx::fee::operator() clearer - Rename checkMyPrivilege to hasPrivilege --- include/xrpl/protocol/Protocol.h | 1 - include/xrpl/protocol/SField.h | 6 +- .../xrpl/protocol/detail/transactions.macro | 2 +- src/libxrpl/protocol/SField.cpp | 4 +- src/test/jtx/impl/fee.cpp | 3 +- src/xrpld/app/tx/detail/InvariantCheck.cpp | 72 +++++++++++++++---- src/xrpld/ledger/detail/View.cpp | 4 ++ 7 files changed, 71 insertions(+), 21 deletions(-) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 898fd06fbd..bd39233cca 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -22,7 +22,6 @@ #include #include -#include #include diff --git a/include/xrpl/protocol/SField.h b/include/xrpl/protocol/SField.h index 7bf5394641..2f85cf3b7c 100644 --- a/include/xrpl/protocol/SField.h +++ b/include/xrpl/protocol/SField.h @@ -300,7 +300,7 @@ public: static int compare(SField const& f1, SField const& f2); - static std::map const& + static std::unordered_map const& getKnownCodeToField() { return knownCodeToField; @@ -308,8 +308,8 @@ public: private: static int num; - static std::map knownCodeToField; - static std::map knownNameToField; + static std::unordered_map knownCodeToField; + static std::unordered_map knownNameToField; }; /** A field with a type known at compile time. */ diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 5b1a6ac774..e75ce67ab1 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -77,7 +77,7 @@ TRANSACTION(ttESCROW_CREATE, 1, EscrowCreate, /** This transaction type completes an existing escrow. */ TRANSACTION(ttESCROW_FINISH, 2, EscrowFinish, Delegation::delegatable, - mayAuthorizeMPT, + noPriv, ({ {sfOwner, soeREQUIRED}, {sfOfferSequence, soeREQUIRED}, diff --git a/src/libxrpl/protocol/SField.cpp b/src/libxrpl/protocol/SField.cpp index fcf89f5e18..57e5849c00 100644 --- a/src/libxrpl/protocol/SField.cpp +++ b/src/libxrpl/protocol/SField.cpp @@ -28,8 +28,8 @@ namespace ripple { // Storage for static const members. SField::IsSigning const SField::notSigning; int SField::num = 0; -std::map SField::knownCodeToField; -std::map SField::knownNameToField; +std::unordered_map SField::knownCodeToField; +std::unordered_map SField::knownNameToField; // Give only this translation unit permission to construct SFields struct SField::private_access_tag_t diff --git a/src/test/jtx/impl/fee.cpp b/src/test/jtx/impl/fee.cpp index b887849946..a58105a85f 100644 --- a/src/test/jtx/impl/fee.cpp +++ b/src/test/jtx/impl/fee.cpp @@ -31,9 +31,10 @@ fee::operator()(Env& env, JTx& jt) const if (!manual_) return; jt.fill_fee = false; + assert(!increment_ || !amount_); if (increment_) jt[sfFee] = STAmount(env.current()->fees().increment).getJson(); - if (amount_) + else if (amount_) jt[sfFee] = amount_->getJson(JsonOptions::none); } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 10c008ea1f..83d5576836 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -37,6 +37,25 @@ namespace ripple { +/* +assert(enforce) + +There are several asserts (or XRPL_ASSERTs) in this file that check a variable +named `enforce` when an invariant fails. At first glace, those asserts may look +incorrect, but they are not. + +Those asserts take advantage of two facts: +1. `asserts` are not (normally) executed in release builds. +2. Invariants should *never* fail, except in tests that specifically modify + the open ledger to break them. + +To make it sort of a second-layer of invariant enforcement aimed at +_developers_. It's designed to fire if a developer writes code that violates an +invariant, and runs it in unit tests or a develop build that _does not have the +relevant amendments enabled_. It's intentionally a pain in the neck so that bad +code gets caught and fixed as early as possible. +*/ + enum Privilege { noPriv = 0x0000, // The transaction can not do any of the enumerated operations @@ -75,7 +94,7 @@ operator|(Privilege lhs, Privilege rhs) } bool -checkMyPrivilege(STTx const& tx, Privilege priv) +hasPrivilege(STTx const& tx, Privilege priv) { switch (tx.getTxnType()) { @@ -432,7 +451,7 @@ AccountRootsNotDeleted::finalize( // transaction when the total AMM LP Tokens balance goes to 0. // A successful AccountDelete or AMMDelete MUST delete exactly // one account root. - if (checkMyPrivilege(tx, mustDeleteAcct) && result == tesSUCCESS) + if (hasPrivilege(tx, mustDeleteAcct) && result == tesSUCCESS) { if (accountsDeleted_ == 1) return true; @@ -449,7 +468,7 @@ AccountRootsNotDeleted::finalize( // A successful AMMWithdraw/AMMClawback MAY delete one account root // when the total AMM LP Tokens balance goes to 0. Not every AMM withdraw // deletes the AMM account, accountsDeleted_ is set if it is deleted. - if (checkMyPrivilege(tx, mayDeleteAcct) && result == tesSUCCESS && + if (hasPrivilege(tx, mayDeleteAcct) && result == tesSUCCESS && accountsDeleted_ == 1) return true; @@ -505,6 +524,8 @@ AccountRootsDeletedClean::finalize( JLOG(j.fatal()) << "Invariant failed: account deletion left behind a " << typeName << " object"; + // The comment above starting with "assert(enforce)" explains this + // assert. XRPL_ASSERT( enforce, "ripple::AccountRootsDeletedClean::finalize::objectExists : " @@ -750,6 +771,8 @@ TransfersNotFrozen::finalize( // just in case so rippled doesn't crash in release. if (!issuerSle) { + // The comment above starting with "assert(enforce)" explains this + // assert. XRPL_ASSERT( enforce, "ripple::TransfersNotFrozen::finalize : enforce " @@ -938,7 +961,7 @@ TransfersNotFrozen::validateFrozenState( } // AMMClawbacks are allowed to override some freeze rules - if ((!isAMMLine || globalFreeze) && checkMyPrivilege(tx, overrideFreeze)) + if ((!isAMMLine || globalFreeze) && hasPrivilege(tx, overrideFreeze)) { JLOG(j.debug()) << "Invariant check allowing funds to be moved " << (change.balanceChangeSign > 0 ? "to" : "from") @@ -949,6 +972,7 @@ TransfersNotFrozen::validateFrozenState( JLOG(j.fatal()) << "Invariant failed: Attempting to move frozen funds for " << tx.getTransactionID(); + // The comment above starting with "assert(enforce)" explains this assert. XRPL_ASSERT( enforce, "ripple::TransfersNotFrozen::validateFrozenState : enforce " @@ -998,13 +1022,12 @@ ValidNewAccountRoot::finalize( } // From this point on we know exactly one account was created. - if (checkMyPrivilege(tx, createAcct | createPseudoAcct) && - result == tesSUCCESS) + if (hasPrivilege(tx, createAcct | createPseudoAcct) && result == tesSUCCESS) { bool const pseudoAccount = (pseudoAccount_ && view.rules().enabled(featureSingleAssetVault)); - if (pseudoAccount && !checkMyPrivilege(tx, createPseudoAcct)) + if (pseudoAccount && !hasPrivilege(tx, createPseudoAcct)) { JLOG(j.fatal()) << "Invariant failed: pseudo-account created by a " "wrong transaction type"; @@ -1238,7 +1261,7 @@ NFTokenCountTracking::finalize( ReadView const& view, beast::Journal const& j) { - if (!checkMyPrivilege(tx, changeNFTCounts)) + if (!hasPrivilege(tx, changeNFTCounts)) { if (beforeMintedTotal != afterMintedTotal) { @@ -1423,12 +1446,12 @@ ValidMPTIssuance::finalize( STTx const& tx, TER const result, XRPAmount const _fee, - ReadView const& _view, + ReadView const& view, beast::Journal const& j) { if (result == tesSUCCESS) { - if (checkMyPrivilege(tx, createMPTIssuance)) + if (hasPrivilege(tx, createMPTIssuance)) { if (mptIssuancesCreated_ == 0) { @@ -1449,7 +1472,7 @@ ValidMPTIssuance::finalize( return mptIssuancesCreated_ == 1 && mptIssuancesDeleted_ == 0; } - if (checkMyPrivilege(tx, destroyMPTIssuance)) + if (hasPrivilege(tx, destroyMPTIssuance)) { if (mptIssuancesDeleted_ == 0) { @@ -1470,7 +1493,17 @@ ValidMPTIssuance::finalize( return mptIssuancesCreated_ == 0 && mptIssuancesDeleted_ == 1; } - if (checkMyPrivilege(tx, mustAuthorizeMPT | mayAuthorizeMPT)) + // ttESCROW_FINISH may authorize an MPT, but it can't have the + // mayAuthorizeMPT privilege, because that may cause + // non-amendment-gated side effects. + bool const enforceEscrowFinish = (tx.getTxnType() == ttESCROW_FINISH) && + (view.rules().enabled(featureSingleAssetVault) + /* + TODO: Uncomment when LendingProtocol is defined + || view.rules().enabled(featureLendingProtocol)*/ + ); + if (hasPrivilege(tx, mustAuthorizeMPT | mayAuthorizeMPT) || + enforceEscrowFinish) { bool const submittedByIssuer = tx.isFieldPresent(sfHolder); @@ -1496,7 +1529,7 @@ ValidMPTIssuance::finalize( return false; } else if ( - !submittedByIssuer && !checkMyPrivilege(tx, mayAuthorizeMPT) && + !submittedByIssuer && hasPrivilege(tx, mustAuthorizeMPT) && (mptokensCreated_ + mptokensDeleted_ != 1)) { // if the holder submitted this tx, then a mptoken must be @@ -1509,6 +1542,17 @@ ValidMPTIssuance::finalize( return true; } + if (tx.getTxnType() == ttESCROW_FINISH) + { + // ttESCROW_FINISH may authorize an MPT, but it can't have the + // mayAuthorizeMPT privilege, because that may cause + // non-amendment-gated side effects. + XRPL_ASSERT_PARTS( + !enforceEscrowFinish, + "ripple::ValidMPTIssuance::finalize", + "not escrow finish tx"); + return true; + } } if (mptIssuancesCreated_ != 0) @@ -1711,6 +1755,8 @@ ValidPseudoAccounts::finalize( beast::Journal const& j) { bool const enforce = view.rules().enabled(featureSingleAssetVault); + + // The comment above starting with "assert(enforce)" explains this assert. XRPL_ASSERT( errors_.empty() || enforce, "ripple::ValidPseudoAccounts::finalize : no bad " diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 9e7cb34e19..3a46a4bd32 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -1092,9 +1092,13 @@ getPseudoAccountFields() static std::vector const pseudoFields = []() { auto const ar = LedgerFormats::getInstance().findByType(ltACCOUNT_ROOT); if (!ar) + { + // LCOV_EXCL_START LogicError( "ripple::isPseudoAccount : unable to find account root ledger " "format"); + // LCOV_EXCL_STOP + } auto const& soTemplate = ar->getSOTemplate(); std::vector pseudoFields;