diff --git a/include/xrpl/basics/safe_cast.h b/include/xrpl/basics/safe_cast.h index f193f1793f..a750a3b6fc 100644 --- a/include/xrpl/basics/safe_cast.h +++ b/include/xrpl/basics/safe_cast.h @@ -28,9 +28,8 @@ namespace ripple { // the destination can hold all values of the source. This is particularly // handy when the source or destination is an enumeration type. -template -static constexpr bool is_safetocasttovalue_v = - (std::is_integral_v && std::is_integral_v) && +template +concept SafeToCast = (std::is_integral_v && std::is_integral_v) && (std::is_signed::value || std::is_unsigned::value) && (std::is_signed::value != std::is_signed::value ? sizeof(Dest) > sizeof(Src) @@ -78,7 +77,7 @@ inline constexpr std:: unsafe_cast(Src s) noexcept { static_assert( - !is_safetocasttovalue_v, + !SafeToCast, "Only unsafe if casting signed to unsigned or " "destination is too small"); return static_cast(s); 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/Units.h b/include/xrpl/protocol/Units.h index d6085891e6..9459f5cb82 100644 --- a/include/xrpl/protocol/Units.h +++ b/include/xrpl/protocol/Units.h @@ -50,25 +50,32 @@ struct unitlessTag; class BipsTag; class TenthBipsTag; -template -using enable_if_unit_t = typename std::enable_if_t< - std::is_class_v && std::is_object_v && - std::is_object_v>; +// These names don't have to be too descriptive, because we're in the "unit" +// namespace. -/** `is_usable_unit_v` is checked to ensure that only values with +template +concept Valid = std::is_class_v && std::is_object_v && + std::is_object_v; + +/** `Usable` is checked to ensure that only values with known valid type tags can be used (sometimes transparently) in non-unit contexts. At the time of implementation, this includes all known tags, but more may be added in the future, and they should not be added automatically unless determined to be appropriate. */ -template > -constexpr bool is_usable_unit_v = - std::is_same_v || - std::is_same_v || - std::is_same_v || - std::is_same_v || - std::is_same_v; +template +concept Usable = Valid && + (std::is_same_v || + std::is_same_v || + std::is_same_v || + std::is_same_v || + std::is_same_v); + +template +concept Compatible = Valid && std::is_arithmetic_v && + std::is_arithmetic_v && + std::is_convertible_v; template class ValueUnit : private boost::totally_ordered>, @@ -85,25 +92,6 @@ public: private: value_type value_; -protected: - template - static constexpr bool is_compatible_v = - std::is_arithmetic_v && std::is_arithmetic_v && - std::is_convertible_v; - - template > - static constexpr bool is_compatiblevalue_v = - is_compatible_v && - std::is_same_v; - - template - using enable_if_compatible_t = - typename std::enable_if_t>; - - template - using enable_if_compatiblevalue_t = - typename std::enable_if_t>; - public: ValueUnit() = default; constexpr ValueUnit(ValueUnit const& other) = default; @@ -135,12 +123,9 @@ public: /** Instances with the same unit, and a type that is "safe" to convert to this one can be converted implicitly */ - template < - class Other, - class = std::enable_if_t< - is_compatible_v && - is_safetocasttovalue_v>> + template Other> constexpr ValueUnit(ValueUnit const& value) + requires SafeToCast : ValueUnit(safe_cast(value.value())) { } @@ -255,7 +240,7 @@ public: return value_ == other.value_; } - template > + template Other> constexpr bool operator==(ValueUnit const& other) const { @@ -268,7 +253,7 @@ public: return value_ == other; } - template > + template Other> constexpr bool operator!=(ValueUnit const& other) const { @@ -310,12 +295,13 @@ public: return static_cast(value_) / reference.value(); } - // `is_usable_unit_v` is checked to ensure that only values with + // `Usable` is checked to ensure that only values with // known valid type tags can be converted to JSON. At the time // of implementation, that includes all known tags, but more may // be added in the future. - std::enable_if_t, Json::Value> + Json::Value jsonClipped() const + requires Usable { if constexpr (std::is_integral_v) { @@ -372,43 +358,27 @@ to_string(ValueUnit const& amount) return std::to_string(amount.value()); } -template > +template constexpr bool can_muldiv_source_v = std::is_convertible_v; -template > +template constexpr bool can_muldiv_dest_v = can_muldiv_source_v && // Dest is also a source std::is_convertible_v && sizeof(typename Dest::value_type) >= sizeof(std::uint64_t); -template < - class Source1, - class Source2, - class = enable_if_unit_t, - class = enable_if_unit_t> +template constexpr bool can_muldiv_sources_v = can_muldiv_source_v && can_muldiv_source_v && std::is_same_v; -template < - class Source1, - class Source2, - class Dest, - class = enable_if_unit_t, - class = enable_if_unit_t, - class = enable_if_unit_t> +template constexpr bool can_muldiv_v = can_muldiv_sources_v && can_muldiv_dest_v; // Source and Dest can be the same by default -template < - class Source1, - class Source2, - class Dest, - class = enable_if_unit_t, - class = enable_if_unit_t, - class = enable_if_unit_t> +template constexpr bool can_muldiv_commute_v = can_muldiv_v && !std::is_same_v; 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;