diff --git a/include/xrpl/protocol/STParsedJSON.h b/include/xrpl/protocol/STParsedJSON.h index d5b4f33be7..04ffc624fb 100644 --- a/include/xrpl/protocol/STParsedJSON.h +++ b/include/xrpl/protocol/STParsedJSON.h @@ -6,6 +6,13 @@ namespace xrpl { +/** Maximum JSON object nesting depth permitted during parsing. */ +inline constexpr std::size_t kMAX_PARSED_JSON_DEPTH = 64; + +/** Maximum number of elements permitted in any JSON array field during parsing. + Requests exceeding this limit are rejected with an invalidParams error. */ +inline constexpr std::size_t kMAX_PARSED_JSON_ARRAY_SIZE = 512; + /** Holds the serialized result of parsing an input JSON object. This does validation and checking on the provided JSON. */ diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 2c2cd14e97..dc26a5a43b 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -17,7 +17,7 @@ XRPL_FIX (Cleanup3_2_0, Supported::No, VoteBehavior::DefaultNo) XRPL_FEATURE(MPTokensV2, Supported::No, VoteBehavior::DefaultNo) -XRPL_FIX (Security3_1_3, Supported::No, VoteBehavior::DefaultNo) +XRPL_FIX (Cleanup3_1_3, Supported::Yes, VoteBehavior::DefaultYes) XRPL_FIX (PermissionedDomainInvariant, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::No, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::Yes, VoteBehavior::DefaultNo) diff --git a/include/xrpl/shamap/SHAMapTreeNode.h b/include/xrpl/shamap/SHAMapTreeNode.h index d50fc65ccb..ac48df8d46 100644 --- a/include/xrpl/shamap/SHAMapTreeNode.h +++ b/include/xrpl/shamap/SHAMapTreeNode.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -20,6 +21,9 @@ static constexpr unsigned char const kWIRE_TYPE_INNER = 2; static constexpr unsigned char const kWIRE_TYPE_COMPRESSED_INNER = 3; static constexpr unsigned char const kWIRE_TYPE_TRANSACTION_WITH_META = 4; +// Lower bound on SHAMap leaf item payload size, in bytes. +inline constexpr std::size_t kMIN_SHA_MAP_ITEM_BYTES = 12; + enum class SHAMapNodeType { TnInner = 1, TnTransactionNm = 2, // transaction, no metadata diff --git a/include/xrpl/tx/invariants/PermissionedDEXInvariant.h b/include/xrpl/tx/invariants/PermissionedDEXInvariant.h index da187779b2..3784a32840 100644 --- a/include/xrpl/tx/invariants/PermissionedDEXInvariant.h +++ b/include/xrpl/tx/invariants/PermissionedDEXInvariant.h @@ -11,8 +11,8 @@ namespace xrpl { class ValidPermissionedDEX { bool regularOffers_ = false; - bool badHybridsOld_ = false; // pre-fixSecurity3_1_3: missing field/domain or size > 1 - bool badHybrids_ = false; // post-fixSecurity3_1_3: also catches size == 0 (size != 1) + bool badHybridsOld_ = false; // pre-fixCleanup3_1_3: missing field/domain or size > 1 + bool badHybrids_ = false; // post-fixCleanup3_1_3: also catches size == 0 (size != 1) hash_set domains_; public: diff --git a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp index 05e45a404c..cb0cbf6322 100644 --- a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp @@ -61,7 +61,7 @@ removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j) JLOG(j.trace()) << "Credentials are expired. Cred: " << sleCred->getText(); // delete expired credentials even if the transaction failed auto const err = deleteSLE(view, sleCred, j); - if (view.rules().enabled(fixSecurity3_1_3) && !isTesSuccess(err)) + if (view.rules().enabled(fixCleanup3_1_3) && !isTesSuccess(err)) return Unexpected(err); foundExpired = true; } diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index a8bee7aecb..3bfa18e3e8 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -1964,13 +1964,13 @@ loanMakePayment( // ------------------------------------------------------------- // overpayment handling // - // If the "fixSecurity3_1_3" amendment is enabled, truncate "amount", + // If the "fixCleanup3_1_3" amendment is enabled, truncate "amount", // at the loan scale. If the raw value is used, the overpayment // amount could be meaningless dust. Trying to process such a small // amount will, at best, waste time when all the result values round // to zero. At worst, it can cause logical errors with tiny amounts // of interest that don't add up correctly. - auto const roundedAmount = view.rules().enabled(fixSecurity3_1_3) + auto const roundedAmount = view.rules().enabled(fixCleanup3_1_3) ? roundToAsset(asset, amount, loanScale, Number::RoundingMode::TowardsZero) : amount; if (paymentType == LoanPaymentType::Overpayment && loan->isFlag(lsfLoanOverpayment) && diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index 75fe61b34b..252921c499 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp @@ -169,7 +169,7 @@ authorizeMPToken( auto const mptokenKey = keylet::mptoken(mptIssuanceID, account); auto const sleMpt = view.peek(mptokenKey); if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0 || - (view.rules().enabled(fixSecurity3_1_3) && + (view.rules().enabled(fixCleanup3_1_3) && (*sleMpt)[~sfLockedAmount].valueOr(0) != 0)) return tecINTERNAL; // LCOV_EXCL_LINE @@ -284,7 +284,7 @@ removeEmptyHolding( // accounting out of balance, so fail. Since this should be impossible // anyway, I'm not going to put any effort into it. if (mptoken->at(sfMPTAmount) != 0 || - (view.rules().enabled(fixSecurity3_1_3) && (*mptoken)[~sfLockedAmount].valueOr(0) != 0)) + (view.rules().enabled(fixCleanup3_1_3) && (*mptoken)[~sfLockedAmount].valueOr(0) != 0)) return tecHAS_OBLIGATIONS; return authorizeMPToken( diff --git a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp index 7b4194cccf..0151c5b2df 100644 --- a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp +++ b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp @@ -60,9 +60,9 @@ offerInDomain( if (sleOffer->getFieldH256(sfDomainID) != domainID) return false; // LCOV_EXCL_LINE - if (view.rules().enabled(fixSecurity3_1_3)) + if (view.rules().enabled(fixCleanup3_1_3)) { - // post-fixSecurity3_1_3: a valid hybrid offer must have + // post-fixCleanup3_1_3: a valid hybrid offer must have // sfAdditionalBooks present with exactly 1 entry if (sleOffer->isFlag(lsfHybrid) && (!sleOffer->isFieldPresent(sfAdditionalBooks) || @@ -75,7 +75,7 @@ offerInDomain( } else { - // pre-fixSecurity3_1_3: a valid hybrid offer must have + // pre-fixCleanup3_1_3: a valid hybrid offer must have // sfAdditionalBooks present (size is not checked) if (sleOffer->isFlag(lsfHybrid) && !sleOffer->isFieldPresent(sfAdditionalBooks)) { diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index 000f459ef6..0894fdba38 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -1222,9 +1222,9 @@ directSendNoLimitMultiMPT( std::uint64_t const sendAmount = amount.mpt().value(); - if (view.rules().enabled(fixSecurity3_1_3)) + if (view.rules().enabled(fixCleanup3_1_3)) { - // Post-fixSecurity3_1_3: aggregate MaximumAmount + // Post-fixCleanup3_1_3: aggregate MaximumAmount // check. WARNING: the order of conditions is // critical — each guards the subtraction in the // next against unsigned underflow. Do not reorder. @@ -1242,7 +1242,7 @@ directSendNoLimitMultiMPT( } else { - // Pre-fixSecurity3_1_3: per-iteration MaximumAmount + // Pre-fixCleanup3_1_3: per-iteration MaximumAmount // check. Reads sfOutstandingAmount from a stale // view.read() snapshot — incorrect for multi-destination // sends but retained for ledger replay compatibility. diff --git a/src/libxrpl/protocol/STParsedJSON.cpp b/src/libxrpl/protocol/STParsedJSON.cpp index 52d7b4d63e..c6d1d0e805 100644 --- a/src/libxrpl/protocol/STParsedJSON.cpp +++ b/src/libxrpl/protocol/STParsedJSON.cpp @@ -136,6 +136,15 @@ arrayExpected(std::string const& object, std::string const& field) RpcInvalidParams, "Field '" + makeName(object, field) + "' must be a JSON array."); } +static inline json::Value +arrayTooBig(std::string const& object, std::string const& field) +{ + return RPC::makeError( + RpcInvalidParams, + "Field '" + makeName(object, field) + "' exceeds allowed JSON array size of " + + std::to_string(kMAX_PARSED_JSON_ARRAY_SIZE) + " elements per field."); +} + static inline json::Value stringExpected(std::string const& object, std::string const& field) { @@ -681,12 +690,18 @@ parseLeaf( break; case STI_VECTOR256: - if (!value.isArrayOrNull()) + if (not value.isArrayOrNull()) { error = arrayExpected(jsonName, fieldName); return ret; } + if (not value.isNull() and value.size() > kMAX_PARSED_JSON_ARRAY_SIZE) + { + error = arrayTooBig(jsonName, fieldName); + return ret; + } + try { STVector256 tail(field); @@ -708,12 +723,18 @@ parseLeaf( break; case STI_PATHSET: - if (!value.isArrayOrNull()) + if (not value.isArrayOrNull()) { error = arrayExpected(jsonName, fieldName); return ret; } + if (not value.isNull() and value.size() > kMAX_PARSED_JSON_ARRAY_SIZE) + { + error = arrayTooBig(jsonName, fieldName); + return ret; + } + try { STPathSet tail(field); @@ -722,7 +743,7 @@ parseLeaf( { STPath p; - if (!value[i].isArrayOrNull()) + if (not value[i].isArrayOrNull()) { std::stringstream ss; ss << fieldName << "[" << i << "]"; @@ -730,6 +751,14 @@ parseLeaf( return ret; } + if (not value[i].isNull() and value[i].size() > kMAX_PARSED_JSON_ARRAY_SIZE) + { + std::stringstream ss; + ss << fieldName << "[" << i << "]"; + error = arrayTooBig(jsonName, ss.str()); + return ret; + } + for (json::UInt j = 0; value[i].isValidIndex(j); ++j) { std::stringstream ss; @@ -946,8 +975,6 @@ parseLeaf( return ret; } -static int const kMAX_DEPTH = 64; - // Forward declaration since parseObject() and parseArray() call each other. static std::optional parseArray( @@ -965,13 +992,13 @@ parseObject( int depth, json::Value& error) { - if (!json.isObjectOrNull()) + if (not json.isObjectOrNull()) { error = notAnObject(jsonName); return std::nullopt; } - if (depth > kMAX_DEPTH) + if (depth > kMAX_PARSED_JSON_DEPTH) { error = tooDeep(jsonName); return std::nullopt; @@ -984,7 +1011,6 @@ parseObject( for (auto const& fieldName : json.getMemberNames()) { json::Value const& value = json[fieldName]; - auto const& field = SField::getField(fieldName); if (field == kSF_INVALID) @@ -1079,18 +1105,24 @@ parseArray( int depth, json::Value& error) { - if (!json.isArrayOrNull()) + if (not json.isArrayOrNull()) { error = notAnArray(jsonName); return std::nullopt; } - if (depth > kMAX_DEPTH) + if (depth > kMAX_PARSED_JSON_DEPTH) { error = tooDeep(jsonName); return std::nullopt; } + if (not json.isNull() and json.size() > kMAX_PARSED_JSON_ARRAY_SIZE) + { + error = arrayTooBig(jsonName, ""); + return std::nullopt; + } + try { STArray tail(inName); @@ -1108,10 +1140,8 @@ parseArray( } // TODO: There doesn't seem to be a nice way to get just the - // first/only key in an object without copying all keys into - // a vector + // first/only key in an object without copying all keys into a vector std::string const memberName(json[i].getMemberNames()[0]); - ; auto const& nameField(SField::getField(memberName)); if (nameField == kSF_INVALID) diff --git a/src/libxrpl/shamap/SHAMapLeafNode.cpp b/src/libxrpl/shamap/SHAMapLeafNode.cpp index ce41a64e56..84c4498698 100644 --- a/src/libxrpl/shamap/SHAMapLeafNode.cpp +++ b/src/libxrpl/shamap/SHAMapLeafNode.cpp @@ -19,7 +19,7 @@ SHAMapLeafNode::SHAMapLeafNode(boost::intrusive_ptr item, std: : SHAMapTreeNode(cowid), item_(std::move(item)) { XRPL_ASSERT( - item_->size() >= 12, + item_->size() >= kMIN_SHA_MAP_ITEM_BYTES, "xrpl::SHAMapLeafNode::SHAMapLeafNode(boost::intrusive_ptr<" "SHAMapItem const>, std::uint32_t) : minimum input size"); } @@ -31,7 +31,7 @@ SHAMapLeafNode::SHAMapLeafNode( : SHAMapTreeNode(cowid, hash), item_(std::move(item)) { XRPL_ASSERT( - item_->size() >= 12, + item_->size() >= kMIN_SHA_MAP_ITEM_BYTES, "xrpl::SHAMapLeafNode::SHAMapLeafNode(boost::intrusive_ptr<" "SHAMapItem const>, std::uint32_t, SHAMapHash const&) : minimum input " "size"); diff --git a/src/libxrpl/shamap/SHAMapTreeNode.cpp b/src/libxrpl/shamap/SHAMapTreeNode.cpp index ac405f3286..66eced2192 100644 --- a/src/libxrpl/shamap/SHAMapTreeNode.cpp +++ b/src/libxrpl/shamap/SHAMapTreeNode.cpp @@ -28,6 +28,13 @@ namespace xrpl { intr_ptr::SharedPtr SHAMapTreeNode::makeTransaction(Slice data, SHAMapHash const& hash, bool hashValid) { + if (data.size() < kMIN_SHA_MAP_ITEM_BYTES) + { + Throw( + "Short TXN node: " + std::to_string(data.size()) + " bytes (minimum " + + std::to_string(kMIN_SHA_MAP_ITEM_BYTES) + " required)"); + } + auto item = makeShamapitem(sha512Half(HashPrefix::TransactionId, data), data); if (hashValid) @@ -44,14 +51,30 @@ SHAMapTreeNode::makeTransactionWithMeta(Slice data, SHAMapHash const& hash, bool uint256 tag; if (s.size() < tag.kBYTES) - Throw("Short TXN+MD node"); + { + Throw( + "Short TXN+MD node: " + std::to_string(s.size()) + " bytes (minimum " + + std::to_string(tag.kBYTES) + " required for tag)"); + } // FIXME: improve this interface so that the above check isn't needed if (!s.getBitString(tag, s.size() - tag.kBYTES)) - Throw("Short TXN+MD node (" + std::to_string(s.size()) + ")"); + { + Throw( + "Short TXN+MD node: failed to read tag at offset " + + std::to_string(s.size() - tag.kBYTES)); + } s.chop(tag.kBYTES); + if (s.size() < kMIN_SHA_MAP_ITEM_BYTES) + { + Throw( + "Short TXN+MD node: " + std::to_string(s.size()) + + " bytes after tag removal (minimum " + std::to_string(kMIN_SHA_MAP_ITEM_BYTES) + + " required)"); + } + auto item = makeShamapitem(tag, s.slice()); if (hashValid) @@ -68,17 +91,31 @@ SHAMapTreeNode::makeAccountState(Slice data, SHAMapHash const& hash, bool hashVa uint256 tag; if (s.size() < tag.kBYTES) - Throw("short AS node"); + { + Throw( + "Short AS node: " + std::to_string(s.size()) + " bytes (minimum " + + std::to_string(tag.kBYTES) + " required for tag)"); + } // FIXME: improve this interface so that the above check isn't needed if (!s.getBitString(tag, s.size() - tag.kBYTES)) - Throw("Short AS node (" + std::to_string(s.size()) + ")"); + { + Throw( + "Short AS node: failed to read tag at offset " + std::to_string(s.size() - tag.kBYTES)); + } s.chop(tag.kBYTES); if (tag.isZero()) Throw("Invalid AS node"); + if (s.size() < kMIN_SHA_MAP_ITEM_BYTES) + { + Throw( + "Short AS node: " + std::to_string(s.size()) + " bytes after tag removal (minimum " + + std::to_string(kMIN_SHA_MAP_ITEM_BYTES) + " required)"); + } + auto item = makeShamapitem(tag, s.slice()); if (hashValid) diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index e9c32bf14c..59887ad18c 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -341,7 +341,7 @@ NoZeroEscrow::visitEntry( bad_ |= true; }; - bool const overwriteFixEnabled = isFeatureEnabled(fixSecurity3_1_3, true); + bool const overwriteFixEnabled = isFeatureEnabled(fixCleanup3_1_3, true); if (after && after->getType() == ltMPTOKEN_ISSUANCE) { @@ -628,7 +628,7 @@ NoXRPTrustLines::visitEntry( std::shared_ptr const&, std::shared_ptr const& after) { - bool const overwriteFixEnabled = isFeatureEnabled(fixSecurity3_1_3, true); + bool const overwriteFixEnabled = isFeatureEnabled(fixCleanup3_1_3, true); if (after && after->getType() == ltRIPPLE_STATE) { @@ -673,7 +673,7 @@ NoDeepFreezeTrustLinesWithoutFreeze::visitEntry( { if (after && after->getType() == ltRIPPLE_STATE) { - bool const overwriteFixEnabled = isFeatureEnabled(fixSecurity3_1_3, true); + bool const overwriteFixEnabled = isFeatureEnabled(fixCleanup3_1_3, true); std::uint32_t const uFlags = after->getFieldU32(sfFlags); bool const lowFreeze = (uFlags & lsfLowFreeze) != 0u; diff --git a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp index b98d769393..56995ef94c 100644 --- a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp @@ -197,7 +197,7 @@ ValidLoanBroker::finalize( return false; } - if (view.rules().enabled(fixSecurity3_1_3)) + if (view.rules().enabled(fixCleanup3_1_3)) { // Don't check the balance when LoanBroker is deleted, // sfCoverAvailable is not zeroed diff --git a/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp b/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp index 6466812743..b480b90a31 100644 --- a/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp +++ b/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp @@ -41,14 +41,14 @@ ValidPermissionedDEX::visitEntry( regularOffers_ = true; } - // pre-fixSecurity3_1_3: hybrid offer missing domain, missing + // pre-fixCleanup3_1_3: hybrid offer missing domain, missing // sfAdditionalBooks, or sfAdditionalBooks has more than one entry if (after->isFlag(lsfHybrid) && (!after->isFieldPresent(sfDomainID) || !after->isFieldPresent(sfAdditionalBooks) || after->getFieldArray(sfAdditionalBooks).size() > 1)) badHybridsOld_ = true; - // post-fixSecurity3_1_3: same as above but also catches size == 0 + // post-fixCleanup3_1_3: same as above but also catches size == 0 if (after->isFlag(lsfHybrid) && (!after->isFieldPresent(sfDomainID) || !after->isFieldPresent(sfAdditionalBooks) || after->getFieldArray(sfAdditionalBooks).size() != 1)) @@ -70,7 +70,7 @@ ValidPermissionedDEX::finalize( // For each offercreate transaction, check if // permissioned offers are valid - bool const isMalformed = view.rules().enabled(fixSecurity3_1_3) ? badHybrids_ : badHybridsOld_; + bool const isMalformed = view.rules().enabled(fixCleanup3_1_3) ? badHybrids_ : badHybridsOld_; if (txType == ttOFFER_CREATE && isMalformed) { JLOG(j.fatal()) << "Invariant failed: hybrid offer is malformed"; diff --git a/src/libxrpl/tx/transactors/lending/LoanManage.cpp b/src/libxrpl/tx/transactors/lending/LoanManage.cpp index 648f33b980..46c85c9f96 100644 --- a/src/libxrpl/tx/transactors/lending/LoanManage.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanManage.cpp @@ -424,7 +424,7 @@ LoanManage::doApply() // Pre-amendment, associateAsset was only called on the noop (no flags) // path. Post-amendment, we call associateAsset on all successful paths. - if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result)) + if (view.rules().enabled(fixCleanup3_1_3) && isTesSuccess(result)) { associateAsset(*loanSle, vaultAsset); associateAsset(*brokerSle, vaultAsset); diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index d7147128cc..a9668d9f4c 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -147,7 +147,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) std::int64_t constexpr kMAX_FEE_INCREMENTS = kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION / kLOAN_PAYMENTS_PER_FEE_INCREMENT; - if (view.rules().enabled(fixSecurity3_1_3) && + if (view.rules().enabled(fixCleanup3_1_3) && amount >= regularPayment * kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION) { // The payment handler will never process more than @@ -169,7 +169,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) std::int64_t(1), static_cast(numPaymentEstimate / kLOAN_PAYMENTS_PER_FEE_INCREMENT)); XRPL_ASSERT( - !view.rules().enabled(fixSecurity3_1_3) || feeIncrements <= kMAX_FEE_INCREMENTS, + !view.rules().enabled(fixCleanup3_1_3) || feeIncrements <= kMAX_FEE_INCREMENTS, "xrpl::LoanPay::calculateBaseFee : number of fee increments is in " "range"); @@ -201,7 +201,7 @@ LoanPay::preclaim(PreclaimContext const& ctx) if (tx.isFlag(tfLoanOverpayment) && !loanSle->isFlag(lsfLoanOverpayment)) { JLOG(ctx.j.warn()) << "Requested overpayment on a loan that doesn't allow it"; - return ctx.view.rules().enabled(fixSecurity3_1_3) ? TER{tecNO_PERMISSION} : temINVALID_FLAG; + return ctx.view.rules().enabled(fixCleanup3_1_3) ? TER{tecNO_PERMISSION} : temINVALID_FLAG; } auto const principalOutstanding = loanSle->at(sfPrincipalOutstanding); diff --git a/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp b/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp index 2aa2720e46..c46930ba08 100644 --- a/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp +++ b/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp @@ -68,10 +68,10 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration])) { - // Before fixSecurity3_1_3 amendment, expired offers caused tecEXPIRED in preclaim, + // Before fixCleanup3_1_3 amendment, expired offers caused tecEXPIRED in preclaim, // leaving them on ledger forever. After the amendment, we allow expired offers to // reach doApply() where they get deleted and tecEXPIRED is returned. - if (!ctx.view.rules().enabled(fixSecurity3_1_3)) + if (!ctx.view.rules().enabled(fixCleanup3_1_3)) return {nullptr, tecEXPIRED}; // Amendment enabled: return the expired offer to be handled in doApply. } @@ -450,9 +450,9 @@ NFTokenAcceptOffer::doApply() auto bo = loadToken(ctx_.tx[~sfNFTokenBuyOffer]); auto so = loadToken(ctx_.tx[~sfNFTokenSellOffer]); - // With fixSecurity3_1_3 amendment, check for expired offers and delete them, returning + // With fixCleanup3_1_3 amendment, check for expired offers and delete them, returning // tecEXPIRED. This ensures expired offers are properly cleaned up from the ledger. - if (view().rules().enabled(fixSecurity3_1_3)) + if (view().rules().enabled(fixCleanup3_1_3)) { bool foundExpired = false; diff --git a/src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp b/src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp index 39b65d0947..3258dda409 100644 --- a/src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp +++ b/src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp @@ -110,7 +110,7 @@ PermissionedDomainSet::doApply() if (balance < reserve) return tecINSUFFICIENT_RESERVE; - bool const fix313 = view().rules().enabled(fixSecurity3_1_3); + bool const fix313 = view().rules().enabled(fixCleanup3_1_3); auto const seq = fix313 ? ctx_.tx.getSeqValue() : ctx_.tx.getFieldU32(sfSequence); Keylet const pdKeylet = keylet::permissionedDomain(account_, seq); auto slePd = std::make_shared(pdKeylet); diff --git a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp index b6d92a6083..33f4dbf4fa 100644 --- a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp @@ -243,11 +243,11 @@ VaultClawback::assetsToClawback( auto const mptIssuanceID = *vault->at(sfShareMPTID); MPTIssue const share{mptIssuanceID}; - // Pre-fixSecurity3_1_3: zero-amount clawback returned early without + // Pre-fixCleanup3_1_3: zero-amount clawback returned early without // clamping to assetsAvailable, allowing more assets to be recovered // than available when there was an outstanding loan. Retained for // ledger replay compatibility. - if (!ctx_.view().rules().enabled(fixSecurity3_1_3) && clawbackAmount == beast::kZERO) + if (!ctx_.view().rules().enabled(fixCleanup3_1_3) && clawbackAmount == beast::kZERO) { auto const sharesDestroyed = accountHolds( view(), holder, share, FreezeHandling::IgnoreFreeze, AuthHandling::IgnoreAuth, j_); diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 94c6f0f6d2..5b4949ccf6 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -80,9 +80,9 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } - if (ctx.view.rules().enabled(fixSecurity3_1_3) && amount.asset() == vaultShare) + if (ctx.view.rules().enabled(fixCleanup3_1_3) && amount.asset() == vaultShare) { - // Post-fixSecurity3_1_3: if the user specified shares, convert + // Post-fixCleanup3_1_3: if the user specified shares, convert // to the equivalent asset amount before checking withdrawal // limits. Pre-amendment the limit check was skipped for // share-denominated withdrawals. diff --git a/src/test/app/Credentials_test.cpp b/src/test/app/Credentials_test.cpp index fced18c387..1d3f3b3ea8 100644 --- a/src/test/app/Credentials_test.cpp +++ b/src/test/app/Credentials_test.cpp @@ -1036,7 +1036,7 @@ struct Credentials_test : public beast::unit_test::Suite void testRemoveExpiredCorruption(FeatureBitset features) { - bool const fixEnabled = features[fixSecurity3_1_3]; + bool const fixEnabled = features[fixCleanup3_1_3]; testcase( "removeExpired ignores deleteSLE failure " + (fixEnabled ? std::string(" after fix") : std::string(" before fix"))); @@ -1164,8 +1164,8 @@ struct Credentials_test : public beast::unit_test::Suite testFlags(all); testRPC(); - testRemoveExpiredCorruption(all - fixSecurity3_1_3); - testRemoveExpiredCorruption(all | fixSecurity3_1_3); + testRemoveExpiredCorruption(all - fixCleanup3_1_3); + testRemoveExpiredCorruption(all | fixCleanup3_1_3); } }; diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 7fa8a413a2..e2aed9e88b 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -94,7 +94,7 @@ class Invariants_test : public beast::unit_test::Suite static FeatureBitset defaultAmendments() { - return xrpl::test::jtx::testableAmendments() | fixSecurity3_1_3 | fixCleanup3_2_0; + return xrpl::test::jtx::testableAmendments() | fixCleanup3_1_3 | fixCleanup3_2_0; } /** Run a specific test case to put the ledger into a state that will be @@ -1801,7 +1801,7 @@ class Invariants_test : public beast::unit_test::Suite using namespace test::jtx; bool const fixPDEnabled = features[fixPermissionedDomainInvariant]; - bool const fixS313Enabled = features[fixSecurity3_1_3]; + bool const fixS313Enabled = features[fixCleanup3_1_3]; testcase << "PermissionedDEX" + std::string(fixPDEnabled ? " fixPD" : "") + std::string(fixS313Enabled ? " fixS313" : ""); @@ -2387,7 +2387,7 @@ class Invariants_test : public beast::unit_test::Suite } // Test: cover available greater than pseudo-account asset balance - // (requires fixSecurity3_1_3) + // (requires fixCleanup3_1_3) doInvariantCheck( {{"Loan Broker cover available is greater than pseudo-account asset balance"}}, [&](Account const&, Account const&, ApplyContext& ac) { @@ -4116,7 +4116,7 @@ class Invariants_test : public beast::unit_test::Suite testInvariantOverwrite(FeatureBitset features) { using namespace test::jtx; - bool const fixEnabled = features[fixSecurity3_1_3]; + bool const fixEnabled = features[fixCleanup3_1_3]; std::initializer_list const failTers = {tecINVARIANT_FAILED, tefINVARIANT_FAILED}; std::initializer_list const passTers = {tesSUCCESS, tesSUCCESS}; @@ -4395,16 +4395,15 @@ public: testPermissionedDEX(defaultAmendments() | fixPermissionedDomainInvariant); testPermissionedDEX(defaultAmendments() - fixPermissionedDomainInvariant); testPermissionedDEX( - (defaultAmendments() | fixPermissionedDomainInvariant) - fixSecurity3_1_3); - testPermissionedDEX( - defaultAmendments() - fixPermissionedDomainInvariant - fixSecurity3_1_3); + (defaultAmendments() | fixPermissionedDomainInvariant) - fixCleanup3_1_3); + testPermissionedDEX(defaultAmendments() - fixPermissionedDomainInvariant - fixCleanup3_1_3); testNoModifiedUnmodifiableFields(); testValidPseudoAccounts(); testValidLoanBroker(); testVault(); testMPT(); testInvariantOverwrite(defaultAmendments()); - testInvariantOverwrite(defaultAmendments() - fixSecurity3_1_3); + testInvariantOverwrite(defaultAmendments() - fixCleanup3_1_3); testVaultComputeCoarsestScale(); } }; diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 70997b1dcd..ede616bc7c 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -2146,7 +2146,7 @@ protected: Ter(tecNO_PERMISSION)); { - env.disableFeature(fixSecurity3_1_3); + env.disableFeature(fixCleanup3_1_3); env(pay(borrower, loanKeylet.key, STAmount{broker.asset, state.periodicPayment * Number{15, -1}}, @@ -2154,7 +2154,7 @@ protected: Fee(XRPAmount{ baseFee * (Number{15, -1} / kLOAN_PAYMENTS_PER_FEE_INCREMENT + 1)}), Ter(temINVALID_FLAG)); - env.enableFeature(fixSecurity3_1_3); + env.enableFeature(fixCleanup3_1_3); } } // Try to send a payment marked as multiple mutually exclusive @@ -4763,7 +4763,7 @@ protected: void testDosLoanPay(FeatureBitset features) { - bool const feeCapped = features[fixSecurity3_1_3]; + bool const feeCapped = features[fixCleanup3_1_3]; // From FIND-005 testcase << "DoS LoanPay: fee calculation " << (feeCapped ? "capped" : "uncapped"); @@ -4780,7 +4780,7 @@ protected: env.fund(XRP(1'000'000), issuer, lender, borrower); env.close(); - BEAST_EXPECT(feeCapped == env.current()->rules().enabled(fixSecurity3_1_3)); + BEAST_EXPECT(feeCapped == env.current()->rules().enabled(fixCleanup3_1_3)); PrettyAsset const iouAsset = issuer[iouCurrency_]; env(trust(lender, iouAsset(100'000'000))); @@ -7664,8 +7664,8 @@ public: testLoanPayDebtDecreaseInvariant(); testWrongMaxDebtBehavior(); testLoanPayComputePeriodicPaymentValidTotalInterestInvariant(); - testDosLoanPay(all | fixSecurity3_1_3); - testDosLoanPay(all - fixSecurity3_1_3); + testDosLoanPay(all | fixCleanup3_1_3); + testDosLoanPay(all - fixCleanup3_1_3); testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant(); testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant(); testLoanNextPaymentDueDateOverflow(); diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 126778c6a0..36895dd1cf 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -3310,7 +3310,7 @@ class MPToken_test : public beast::unit_test::Suite testMultiSendMaximumAmount(FeatureBitset features) { // Verify that directSendNoLimitMultiMPT correctly enforces MaximumAmount - // when the issuer sends to multiple receivers. Pre-fixSecurity3_1_3, + // when the issuer sends to multiple receivers. Pre-fixCleanup3_1_3, // a stale view.read() snapshot caused per-iteration checks to miss // aggregate overflows. Post-fix, a running total is used instead. testcase("Multi-send MaximumAmount enforcement"); @@ -3412,14 +3412,14 @@ class MPToken_test : public beast::unit_test::Suite // individual send (100 <= 150) even though the aggregate (200) // exceeds MaximumAmount. Preserved for ledger replay. { - // KNOWN BUG (pre-fixSecurity3_1_3): preserved for ledger replay only - env.disableFeature(fixSecurity3_1_3); + // KNOWN BUG (pre-fixCleanup3_1_3): preserved for ledger replay only + env.disableFeature(fixCleanup3_1_3); runTest( R{{alice.id(), 100}, {bob.id(), 100}}, tesSUCCESS, 250, "pre-amendment allows over-send"); - env.enableFeature(fixSecurity3_1_3); + env.enableFeature(fixCleanup3_1_3); } } diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index b81976b83a..5be33fc4e5 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -1095,10 +1095,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The buy offer must not have expired. - // NOTE: this is only a preclaim check with the fixSecurity3_1_3 amendment disabled. + // NOTE: this is only a preclaim check with the fixCleanup3_1_3 amendment disabled. env(token::acceptBuyOffer(alice, buyerExpOfferIndex), Ter(tecEXPIRED)); env.close(); - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { buyerCount--; } @@ -1115,13 +1115,13 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The sell offer must not have expired. - // NOTE: this is only a preclaim check with the fixSecurity3_1_3 amendment disabled. + // NOTE: this is only a preclaim check with the fixCleanup3_1_3 amendment disabled. env(token::acceptSellOffer(buyer, aliceExpOfferIndex), Ter(tecEXPIRED)); env.close(); // Alice's count is decremented by one when the expired offer is // removed. - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { aliceCount--; } @@ -3101,10 +3101,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite // No one can accept an expired sell offer. env(token::acceptSellOffer(buyer, offer1), Ter(tecEXPIRED)); - // With fixSecurity3_1_3 amendment, the first accept + // With fixCleanup3_1_3 amendment, the first accept // attempt deletes the expired offer. Without the amendment, // the offer remains and we can try to accept it again. - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { // After amendment: offer was deleted by first accept attempt minterCount--; @@ -3123,7 +3123,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, minter) == minterCount); BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - if (!features[fixSecurity3_1_3]) + if (!features[fixCleanup3_1_3]) { // Before amendment: expired offer still exists and needs to be // cancelled @@ -3189,10 +3189,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite // An expired buy offer cannot be accepted. env(token::acceptBuyOffer(minter, offer1), Ter(tecEXPIRED)); - // With fixSecurity3_1_3 amendment, the first accept + // With fixCleanup3_1_3 amendment, the first accept // attempt deletes the expired offer. Without the amendment, // the offer remains and we can try to accept it again. - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { // After amendment: offer was deleted by first accept attempt buyerCount--; @@ -3211,7 +3211,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, minter) == minterCount); BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - if (!features[fixSecurity3_1_3]) + if (!features[fixCleanup3_1_3]) { // Before amendment: expired offer still exists and can be // cancelled @@ -3288,7 +3288,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite env(token::brokerOffers(issuer, buyOffer1, sellOffer1), Ter(tecEXPIRED)); env.close(); - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { // With amendment: expired offers are deleted minterCount--; @@ -3298,7 +3298,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, minter) == minterCount); BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { // The buy offer was deleted, so no need to cancel it // The sell offer still exists, so we can cancel it @@ -3377,7 +3377,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); - if (features[fixSecurity3_1_3]) + if (features[fixCleanup3_1_3]) { // After amendment: expired offers were deleted during broker // attempt @@ -3463,7 +3463,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite // The expired offers are still in the ledger. BEAST_EXPECT(ownerCount(env, issuer) == 0); - if (!features[fixSecurity3_1_3]) + if (!features[fixCleanup3_1_3]) { // Before amendment: expired offers still exist in ledger BEAST_EXPECT(ownerCount(env, minter) == 2); @@ -7193,7 +7193,7 @@ public: { testWithFeats( allFeatures_ - fixNFTokenReserve - featureNFTokenMintOffer - featureDynamicNFT - - fixSecurity3_1_3); + fixCleanup3_1_3); } }; @@ -7230,7 +7230,7 @@ class NfTokenWoExpiredOfferRemovalTest : public NFTokenBaseUtil_test void run() override { - testWithFeats(allFeatures_ - fixSecurity3_1_3); + testWithFeats(allFeatures_ - fixCleanup3_1_3); } }; diff --git a/src/test/app/PermissionedDEX_test.cpp b/src/test/app/PermissionedDEX_test.cpp index a0b16aa557..21e6bd8baa 100644 --- a/src/test/app/PermissionedDEX_test.cpp +++ b/src/test/app/PermissionedDEX_test.cpp @@ -1392,13 +1392,12 @@ class PermissionedDEX_test : public beast::unit_test::Suite void testHybridMalformedOffer(FeatureBitset features) { - bool const fixS313Enabled = features[fixSecurity3_1_3]; + bool const fixS313Enabled = features[fixCleanup3_1_3]; testcase << "Hybrid offer with empty AdditionalBooks" - << (fixS313Enabled ? " (fixSecurity3_1_3 enabled)" - : " (fixSecurity3_1_3 disabled)"); + << (fixS313Enabled ? " (fixCleanup3_1_3 enabled)" : " (fixCleanup3_1_3 disabled)"); - // offerInDomain has two code paths gated by fixSecurity3_1_3: + // offerInDomain has two code paths gated by fixCleanup3_1_3: // // pre-fix: only rejects a hybrid offer when sfAdditionalBooks is // entirely absent — an empty array (size 0) passes through. @@ -1425,7 +1424,7 @@ class PermissionedDEX_test : public beast::unit_test::Suite // Directly manipulate the offer SLE in the open ledger so that // sfAdditionalBooks is present but empty (size 0). This is the - // malformed state that fixSecurity3_1_3 is designed to catch. + // malformed state that fixCleanup3_1_3 is designed to catch. auto const offerKey = keylet::offer(bob.id(), bobOfferSeq); env.app().getOpenLedger().modify([&offerKey](OpenView& view, beast::Journal) { auto const sle = view.read(offerKey); @@ -1439,7 +1438,7 @@ class PermissionedDEX_test : public beast::unit_test::Suite if (fixS313Enabled) { - // post-fixSecurity3_1_3: offerInDomain rejects the malformed + // post-fixCleanup3_1_3: offerInDomain rejects the malformed // offer (size == 0), so no valid domain offer is found. env(pay(alice, carol, USD(10)), Path(~USD), @@ -1449,7 +1448,7 @@ class PermissionedDEX_test : public beast::unit_test::Suite } else { - // pre-fixSecurity3_1_3: offerInDomain only checks for a missing + // pre-fixCleanup3_1_3: offerInDomain only checks for a missing // sfAdditionalBooks field; size == 0 passes through, so the // malformed offer is crossed and the payment succeeds. env(pay(alice, carol, USD(10)), Path(~USD), Sendmax(XRP(10)), Domain(domainID)); @@ -1478,7 +1477,7 @@ public: testHybridInvalidOffer(all); testHybridOfferDirectories(all); testHybridMalformedOffer(all); - testHybridMalformedOffer(all - fixSecurity3_1_3); + testHybridMalformedOffer(all - fixCleanup3_1_3); } }; diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index cf79fb1d18..c950739078 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1023,16 +1023,21 @@ public: checkMetrics(*this, env, 0, std::nullopt, 0, 3); // ledgers in queue is 2 because of makeConfig - auto const initQueueMax = initFee(env, 3, 2, 10, 200, 50); + initFee(env, 3, 2, 10, 200, 50); + // Close an empty ledger to shrink queue from the flag-ledger + // size to 2*3=6, independent of amendment count. + env.close(); + constexpr std::size_t kINIT_QUEUE_MAX = 6; + checkMetrics(*this, env, 0, kINIT_QUEUE_MAX, 0, 3); // Create several accounts while the fee is cheap so they all apply. env.fund(drops(2000), noripple(alice)); env.fund(XRP(500000), noripple(bob, charlie, daria)); - checkMetrics(*this, env, 0, initQueueMax, 4, 3); + checkMetrics(*this, env, 0, kINIT_QUEUE_MAX, 4, 3); // Alice - price starts exploding: held env(noop(alice), Fee(11), queued); - checkMetrics(*this, env, 1, initQueueMax, 4, 3); + checkMetrics(*this, env, 1, kINIT_QUEUE_MAX, 4, 3); auto aliceSeq = env.seq(alice); auto bobSeq = env.seq(bob); @@ -1040,28 +1045,28 @@ public: // Alice - try to queue a second transaction, but leave a gap env(noop(alice), Seq(aliceSeq + 2), Fee(100), Ter(telCAN_NOT_QUEUE)); - checkMetrics(*this, env, 1, initQueueMax, 4, 3); + checkMetrics(*this, env, 1, kINIT_QUEUE_MAX, 4, 3); // Alice - queue a second transaction. Yay! env(noop(alice), Seq(aliceSeq + 1), Fee(13), queued); - checkMetrics(*this, env, 2, initQueueMax, 4, 3); + checkMetrics(*this, env, 2, kINIT_QUEUE_MAX, 4, 3); // Alice - queue a third transaction. Yay. env(noop(alice), Seq(aliceSeq + 2), Fee(17), queued); - checkMetrics(*this, env, 3, initQueueMax, 4, 3); + checkMetrics(*this, env, 3, kINIT_QUEUE_MAX, 4, 3); // Bob - queue a transaction env(noop(bob), queued); - checkMetrics(*this, env, 4, initQueueMax, 4, 3); + checkMetrics(*this, env, 4, kINIT_QUEUE_MAX, 4, 3); // Bob - queue a second transaction env(noop(bob), Seq(bobSeq + 1), Fee(50), queued); - checkMetrics(*this, env, 5, initQueueMax, 4, 3); + checkMetrics(*this, env, 5, kINIT_QUEUE_MAX, 4, 3); // Charlie - queue a transaction, with a higher fee // than default env(noop(charlie), Fee(15), queued); - checkMetrics(*this, env, 6, initQueueMax, 4, 3, 257); + checkMetrics(*this, env, 6, kINIT_QUEUE_MAX, 4, 3, 257); BEAST_EXPECT(env.seq(alice) == aliceSeq); BEAST_EXPECT(env.seq(bob) == bobSeq); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index f2c42b7bae..2f32cf605a 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -4916,7 +4916,7 @@ class Vault_test : public beast::unit_test::Suite using namespace loanBroker; using namespace loan; Env env(*this); - env.enableFeature(fixSecurity3_1_3); + env.enableFeature(fixCleanup3_1_3); auto const setupVault = [&](PrettyAsset const& asset, Account const& owner, @@ -5403,14 +5403,14 @@ class Vault_test : public beast::unit_test::Suite env.close(); testCase(mpt, "MPT", owner, depositor, issuer); - // Test pre-fixSecurity3_1_3 legacy path: zero-amount clawback + // Test pre-fixCleanup3_1_3 legacy path: zero-amount clawback // returns early without clamping to assetsAvailable. { testcase( - "VaultClawback (asset) - IOU pre-fixSecurity3_1_3" + "VaultClawback (asset) - IOU pre-fixCleanup3_1_3" " zero-amount clawback unclamped with outstanding loan"); - env.disableFeature(fixSecurity3_1_3); + env.disableFeature(fixCleanup3_1_3); auto [vault, vaultKeylet] = setupVault(iou, owner, depositor, issuer); @@ -5468,7 +5468,7 @@ class Vault_test : public beast::unit_test::Suite BEAST_EXPECT(sharesAfter == sharesBefore); } - env.enableFeature(fixSecurity3_1_3); + env.enableFeature(fixCleanup3_1_3); } } @@ -5867,7 +5867,7 @@ class Vault_test : public beast::unit_test::Suite { testcase("Vault clawback only recovers unlocked shares"); - Env env{*this, testableAmendments() | fixSecurity3_1_3}; + Env env{*this, testableAmendments() | fixCleanup3_1_3}; auto const baseFee = env.current()->fees().base; Account const owner{"owner"}; Account const depositor{"depositor"}; @@ -5957,9 +5957,9 @@ class Vault_test : public beast::unit_test::Suite auto const allAmendments = testableAmendments() | featureSingleAssetVault; - for (auto const& features : {allAmendments, allAmendments - fixSecurity3_1_3}) + for (auto const& features : {allAmendments, allAmendments - fixCleanup3_1_3}) { - bool const withFix = features[fixSecurity3_1_3]; + bool const withFix = features[fixCleanup3_1_3]; Env env{*this, features}; Account const owner{"owner"}; @@ -6117,7 +6117,7 @@ class Vault_test : public beast::unit_test::Suite env.close(); auto const sleMptAfter = env.le(keylet::mptoken(shareMptID, depositor)); - if (!f[fixSecurity3_1_3]) + if (!f[fixCleanup3_1_3]) { // Without the fix, removeEmptyHolding deletes the MPToken // even though sfLockedAmount > 0, leaving the escrow's locked @@ -6137,7 +6137,7 @@ class Vault_test : public beast::unit_test::Suite } }; - runTest(amendments - fixSecurity3_1_3); + runTest(amendments - fixCleanup3_1_3); runTest(amendments); } diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index d98fabecf5..c270ddbe2d 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -744,6 +744,9 @@ TxQ::apply( if (auto directApplied = tryDirectApply(app, view, tx, flags, j)) return *directApplied; + if ((flags & TapDryRun) != 0u) + return {telCAN_NOT_QUEUE, false}; + // If we get past tryDirectApply() without returning then we expect // one of the following to occur: // diff --git a/src/xrpld/rpc/handlers/account/AccountObjects.cpp b/src/xrpld/rpc/handlers/account/AccountObjects.cpp index e17543b339..c544a6ff1a 100644 --- a/src/xrpld/rpc/handlers/account/AccountObjects.cpp +++ b/src/xrpld/rpc/handlers/account/AccountObjects.cpp @@ -59,12 +59,12 @@ getAccountObjects( // iterate NFT pages if the filter says so AND dirIndex == 0 bool iterateNFTPages = (!typeFilter.has_value() || typeMatchesFilter(typeFilter.value(), ltNFTOKEN_PAGE)) && - dirIndex == beast::kZERO; + dirIndex.isZero(); Keylet const firstNFTPage = keylet::nftpageMin(account); // we need to check the marker to see if it is an NFTTokenPage index. - if (iterateNFTPages && entryIndex != beast::kZERO) + if (iterateNFTPages && entryIndex.isNonZero()) { // if it is we will try to iterate the pages up to the limit // and then change over to the owner directory @@ -77,49 +77,44 @@ getAccountObjects( // this is a mutable version of limit, used to seamlessly switch // to iterating directory entries when nftokenpages are exhausted - uint32_t mlimit = limit; + uint32_t limitLeft = limit; // iterate NFTokenPages preferentially if (iterateNFTPages) { Keylet const first = - entryIndex == beast::kZERO ? firstNFTPage : Keylet{ltNFTOKEN_PAGE, entryIndex}; + entryIndex.isZero() ? firstNFTPage : Keylet{ltNFTOKEN_PAGE, entryIndex}; Keylet const last = keylet::nftpageMax(account); - // current key - uint256 ck = ledger.succ(first.key, last.key.next()).value_or(last.key); + auto currentKey = ledger.succ(first.key, last.key.next()).value_or(last.key); - // current page - auto cp = ledger.read(Keylet{ltNFTOKEN_PAGE, ck}); + auto currentPage = ledger.read(Keylet{ltNFTOKEN_PAGE, currentKey}); - while (cp) + while (currentPage) { - jvObjects.append(cp->getJson(JsonOptions::Values::None)); - auto const npm = (*cp)[~sfNextPageMin]; + jvObjects.append(currentPage->getJson(JsonOptions::Values::None)); + auto const npm = (*currentPage)[~sfNextPageMin]; if (npm) { - cp = ledger.read(Keylet(ltNFTOKEN_PAGE, *npm)); + currentPage = ledger.read(Keylet(ltNFTOKEN_PAGE, *npm)); } else { - cp = nullptr; + currentPage = nullptr; } - if (--mlimit == 0) + if (--limitLeft == 0 && currentPage) { - if (cp) - { - jvResult[jss::limit] = limit; - jvResult[jss::marker] = std::string("0,") + to_string(ck); - return true; - } + jvResult[jss::limit] = limit; + jvResult[jss::marker] = std::string("0,") + to_string(currentKey); + return true; } if (!npm) break; - ck = *npm; + currentKey = *npm; } // if execution reaches here then we're about to transition @@ -130,12 +125,12 @@ getAccountObjects( } auto const root = keylet::ownerDir(account); - auto found = false; + auto startEntryFound = false; if (dirIndex.isZero()) { dirIndex = root.key; - found = true; + startEntryFound = true; } auto dir = ledger.read({ltDIR_NODE, dirIndex}); @@ -144,7 +139,7 @@ getAccountObjects( // it's possible the user had nftoken pages but no // directory entries. If there's no nftoken page, we will // give empty array for account_objects. - if (mlimit >= limit) + if (limitLeft >= limit) jvResult[jss::account_objects] = json::ValueType::Array; // non-zero dirIndex validity was checked in the beginning of this @@ -156,33 +151,33 @@ getAccountObjects( return true; } - std::uint32_t i = 0; + std::uint32_t itemsAdded = 0; for (;;) { - auto const& entries = dir->getFieldV256(sfIndexes); - auto iter = entries.begin(); + auto const& dirEntries = dir->getFieldV256(sfIndexes); + auto entryIter = dirEntries.begin(); - if (!found) + if (!startEntryFound) { - iter = std::find(iter, entries.end(), entryIndex); - if (iter == entries.end()) + entryIter = std::find(entryIter, dirEntries.end(), entryIndex); + if (entryIter == dirEntries.end()) return false; - found = true; + startEntryFound = true; } // it's possible that the returned NFTPages exactly filled the // response. Check for that condition. - if (i == mlimit && mlimit < limit) + if (itemsAdded == limitLeft && limitLeft < limit && entryIter != dirEntries.end()) { jvResult[jss::limit] = limit; - jvResult[jss::marker] = to_string(dirIndex) + ',' + to_string(*iter); + jvResult[jss::marker] = to_string(dirIndex) + ',' + to_string(*entryIter); return true; } - for (; iter != entries.end(); ++iter) + for (; entryIter != dirEntries.end(); ++entryIter) { - auto const sleNode = ledger.read(keylet::child(*iter)); + auto const sleNode = ledger.read(keylet::child(*entryIter)); if (!typeFilter.has_value() || typeMatchesFilter(typeFilter.value(), sleNode->getType())) @@ -190,12 +185,12 @@ getAccountObjects( jvObjects.append(sleNode->getJson(JsonOptions::Values::None)); } - if (++i == mlimit) + if (++itemsAdded == limitLeft) { - if (++iter != entries.end()) + if (++entryIter != dirEntries.end()) { jvResult[jss::limit] = limit; - jvResult[jss::marker] = to_string(dirIndex) + ',' + to_string(*iter); + jvResult[jss::marker] = to_string(dirIndex) + ',' + to_string(*entryIter); return true; } @@ -212,13 +207,14 @@ getAccountObjects( if (!dir) return true; - if (i == mlimit) + if (itemsAdded == limitLeft) { - auto const& e = dir->getFieldV256(sfIndexes); - if (!e.empty()) + auto const& currentDirEntries = dir->getFieldV256(sfIndexes); + if (!currentDirEntries.empty()) { jvResult[jss::limit] = limit; - jvResult[jss::marker] = to_string(dirIndex) + ',' + to_string(*e.begin()); + jvResult[jss::marker] = + to_string(dirIndex) + ',' + to_string(*currentDirEntries.begin()); } return true;