diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 96ad7e00bc9..ef8a0537ed0 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -578,9 +579,9 @@ class Transactor : public TxInvariantCheck * ordering is enforced. */ void - visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) final + visitEntry(InvariantEntry const& entry) final { - visitInvariantEntry(isDelete, before, after); + visitInvariantEntry(entry.isDelete(), entry.before(), entry.after()); } [[nodiscard]] bool diff --git a/include/xrpl/tx/invariants/AMMInvariant.h b/include/xrpl/tx/invariants/AMMInvariant.h index 29d962fb79d..63a98783a29 100644 --- a/include/xrpl/tx/invariants/AMMInvariant.h +++ b/include/xrpl/tx/invariants/AMMInvariant.h @@ -4,10 +4,10 @@ #include #include #include -#include #include #include #include +#include #include @@ -27,7 +27,7 @@ class ValidAMM ValidAMM() = default; void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/DirectoryInvariant.h b/include/xrpl/tx/invariants/DirectoryInvariant.h index d18ac539cc0..3386ce3f6f2 100644 --- a/include/xrpl/tx/invariants/DirectoryInvariant.h +++ b/include/xrpl/tx/invariants/DirectoryInvariant.h @@ -4,12 +4,10 @@ #include #include #include -#include #include #include #include - -#include +#include namespace xrpl { @@ -20,7 +18,7 @@ class ValidBookDirectory public: void - visitEntry(bool, std::shared_ptr const&, std::shared_ptr const&); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/FreezeInvariant.h b/include/xrpl/tx/invariants/FreezeInvariant.h index 301e464daf7..a2fccbbc3f7 100644 --- a/include/xrpl/tx/invariants/FreezeInvariant.h +++ b/include/xrpl/tx/invariants/FreezeInvariant.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -44,7 +45,7 @@ class TransfersNotFrozen public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/InvariantCheck.h b/include/xrpl/tx/invariants/InvariantCheck.h index e8dafbd3017..6a183b2db2e 100644 --- a/include/xrpl/tx/invariants/InvariantCheck.h +++ b/include/xrpl/tx/invariants/InvariantCheck.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -71,21 +72,13 @@ class InvariantChecker_PROTOTYPE /** * @brief called for each ledger entry in the current transaction. * - * @param isDelete true if the SLE is being deleted. - * @param before ledger entry before modification by the transaction. `before` will be null if - * the entry is new. - * @param after ledger entry after modification by the transaction. Always non-null. When - * deleting, `after` may differ from `before`. Whether that is important is up to the - * individual invariant check. + * @param entry validated, non-owning view of the modified ledger entry. * - * @note `after` IS NEVER NULL. `isDelete` is the only correct way to check for deletions. - * Do not make logic or branching decisions on whether on `after` is set, because it will - * always be set. Treat a null `after` as a programming error (with XRPL_ASSERT). An - * invariant MAY check for null defensively, if it makes more sense, but an assertion is - * preferred for new invariants. + * @note `entry.after()` IS NEVER NULL. `entry.isDelete()` is the only + * correct way to check for deletions. */ void - visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after); + visitEntry(InvariantEntry const& entry); /** * @brief called after all ledger entries have been visited to determine @@ -123,7 +116,7 @@ class TransactionFeeCheck { public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); static bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); @@ -143,7 +136,7 @@ class XRPNotCreated public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -163,7 +156,7 @@ class AccountRootsNotDeleted public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -190,7 +183,7 @@ class AccountRootsDeletedClean public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); @@ -209,7 +202,7 @@ class XRPBalanceChecks public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -226,7 +219,7 @@ class LedgerEntryTypesMatch public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -244,7 +237,7 @@ class NoXRPTrustLines public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -263,7 +256,7 @@ class NoDeepFreezeTrustLinesWithoutFreeze public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -282,7 +275,7 @@ class NoBadOffers public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -298,7 +291,7 @@ class NoZeroEscrow public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -318,7 +311,7 @@ class ValidNewAccountRoot public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -348,7 +341,7 @@ class ValidClawback public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -368,7 +361,7 @@ class ValidPseudoAccounts public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); @@ -388,7 +381,7 @@ class NoModifiedUnmodifiableFields public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); @@ -404,7 +397,7 @@ class ValidAmounts public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -421,7 +414,7 @@ class ObjectHasPseudoAccount { public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; diff --git a/include/xrpl/tx/invariants/InvariantEntry.h b/include/xrpl/tx/invariants/InvariantEntry.h new file mode 100644 index 00000000000..1d3b3c73956 --- /dev/null +++ b/include/xrpl/tx/invariants/InvariantEntry.h @@ -0,0 +1,53 @@ +#pragma once + +#include +#include + +#include +#include + +namespace xrpl { + +/** + * A validated ledger entry visited by invariants. + */ +class InvariantEntry +{ + bool isDelete_; + SLE::const_pointer before_; + SLE::const_pointer after_; + +public: + InvariantEntry(bool isDelete, SLE::const_pointer before, SLE::const_pointer after) + : isDelete_(isDelete), before_(std::move(before)), after_(std::move(after)) + { + if (after_ == nullptr) + Throw("InvariantEntry: after is never null"); + if (isDelete_ && before_ == nullptr) + Throw("InvariantEntry: deleted entry missing before state"); + } + + InvariantEntry(InvariantEntry const&) = delete; + InvariantEntry& + operator=(InvariantEntry const&) = delete; + + [[nodiscard]] bool + isDelete() const + { + return isDelete_; + } + + [[nodiscard]] SLE::const_ref + before() const + { + return before_; + } + + [[nodiscard]] SLE::const_ref + after() const + { + return after_; + } +}; + +} // namespace xrpl diff --git a/include/xrpl/tx/invariants/InvariantRunner.h b/include/xrpl/tx/invariants/InvariantRunner.h index 29a9dc09b26..8489b640ed2 100644 --- a/include/xrpl/tx/invariants/InvariantRunner.h +++ b/include/xrpl/tx/invariants/InvariantRunner.h @@ -2,11 +2,11 @@ #include #include -#include #include #include #include #include +#include #include #include @@ -66,16 +66,10 @@ class TxInvariantCheck /** * @brief Called for each ledger entry modified by the transaction. * - * @param isDelete true if the SLE is being deleted. - * @param before the entry's state before the transaction (nullptr for - * newly created entries). - * @param after the entry's state after the transaction. For deletions - * this is the SLE being erased; use @p isDelete rather than - * a null @p after to detect deletions. @p after is - * never null. + * @param entry a validated, non-owning view of the modified entry. */ virtual void - visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) = 0; + visitEntry(InvariantEntry const& entry) = 0; /** * @brief Called after all entries have been visited. diff --git a/include/xrpl/tx/invariants/LoanBrokerInvariant.h b/include/xrpl/tx/invariants/LoanBrokerInvariant.h index 979f57de350..a7bc2e5a513 100644 --- a/include/xrpl/tx/invariants/LoanBrokerInvariant.h +++ b/include/xrpl/tx/invariants/LoanBrokerInvariant.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -48,7 +49,7 @@ class ValidLoanBroker public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/LoanInvariant.h b/include/xrpl/tx/invariants/LoanInvariant.h index fc72b8d4205..83cee641b8f 100644 --- a/include/xrpl/tx/invariants/LoanInvariant.h +++ b/include/xrpl/tx/invariants/LoanInvariant.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -28,7 +29,7 @@ class ValidLoan public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/MPTInvariant.h b/include/xrpl/tx/invariants/MPTInvariant.h index 5740cd5be26..5f2cf378aeb 100644 --- a/include/xrpl/tx/invariants/MPTInvariant.h +++ b/include/xrpl/tx/invariants/MPTInvariant.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -54,12 +55,10 @@ class ValidMPTIssuance * @brief Track MPT issuance and holding creations, deletions, and * mutations. * - * @param isDelete Whether the ledger entry is being deleted. - * @param before The ledger entry before transaction application. - * @param after The ledger entry after transaction application. + * @param entry The ledger entry change being checked. */ void - visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after); + visitEntry(InvariantEntry const& entry); /** * @brief Verify MPT issuance invariants after transaction application. @@ -106,12 +105,10 @@ class ValidMPTBalanceChanges /** * @brief Track MPT amount and outstanding amount changes. * - * @param isDelete Whether the ledger entry is being deleted. - * @param before The ledger entry before transaction application. - * @param after The ledger entry after transaction application. + * @param entry The ledger entry change being checked. */ void - visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after); + visitEntry(InvariantEntry const& entry); /** * @brief Verify public MPT payment accounting invariants. @@ -173,15 +170,10 @@ class ValidConfidentialMPToken /** * @brief Track confidential MPT balance, issuance, and version changes. * - * @param isDelete Whether the ledger entry is being deleted. - * @param before The ledger entry before transaction application. - * @param after The ledger entry after transaction application. + * @param entry The ledger entry change being checked. */ void - visitEntry( - bool isDelete, - std::shared_ptr const& before, - std::shared_ptr const& after); + visitEntry(InvariantEntry const& entry); /** * @brief Verify confidential MPT accounting and encrypted-field @@ -220,15 +212,10 @@ class ValidMPTTransfer /** * @brief Track MPT balance changes and deleted authorization state. * - * @param isDelete Whether the ledger entry is being deleted. - * @param before The ledger entry before transaction application. - * @param after The ledger entry after transaction application. + * @param entry The ledger entry change being checked. */ void - visitEntry( - bool isDelete, - std::shared_ptr const& before, - std::shared_ptr const& after); + visitEntry(InvariantEntry const& entry); /** * @brief Verify MPT transfer authorization invariants. diff --git a/include/xrpl/tx/invariants/NFTInvariant.h b/include/xrpl/tx/invariants/NFTInvariant.h index 11bdf5a5690..f52a79661c4 100644 --- a/include/xrpl/tx/invariants/NFTInvariant.h +++ b/include/xrpl/tx/invariants/NFTInvariant.h @@ -2,10 +2,10 @@ #include #include -#include #include #include #include +#include #include @@ -34,7 +34,7 @@ class ValidNFTokenPage public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -62,7 +62,7 @@ class NFTokenCountTracking public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; diff --git a/include/xrpl/tx/invariants/PermissionedDEXInvariant.h b/include/xrpl/tx/invariants/PermissionedDEXInvariant.h index ae0d5733855..a189ae5f530 100644 --- a/include/xrpl/tx/invariants/PermissionedDEXInvariant.h +++ b/include/xrpl/tx/invariants/PermissionedDEXInvariant.h @@ -4,10 +4,10 @@ #include #include #include -#include #include #include #include +#include namespace xrpl { @@ -22,7 +22,7 @@ class ValidPermissionedDEX public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/PermissionedDomainInvariant.h b/include/xrpl/tx/invariants/PermissionedDomainInvariant.h index fae7c24d257..3a9a047d8f6 100644 --- a/include/xrpl/tx/invariants/PermissionedDomainInvariant.h +++ b/include/xrpl/tx/invariants/PermissionedDomainInvariant.h @@ -2,10 +2,10 @@ #include #include -#include #include #include #include +#include #include #include @@ -35,7 +35,7 @@ class ValidPermissionedDomain public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/include/xrpl/tx/invariants/SponsorshipInvariant.h b/include/xrpl/tx/invariants/SponsorshipInvariant.h index e664685c16e..9e2c6916819 100644 --- a/include/xrpl/tx/invariants/SponsorshipInvariant.h +++ b/include/xrpl/tx/invariants/SponsorshipInvariant.h @@ -2,10 +2,10 @@ #include #include -#include #include #include #include +#include #include @@ -31,7 +31,7 @@ class SponsorshipOwnerCountsMatch public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; @@ -52,7 +52,7 @@ class SponsorshipAccountCountMatchesField public: void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; diff --git a/include/xrpl/tx/invariants/VaultInvariant.h b/include/xrpl/tx/invariants/VaultInvariant.h index 2ba42f0ab4a..e8934304171 100644 --- a/include/xrpl/tx/invariants/VaultInvariant.h +++ b/include/xrpl/tx/invariants/VaultInvariant.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -183,7 +184,7 @@ class ValidVault computeCoarsestScale(std::vector const& numbers); void - visitEntry(bool, SLE::const_ref, SLE::const_ref); + visitEntry(InvariantEntry const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); diff --git a/src/libxrpl/tx/invariants/AMMInvariant.cpp b/src/libxrpl/tx/invariants/AMMInvariant.cpp index cca0ce149c2..b4267610cad 100644 --- a/src/libxrpl/tx/invariants/AMMInvariant.cpp +++ b/src/libxrpl/tx/invariants/AMMInvariant.cpp @@ -18,14 +18,19 @@ #include #include #include +#include #include namespace xrpl { void -ValidAMM::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidAMM::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + if (isDelete) { if (before && before->getType() == ltAMM) @@ -36,23 +41,20 @@ ValidAMM::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) return; } - if (after) + auto const type = after->getType(); + // AMM object changed + if (type == ltAMM) { - auto const type = after->getType(); - // AMM object changed - if (type == ltAMM) - { - ammAccount_ = after->getAccountID(sfAccount); - lptAMMBalanceAfter_ = after->getFieldAmount(sfLPTokenBalance); - } - // AMM pool changed - else if ( - (type == ltRIPPLE_STATE && after->isFlag(lsfAMMNode)) || - (type == ltACCOUNT_ROOT && after->isFieldPresent(sfAMMID)) || - (type == ltMPTOKEN && after->isFlag(lsfMPTAMM))) - { - ammPoolChanged_ = true; - } + ammAccount_ = after->getAccountID(sfAccount); + lptAMMBalanceAfter_ = after->getFieldAmount(sfLPTokenBalance); + } + // AMM pool changed + else if ( + (type == ltRIPPLE_STATE && after->isFlag(lsfAMMNode)) || + (type == ltACCOUNT_ROOT && after->isFieldPresent(sfAMMID)) || + (type == ltMPTOKEN && after->isFlag(lsfMPTAMM))) + { + ammPoolChanged_ = true; } if (before) diff --git a/src/libxrpl/tx/invariants/DirectoryInvariant.cpp b/src/libxrpl/tx/invariants/DirectoryInvariant.cpp index b850f02ab57..be35cffd708 100644 --- a/src/libxrpl/tx/invariants/DirectoryInvariant.cpp +++ b/src/libxrpl/tx/invariants/DirectoryInvariant.cpp @@ -12,9 +12,9 @@ #include #include #include +#include #include -#include namespace xrpl { @@ -41,19 +41,19 @@ badExchangeRate(SLE const& dir) } // namespace void -ValidBookDirectory::visitEntry( - bool isDelete, - std::shared_ptr const& before, - std::shared_ptr const& after) +ValidBookDirectory::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + // New root directories must have matching exchange-rate metadata. New // child directories, and modified directories that change sfRootIndex, must // point to an existing root. // Only validate newly-created directories and sfRootIndex changes; - // LedgerStateFix handles legacy bad exchange-rate metadata. Skip deletions - // because `after` is not guaranteed to be null. - if (badBookDirectory_ || isDelete || !after || after->getType() != ltDIR_NODE) + // LedgerStateFix handles legacy bad exchange-rate metadata. Skip deletions. + if (badBookDirectory_ || isDelete || after->getType() != ltDIR_NODE) return; auto const rootIndex = after->getFieldH256(sfRootIndex); diff --git a/src/libxrpl/tx/invariants/FreezeInvariant.cpp b/src/libxrpl/tx/invariants/FreezeInvariant.cpp index 272e52f09a7..7dc77d2441c 100644 --- a/src/libxrpl/tx/invariants/FreezeInvariant.cpp +++ b/src/libxrpl/tx/invariants/FreezeInvariant.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -25,8 +26,12 @@ namespace xrpl { void -TransfersNotFrozen::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +TransfersNotFrozen::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + /* * A trust line freeze state alone doesn't determine if a transfer is * frozen. The transfer must be examined "end-to-end" because both sides of @@ -116,13 +121,6 @@ TransfersNotFrozen::finalize( bool TransfersNotFrozen::isValidEntry(SLE::const_ref before, SLE::const_ref after) { - // `after` can never be null, even if the trust line is deleted. - XRPL_ASSERT(after, "xrpl::TransfersNotFrozen::isValidEntry : valid after."); - if (!after) - { - return false; - } - if (after->getType() == ltACCOUNT_ROOT) { possibleIssuers_.emplace(after->at(sfAccount), after); diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index aa4df8db42a..1a6272da9a4 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -29,11 +29,11 @@ #include #include #include +#include #include #include #include -#include #include #include #include @@ -88,7 +88,7 @@ ledgerEntryTypeName(SLE const& sle) } void -TransactionFeeCheck::visitEntry(bool, SLE::const_ref, SLE::const_ref) +TransactionFeeCheck::visitEntry(InvariantEntry const&) { // nothing to do } @@ -131,8 +131,12 @@ TransactionFeeCheck::finalize( //------------------------------------------------------------------------------ void -XRPNotCreated::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +XRPNotCreated::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + /* We go through all modified ledger entries, looking only at account roots, * escrow payments, and payment channels. We remove from the total any * previous XRP values and add to the total any new XRP values. The net @@ -168,13 +172,6 @@ XRPNotCreated::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref a } } - if (!after) - { - // LCOV_EXCL_START - UNREACHABLE("xrpl::XRPNotCreated::visitEntry : after can't be null"); - return; - // LCOV_EXCL_STOP - } switch (after->getType()) { case ltACCOUNT_ROOT: @@ -232,8 +229,11 @@ XRPNotCreated::finalize( //------------------------------------------------------------------------------ void -XRPBalanceChecks::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) +XRPBalanceChecks::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + auto isBad = [](STAmount const& balance) { if (!balance.native()) return true; @@ -255,7 +255,7 @@ XRPBalanceChecks::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) if (before && before->getType() == ltACCOUNT_ROOT) bad_ |= isBad((*before)[sfBalance]); - if (after && after->getType() == ltACCOUNT_ROOT) + if (after->getType() == ltACCOUNT_ROOT) bad_ |= isBad((*after)[sfBalance]); } @@ -279,8 +279,11 @@ XRPBalanceChecks::finalize( //------------------------------------------------------------------------------ void -NoBadOffers::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +NoBadOffers::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + auto isBad = [](STAmount const& pays, STAmount const& gets) { // An offer should never be negative if (pays < beast::kZero) @@ -296,7 +299,7 @@ NoBadOffers::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref aft if (before && before->getType() == ltOFFER) bad_ |= isBad((*before)[sfTakerPays], (*before)[sfTakerGets]); - if (after && after->getType() == ltOFFER) + if (after->getType() == ltOFFER) bad_ |= isBad((*after)[sfTakerPays], (*after)[sfTakerGets]); } @@ -320,8 +323,11 @@ NoBadOffers::finalize( //------------------------------------------------------------------------------ void -NoZeroEscrow::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +NoZeroEscrow::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + auto isBad = [](STAmount const& amount) { // XRP case if (amount.native()) @@ -364,7 +370,7 @@ NoZeroEscrow::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref af if (before && before->getType() == ltESCROW) bad_ |= isBad((*before)[sfAmount]); - if (after && after->getType() == ltESCROW) + if (after->getType() == ltESCROW) bad_ |= isBad((*after)[sfAmount]); auto checkAmount = [this](std::int64_t amount) { @@ -374,7 +380,7 @@ NoZeroEscrow::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref af bool const overwriteFixEnabled = isFeatureEnabled(fixCleanup3_1_3, true); - if (after && after->getType() == ltMPTOKEN_ISSUANCE) + if (after->getType() == ltMPTOKEN_ISSUANCE) { auto const outstanding = (*after)[sfOutstandingAmount]; checkAmount(outstanding); @@ -393,7 +399,7 @@ NoZeroEscrow::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref af } } - if (after && after->getType() == ltMPTOKEN) + if (after->getType() == ltMPTOKEN) { auto const mptAmount = (*after)[sfMPTAmount]; checkAmount(mptAmount); @@ -424,8 +430,11 @@ NoZeroEscrow::finalize( //------------------------------------------------------------------------------ void -AccountRootsNotDeleted::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref) +AccountRootsNotDeleted::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + if (isDelete && before && before->getType() == ltACCOUNT_ROOT) accountsDeleted_++; } @@ -476,8 +485,12 @@ AccountRootsNotDeleted::finalize( //------------------------------------------------------------------------------ void -AccountRootsDeletedClean::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +AccountRootsDeletedClean::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + if (isDelete && before && before->getType() == ltACCOUNT_ROOT) accountsDeleted_.emplace_back(before, after); } @@ -599,31 +612,31 @@ AccountRootsDeletedClean::finalize( //------------------------------------------------------------------------------ void -LedgerEntryTypesMatch::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) +LedgerEntryTypesMatch::visitEntry(InvariantEntry const& entry) { - if (before && after && before->getType() != after->getType()) + auto const& before = entry.before(); + auto const& after = entry.after(); + + if (before && before->getType() != after->getType()) typeMismatch_ = true; - if (after) - { #pragma push_macro("LEDGER_ENTRY") #undef LEDGER_ENTRY #define LEDGER_ENTRY(tag, ...) case tag: - switch (after->getType()) - { + switch (after->getType()) + { #include + break; + default: + invalidTypeAdded_ = true; break; - default: - invalidTypeAdded_ = true; - break; - } + } #undef LEDGER_ENTRY #pragma pop_macro("LEDGER_ENTRY") - } } bool @@ -653,11 +666,13 @@ LedgerEntryTypesMatch::finalize( //------------------------------------------------------------------------------ void -NoXRPTrustLines::visitEntry(bool, SLE::const_ref, SLE::const_ref after) +NoXRPTrustLines::visitEntry(InvariantEntry const& entry) { + auto const& after = entry.after(); + bool const overwriteFixEnabled = isFeatureEnabled(fixCleanup3_1_3, true); - if (after && after->getType() == ltRIPPLE_STATE) + if (after->getType() == ltRIPPLE_STATE) { // checking the issue directly here instead of // relying on .native() just in case native somehow @@ -693,9 +708,11 @@ NoXRPTrustLines::finalize( //------------------------------------------------------------------------------ void -NoDeepFreezeTrustLinesWithoutFreeze::visitEntry(bool, SLE::const_ref, SLE::const_ref after) +NoDeepFreezeTrustLinesWithoutFreeze::visitEntry(InvariantEntry const& entry) { - if (after && after->getType() == ltRIPPLE_STATE) + auto const& after = entry.after(); + + if (after->getType() == ltRIPPLE_STATE) { bool const overwriteFixEnabled = isFeatureEnabled(fixCleanup3_1_3, true); @@ -736,8 +753,11 @@ NoDeepFreezeTrustLinesWithoutFreeze::finalize( //------------------------------------------------------------------------------ void -ValidNewAccountRoot::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) +ValidNewAccountRoot::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + if (!before && after->getType() == ltACCOUNT_ROOT) { accountsCreated_++; @@ -834,15 +854,19 @@ clawbackTrustLineBalanceInHolderTerms( } void -ValidClawback::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidClawback::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + if (before && before->getType() == ltRIPPLE_STATE) { trustlinesChanged_++; iou_.before = before; } - if (!isDelete && after && after->getType() == ltRIPPLE_STATE) + if (!isDelete && after->getType() == ltRIPPLE_STATE) iou_.after = after; if (before && before->getType() == ltMPTOKEN) @@ -851,7 +875,7 @@ ValidClawback::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref a mpt_.before = before; } - if (!isDelete && after && after->getType() == ltMPTOKEN) + if (!isDelete && after->getType() == ltMPTOKEN) mpt_.after = after; } @@ -1009,15 +1033,19 @@ ValidClawback::finalize( //------------------------------------------------------------------------------ void -ValidPseudoAccounts::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidPseudoAccounts::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + if (isDelete) { // Deletion is ignored return; } - if (after && after->getType() == ltACCOUNT_ROOT) + if (after->getType() == ltACCOUNT_ROOT) { bool const isPseudo = [&]() { // isPseudoAccount checks that any of the pseudo-account fields are @@ -1104,8 +1132,12 @@ ValidPseudoAccounts::finalize( //------------------------------------------------------------------------------ void -NoModifiedUnmodifiableFields::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +NoModifiedUnmodifiableFields::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + if (isDelete || !before) { // Creation and deletion are ignored @@ -1212,12 +1244,12 @@ NoModifiedUnmodifiableFields::finalize( } void -ValidAmounts::visitEntry( - bool isDelete, - std::shared_ptr const&, - std::shared_ptr const& after) +ValidAmounts::visitEntry(InvariantEntry const& entry) { - if (!isDelete && after) + auto const isDelete = entry.isDelete(); + auto const& after = entry.after(); + + if (!isDelete) afterEntries_.push_back(after); } @@ -1243,20 +1275,13 @@ ValidAmounts::finalize( } void -ObjectHasPseudoAccount::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ObjectHasPseudoAccount::visitEntry(InvariantEntry const& entry) { - if (!isDelete) - return; + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); - // Before should never be null when isDelete = true - if (!before) - { - // LCOV_EXCL_START - UNREACHABLE( - "xrpl::ObjectHasPseudoAccount::visitEntry : deleted ledger entry missing before state"); + if (!isDelete) return; - // LCOV_EXCL_STOP - } switch (before->getType()) { diff --git a/src/libxrpl/tx/invariants/InvariantRunner.cpp b/src/libxrpl/tx/invariants/InvariantRunner.cpp index 55bff2d6938..03b5ca8a194 100644 --- a/src/libxrpl/tx/invariants/InvariantRunner.cpp +++ b/src/libxrpl/tx/invariants/InvariantRunner.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -48,9 +49,11 @@ checkInvariantsHelper( auto checkers = getInvariantChecks(); ctx.visit([&](uint256 const&, bool isDelete, SLE::const_ref before, SLE::const_ref after) { + InvariantEntry const entry{isDelete, before, after}; + if (txCheck) - txCheck->get().visitEntry(isDelete, before, after); - (..., std::get(checkers).visitEntry(isDelete, before, after)); + txCheck->get().visitEntry(entry); + (..., std::get(checkers).visitEntry(entry)); }); if (txCheck) diff --git a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp index b70c02947ff..98cf792d592 100644 --- a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp @@ -14,36 +14,37 @@ #include #include #include +#include #include namespace xrpl { void -ValidLoanBroker::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidLoanBroker::visitEntry(InvariantEntry const& entry) { - if (after) + auto const& before = entry.before(); + auto const& after = entry.after(); + + if (after->getType() == ltLOAN_BROKER) { - if (after->getType() == ltLOAN_BROKER) - { - auto& broker = brokers_[after->key()]; - broker.brokerBefore = before; - broker.brokerAfter = after; - } - else if (after->getType() == ltACCOUNT_ROOT && after->isFieldPresent(sfLoanBrokerID)) - { - auto const& loanBrokerID = after->at(sfLoanBrokerID); - // create an entry if one doesn't already exist - brokers_.emplace(loanBrokerID, BrokerInfo{}); - } - else if (after->getType() == ltRIPPLE_STATE) - { - lines_.emplace_back(after); - } - else if (after->getType() == ltMPTOKEN) - { - mpts_.emplace_back(after); - } + auto& broker = brokers_[after->key()]; + broker.brokerBefore = before; + broker.brokerAfter = after; + } + else if (after->getType() == ltACCOUNT_ROOT && after->isFieldPresent(sfLoanBrokerID)) + { + auto const& loanBrokerID = after->at(sfLoanBrokerID); + // create an entry if one doesn't already exist + brokers_.emplace(loanBrokerID, BrokerInfo{}); + } + else if (after->getType() == ltRIPPLE_STATE) + { + lines_.emplace_back(after); + } + else if (after->getType() == ltMPTOKEN) + { + mpts_.emplace_back(after); } } diff --git a/src/libxrpl/tx/invariants/LoanInvariant.cpp b/src/libxrpl/tx/invariants/LoanInvariant.cpp index 7b967905708..f3bd6d5fdfb 100644 --- a/src/libxrpl/tx/invariants/LoanInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanInvariant.cpp @@ -9,20 +9,23 @@ #include #include #include -#include #include // IWYU pragma: keep #include #include #include +#include #include namespace xrpl { void -ValidLoan::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidLoan::visitEntry(InvariantEntry const& entry) { - if (after && after->getType() == ltLOAN) + auto const& before = entry.before(); + auto const& after = entry.after(); + + if (after->getType() == ltLOAN) { loans_.emplace_back(before, after); } diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 2cfd069420f..b5a266aaac2 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -66,8 +67,12 @@ subtractMPTAmountDelta(std::int64_t delta, std::uint64_t amount) } // namespace void -ValidMPTIssuance::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidMPTIssuance::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + // The sfReferenceHolding tracking and the deleted-holding capture are // only meaningful post-fixCleanup3_2_0 (the field is never set // pre-amendment, and the holding-deletion rule does not apply). @@ -75,7 +80,7 @@ ValidMPTIssuance::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_re // on the hot path. bool const fix320Enabled = isFeatureEnabled(fixCleanup3_2_0); - if (after && after->getType() == ltMPTOKEN_ISSUANCE) + if (after->getType() == ltMPTOKEN_ISSUANCE) { if (isDelete) { @@ -102,7 +107,7 @@ ValidMPTIssuance::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_re } } - if (after && after->getType() == ltMPTOKEN) + if (after->getType() == ltMPTOKEN) { if (isDelete) { @@ -121,7 +126,7 @@ ValidMPTIssuance::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_re // Capture deleted RippleState SLEs so finalize() can verify none of // them were owned by a vault pseudo-account outside VaultDelete. - if (fix320Enabled && isDelete && after && after->getType() == ltRIPPLE_STATE) + if (fix320Enabled && isDelete && after->getType() == ltRIPPLE_STATE) deletedHoldings_.push_back(after); } @@ -416,8 +421,11 @@ ValidMPTIssuance::finalize( } void -ValidMPTBalanceChanges::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) +ValidMPTBalanceChanges::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + if (overflow_) return; @@ -466,15 +474,11 @@ ValidMPTBalanceChanges::visitEntry(bool, SLE::const_ref before, SLE::const_ref a if (before && !update(*before, Order::Before)) return; - if (after) + if (after->getType() == ltMPTOKEN_ISSUANCE) { - if (after->getType() == ltMPTOKEN_ISSUANCE) - { - overflow_ = (*after)[sfOutstandingAmount] > maxMPTAmount(*after); - } - if (!update(*after, Order::After)) - return; + overflow_ = (*after)[sfOutstandingAmount] > maxMPTAmount(*after); } + update(*after, Order::After); } bool @@ -544,11 +548,12 @@ ValidMPTBalanceChanges::finalize( } void -ValidConfidentialMPToken::visitEntry( - bool isDelete, - std::shared_ptr const& before, - std::shared_ptr const& after) +ValidConfidentialMPToken::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + // Helper to get MPToken Issuance ID safely auto const getMptID = [](std::shared_ptr const& sle) -> uint192 { if (!sle) @@ -581,7 +586,7 @@ ValidConfidentialMPToken::visitEntry( } } - if (after && after->getType() == ltMPTOKEN) + if (after->getType() == ltMPTOKEN) { uint192 const id = getMptID(after); auto& change = changes_[id]; @@ -633,7 +638,7 @@ ValidConfidentialMPToken::visitEntry( change.outstandingDelta, before->getFieldU64(sfOutstandingAmount)); } - if (after && after->getType() == ltMPTOKEN_ISSUANCE) + if (after->getType() == ltMPTOKEN_ISSUANCE) { uint192 const id = getMptID(after); auto& change = changes_[id]; @@ -653,7 +658,7 @@ ValidConfidentialMPToken::visitEntry( change.badCOA = true; } - if (before && after && before->getType() == ltMPTOKEN && after->getType() == ltMPTOKEN) + if (before && before->getType() == ltMPTOKEN && after->getType() == ltMPTOKEN) { uint192 const id = getMptID(after); @@ -799,11 +804,12 @@ ValidConfidentialMPToken::finalize( } void -ValidMPTTransfer::visitEntry( - bool isDelete, - std::shared_ptr const& before, - std::shared_ptr const& after) +ValidMPTTransfer::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + // Record the before/after MPTAmount for each (issuanceID, account) pair // so finalize() can determine whether a transfer actually occurred. auto update = [&](SLE const& sle, bool isBefore) { @@ -830,8 +836,7 @@ ValidMPTTransfer::visitEntry( if (before) update(*before, true); - if (after) - update(*after, false); + update(*after, false); } bool diff --git a/src/libxrpl/tx/invariants/NFTInvariant.cpp b/src/libxrpl/tx/invariants/NFTInvariant.cpp index b3b16010189..35d3ae4f908 100644 --- a/src/libxrpl/tx/invariants/NFTInvariant.cpp +++ b/src/libxrpl/tx/invariants/NFTInvariant.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -25,13 +26,16 @@ namespace xrpl { void -ValidNFTokenPage::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidNFTokenPage::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + static constexpr uint256 const& kPageBits = nft::kPageMask; static constexpr uint256 kAccountBits = ~kPageBits; - if ((before && before->getType() != ltNFTOKEN_PAGE) || - (after && after->getType() != ltNFTOKEN_PAGE)) + if ((before && before->getType() != ltNFTOKEN_PAGE) || after->getType() != ltNFTOKEN_PAGE) return; auto check = [this, isDelete](SLE::const_ref sle) { @@ -107,10 +111,9 @@ ValidNFTokenPage::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_re } } - if (after) - check(after); + check(after); - if (!isDelete && before && after) + if (!isDelete && before) { // If the NFTokenPage // 1. Has a NextMinPage field in before, but loses it in after, and @@ -183,15 +186,18 @@ ValidNFTokenPage::finalize( //------------------------------------------------------------------------------ void -NFTokenCountTracking::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) +NFTokenCountTracking::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + if (before && before->getType() == ltACCOUNT_ROOT) { beforeMintedTotal_ += (*before)[~sfMintedNFTokens].value_or(0); beforeBurnedTotal_ += (*before)[~sfBurnedNFTokens].value_or(0); } - if (after && after->getType() == ltACCOUNT_ROOT) + if (after->getType() == ltACCOUNT_ROOT) { afterMintedTotal_ += (*after)[~sfMintedNFTokens].value_or(0); afterBurnedTotal_ += (*after)[~sfBurnedNFTokens].value_or(0); diff --git a/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp b/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp index 5c53552a3f3..525bfbdfa1a 100644 --- a/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp +++ b/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp @@ -15,16 +15,15 @@ #include #include #include +#include namespace xrpl { void -ValidPermissionedDEX::visitEntry(bool isDelete, SLE::const_ref, SLE::const_ref after) +ValidPermissionedDEX::visitEntry(InvariantEntry const& entry) { - // Post-fixCleanup3_4_0: skip when after is null (defensive). - // Pre-amendment: original after-only path via the `if (after && ...)` checks below. - if (isFeatureEnabled(fixCleanup3_4_0) && !after) - return; + auto const isDelete = entry.isDelete(); + auto const& after = entry.after(); auto trackDomain = [this, isDelete](uint256 const& domain) { domainsOld_.insert(domain); @@ -32,13 +31,13 @@ ValidPermissionedDEX::visitEntry(bool isDelete, SLE::const_ref, SLE::const_ref a domains_.insert(domain); }; - if (after && after->getType() == ltDIR_NODE) + if (after->getType() == ltDIR_NODE) { if (after->isFieldPresent(sfDomainID)) trackDomain(after->getFieldH256(sfDomainID)); } - if (after && after->getType() == ltOFFER) + if (after->getType() == ltOFFER) { if (after->isFieldPresent(sfDomainID)) { diff --git a/src/libxrpl/tx/invariants/PermissionedDomainInvariant.cpp b/src/libxrpl/tx/invariants/PermissionedDomainInvariant.cpp index fcb53e7c8fd..90168cdb022 100644 --- a/src/libxrpl/tx/invariants/PermissionedDomainInvariant.cpp +++ b/src/libxrpl/tx/invariants/PermissionedDomainInvariant.cpp @@ -15,17 +15,22 @@ #include #include #include +#include #include namespace xrpl { void -ValidPermissionedDomain::visitEntry(bool isDel, SLE::const_ref before, SLE::const_ref after) +ValidPermissionedDomain::visitEntry(InvariantEntry const& entry) { + auto const isDel = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + if (before && before->getType() != ltPERMISSIONED_DOMAIN) return; - if (after && after->getType() != ltPERMISSIONED_DOMAIN) + if (after->getType() != ltPERMISSIONED_DOMAIN) return; auto check = [isDel](std::vector& sleStatus, SLE::const_ref sle) { @@ -54,8 +59,7 @@ ValidPermissionedDomain::visitEntry(bool isDel, SLE::const_ref before, SLE::cons sleStatus.emplace_back(ss); }; - if (after) - check(sleStatus_, after); + check(sleStatus_, after); } bool diff --git a/src/libxrpl/tx/invariants/SponsorshipInvariant.cpp b/src/libxrpl/tx/invariants/SponsorshipInvariant.cpp index bdf37492f3b..79ab2e4088e 100644 --- a/src/libxrpl/tx/invariants/SponsorshipInvariant.cpp +++ b/src/libxrpl/tx/invariants/SponsorshipInvariant.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include @@ -17,8 +18,12 @@ namespace xrpl { // Add new sponsorship-related invariants implementations void -SponsorshipOwnerCountsMatch::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +SponsorshipOwnerCountsMatch::visitEntry(InvariantEntry const& entry) { + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); + auto getSponsored = [](SLE::const_ref sle) -> std::uint32_t { if (sle && sle->getType() == ltACCOUNT_ROOT) return sle->getFieldU32(sfSponsoredOwnerCount); @@ -116,8 +121,11 @@ SponsorshipOwnerCountsMatch::finalize( } void -SponsorshipAccountCountMatchesField::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) +SponsorshipAccountCountMatchesField::visitEntry(InvariantEntry const& entry) { + auto const& before = entry.before(); + auto const& after = entry.after(); + auto getSponsoringAccountCount = [](SLE::const_ref sle) -> std::uint32_t { if (sle && sle->getType() == ltACCOUNT_ROOT) return sle->getFieldU32(sfSponsoringAccountCount); diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index 1bfb9d3d436..15462c06bc5 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -83,14 +84,11 @@ ValidVault::Shares::make(SLE const& from) } void -ValidVault::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +ValidVault::visitEntry(InvariantEntry const& entry) { - // If `before` is empty, this means an object is being created, in which - // case `isDelete` must be false. Otherwise `before` and `after` are set and - // `isDelete` indicates whether an object is being deleted or modified. - XRPL_ASSERT( - after != nullptr && (before != nullptr || !isDelete), - "xrpl::ValidVault::visitEntry : some object is available"); + auto const isDelete = entry.isDelete(); + auto const& before = entry.before(); + auto const& after = entry.after(); // Number balanceDelta will capture the difference (delta) between "before" // state (zero if created) and "after" state (zero if destroyed), and @@ -144,7 +142,7 @@ ValidVault::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref afte } } - if (!isDelete && after) + if (!isDelete) { switch (after->getType()) { diff --git a/src/test/app/invariants/InvariantsAMM_test.cpp b/src/test/app/invariants/InvariantsAMM_test.cpp index 498c35c6530..256347938f5 100644 --- a/src/test/app/invariants/InvariantsAMM_test.cpp +++ b/src/test/app/invariants/InvariantsAMM_test.cpp @@ -71,7 +71,10 @@ class InvariantsAMM_test : public InvariantsBase ValidAMM invariant; if (deletedLPBalance) - invariant.visitEntry(true, makeAMM(*deletedLPBalance), nullptr); + { + auto const sleAMM = makeAMM(*deletedLPBalance); + invariant.visitEntry(InvariantEntry{true, sleAMM, sleAMM}); + } bool const actual = invariant.finalize( STTx{txType, [](STObject&) {}}, result, XRPAmount{}, *env.current(), jlog); diff --git a/src/test/app/invariants/InvariantsMisc_test.cpp b/src/test/app/invariants/InvariantsMisc_test.cpp index b0b6c02f5cd..42dfa9bb686 100644 --- a/src/test/app/invariants/InvariantsMisc_test.cpp +++ b/src/test/app/invariants/InvariantsMisc_test.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -49,6 +50,7 @@ #include #include #include +#include #include #include @@ -1183,6 +1185,39 @@ class InvariantsMisc_test : public InvariantsBase } } + void + testInvariantEntry() + { + testcase << "invariant entry validation"; + + SLE::const_pointer const null; + SLE::const_pointer const sle = std::make_shared(keylet::amendments()); + + InvariantEntry const created{false, null, sle}; + BEAST_EXPECT(!created.isDelete()); + BEAST_EXPECT(!created.before()); + BEAST_EXPECT(created.after() == sle); + + auto const expectLogicError = [&](bool isDelete, + SLE::const_ref before, + SLE::const_ref after, + std::string_view message) { + try + { + InvariantEntry const entry{isDelete, before, after}; + (void)entry; + BEAST_EXPECT(false); + } + catch (std::logic_error const& ex) + { + BEAST_EXPECT(std::string_view{ex.what()}.contains(message)); + } + }; + + expectLogicError(false, null, null, "after is never null"); + expectLogicError(true, null, sle, "deleted entry missing before state"); + } + void testTxCheckException() { @@ -1205,7 +1240,7 @@ class InvariantsMisc_test : public InvariantsBase } void - visitEntry(bool, SLE::const_ref, SLE::const_ref) override + visitEntry(InvariantEntry const&) override { if (throwFrom == ThrowFrom::VisitEntry) throw std::runtime_error("test-injected visitEntry exception"); @@ -1269,7 +1304,7 @@ class InvariantsMisc_test : public InvariantsBase struct FailingTxInvariantCheck : TxInvariantCheck { void - visitEntry(bool, SLE::const_ref, SLE::const_ref) override + visitEntry(InvariantEntry const&) override { } @@ -1323,6 +1358,7 @@ class InvariantsMisc_test : public InvariantsBase testInvariantOverwrite(all_ - fixCleanup3_1_3); testObjectHasPseudoAccount(); testSponsorship(); + testInvariantEntry(); testTxCheckException(); testTxCheckFinalizeFalse(); } diff --git a/src/test/app/invariants/InvariantsPermissioned_test.cpp b/src/test/app/invariants/InvariantsPermissioned_test.cpp index 87349fb9e11..8aa6956bd6b 100644 --- a/src/test/app/invariants/InvariantsPermissioned_test.cpp +++ b/src/test/app/invariants/InvariantsPermissioned_test.cpp @@ -640,83 +640,67 @@ class InvariantsPermissioned_test : public InvariantsBase { using namespace test::jtx; - testcase << "PermissionedDEX null after"; + testcase << "PermissionedDEX deleted offer after"; // Tx is OfferCreate on pd2. Tracking pd1 fails the invariant iff that - // domain lands in the set finalize consults. after == null is never - // tracked (pre-340: after-only; post-340: early return) — same result, - // both sides are coverage/regression that we do not fall back to before. - auto const check = [this]( - FeatureBitset features, - bool const afterIsNull, - bool const isDelete, - bool const expectInvariantFailure) { - Env env(*this, features); - - Account const a1{"A1"}; - Account const a2{"A2"}; - env.fund(XRP(1000), a1, a2); - env.close(); - - [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env, a1, a2); - [[maybe_unused]] auto [seq2, pd2] = createPermissionedDomainEnv(env, a1, a2); - env.close(); - - auto sleOffer = - std::make_shared(keylet::offer(a2.id(), SeqProxy::rawSequence(10))); - sleOffer->setAccountID(sfAccount, a2); - sleOffer->setFieldAmount(sfTakerPays, a1["USD"](10)); - sleOffer->setFieldAmount(sfTakerGets, XRP(1)); - sleOffer->setFieldH256(sfDomainID, pd1); - - CurrentTransactionRulesGuard const rulesGuard(env.current()->rules()); - - ValidPermissionedDEX invariant; - if (afterIsNull) - { - // Defensive path: after is null. Must not fall back to before. - invariant.visitEntry(isDelete, sleOffer, nullptr); - } - else - { - // Normal / real-erase path: after is the offer on pd1. - invariant.visitEntry(isDelete, nullptr, sleOffer); - } - - STTx const tx{ttOFFER_CREATE, [&pd2, &a1](STObject& tx) { - tx.setFieldH256(sfDomainID, pd2); - tx.setFieldAmount(sfTakerPays, a1["USD"](10)); - tx.setFieldAmount(sfTakerGets, XRP(1)); - }}; - - test::StreamSink sink{beast::Severity::Warning}; - beast::Journal const jlog{sink}; - bool const passed = - invariant.finalize(tx, tesSUCCESS, XRPAmount{}, *env.current(), jlog); - BEAST_EXPECT(passed != expectInvariantFailure); - if (expectInvariantFailure) - { - BEAST_EXPECT(sink.messages().str().contains("transaction consumed wrong domains")); - } - else - { - BEAST_EXPECT(sink.messages().str().empty()); - } - }; + // domain lands in the set finalize consults. + auto const check = + [this](FeatureBitset features, bool const isDelete, bool const expectInvariantFailure) { + Env env(*this, features); + + Account const a1{"A1"}; + Account const a2{"A2"}; + env.fund(XRP(1000), a1, a2); + env.close(); + + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env, a1, a2); + [[maybe_unused]] auto [seq2, pd2] = createPermissionedDomainEnv(env, a1, a2); + env.close(); + + auto sleOffer = + std::make_shared(keylet::offer(a2.id(), SeqProxy::rawSequence(10))); + sleOffer->setAccountID(sfAccount, a2); + sleOffer->setFieldAmount(sfTakerPays, a1["USD"](10)); + sleOffer->setFieldAmount(sfTakerGets, XRP(1)); + sleOffer->setFieldH256(sfDomainID, pd1); + + CurrentTransactionRulesGuard const rulesGuard(env.current()->rules()); + + ValidPermissionedDEX invariant; + SLE::const_pointer const before = isDelete ? sleOffer : nullptr; + invariant.visitEntry(InvariantEntry{isDelete, before, sleOffer}); + + STTx const tx{ttOFFER_CREATE, [&pd2, &a1](STObject& tx) { + tx.setFieldH256(sfDomainID, pd2); + tx.setFieldAmount(sfTakerPays, a1["USD"](10)); + tx.setFieldAmount(sfTakerGets, XRP(1)); + }}; + + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + bool const passed = + invariant.finalize(tx, tesSUCCESS, XRPAmount{}, *env.current(), jlog); + BEAST_EXPECT(passed != expectInvariantFailure); + if (expectInvariantFailure) + { + BEAST_EXPECT( + sink.messages().str().contains("transaction consumed wrong domains")); + } + else + { + BEAST_EXPECT(sink.messages().str().empty()); + } + }; auto const pre = all_ - fixCleanup3_4_0; auto const post = all_; - // after == null: not tracked - check(pre, true, true, false); - check(post, true, true, false); - // after == offer on pd1 // pre-340: domainsOld_ (delete still inserted) → fail - check(pre, false, true, true); + check(pre, true, true); // post-340: isDelete → only domainsOld_ → pass; !isDelete → domains_ → fail - check(post, false, true, false); - check(post, false, false, true); + check(post, true, false); + check(post, false, true); } void @@ -810,7 +794,8 @@ class InvariantsPermissioned_test : public InvariantsBase view.rawInsert(makeRootPage(rootDir, directoryQuality + 1)); ValidBookDirectory invariant; - invariant.visitEntry(false, nullptr, makeChildPage(rootDir)); + auto const childPage = makeChildPage(rootDir); + invariant.visitEntry(InvariantEntry{false, nullptr, childPage}); test::StreamSink sink{beast::Severity::Warning}; beast::Journal const jlog{sink}; @@ -840,7 +825,7 @@ class InvariantsPermissioned_test : public InvariantsBase { // add ValidBookDirectory invariant; - invariant.visitEntry(false, nullptr, badRoot); + invariant.visitEntry(InvariantEntry{false, nullptr, badRoot}); BEAST_EXPECT( !invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); @@ -848,7 +833,7 @@ class InvariantsPermissioned_test : public InvariantsBase { // modify (without changing the sfRootIndex) ValidBookDirectory invariant; - invariant.visitEntry(false, badRoot, badRoot); + invariant.visitEntry(InvariantEntry{false, badRoot, badRoot}); BEAST_EXPECT( invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); @@ -860,7 +845,7 @@ class InvariantsPermissioned_test : public InvariantsBase childAfter->setFieldH256(sfRootIndex, missingRootDir.key); ValidBookDirectory invariant; - invariant.visitEntry(false, childBefore, childAfter); + invariant.visitEntry(InvariantEntry{false, childBefore, childAfter}); test::StreamSink missingRootSink{beast::Severity::Warning}; beast::Journal const missingRootJlog{missingRootSink}; @@ -875,7 +860,7 @@ class InvariantsPermissioned_test : public InvariantsBase BEAST_EXPECT(!view.exists(rootDir)); ValidBookDirectory invariant; - invariant.visitEntry(true, badRoot, badRoot); + invariant.visitEntry(InvariantEntry{true, badRoot, badRoot}); BEAST_EXPECT( invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); }