Merge remote-tracking branch 'upstream/develop' into mvadari/rearch/delegate-entry

# Conflicts:
#	src/libxrpl/tx/transactors/account/AccountDelete.cpp

.github/dependabot.yml

@@ -36,3 +36,18 @@ updates:
       rust-dependencies:
         patterns:
           - "*"
+
+  - package-ecosystem: uv
+    directory: /cmake/scripts/codegen
+    schedule:
+      interval: weekly
+      day: monday
+      time: "04:00"
+      timezone: Etc/GMT
+    commit-message:
+      prefix: "chore: [DEPENDABOT] "
+    target-branch: develop
+    groups:
+      codegen-dependencies:
+        patterns:
+          - "*"

cmake/scripts/codegen/requirements.in

@@ -10,4 +10,4 @@ pcpp>=1.30
 pyparsing>=3.0.0
 
 # Template engine - used to generate C++ code from templates
-Mako>=1.2.2
+Mako>=1.4.2

cmake/scripts/codegen/requirements.txt

@@ -1,8 +1,8 @@
 # This file was autogenerated by uv via the following command:
 #    uv pip compile requirements.in --generate-hashes --output-file requirements.txt
-mako==1.3.12 \
-    --hash=sha256:8f61569480282dbf557145ce441e4ba888be453c30989f879f0d652e39f53ea9 \
-    --hash=sha256:9f778e93289bd410bb35daadeb4fc66d95a746f0b75777b942088b7fd7af550a
+mako==1.4.3 \
+    --hash=sha256:723296007c870bfd6b3f0c3230dba7198096e5269297ebf5e4eff9e7ffa39d4f \
+    --hash=sha256:cd6537fe88d5fec315c55c2f8529bc4ce7a9a352ad7db3eeaa6a66e2dd4ec37a
     # via -r requirements.in
 markupsafe==3.0.3 \
     --hash=sha256:0303439a41979d9e74d18ff5e2dd8c43ed6c6001fd40e5bf2e43f7bd9bbc523f \

include/xrpl/ledger/entries/DelegateEntry.h

@@ -7,9 +7,16 @@
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
+#include <xrpl/protocol/Permissions.h>
+#include <xrpl/protocol/TER.h>
+#include <xrpl/protocol/TxFormats.h>
+
+#include <unordered_set>
 
 namespace xrpl {
 
+class STTx;
+
 template <typename ViewT>
 class DelegateEntry : public SLEBase<ViewT, ltDELEGATE>
 {
@@ -28,6 +35,41 @@ public:
         : Base(keylet::delegate(account, authorizedAccount), view, j)
     {
     }
+
+    /**
+     * Check if this delegate account has permission to execute the
+     * transaction.
+     * @param tx The transaction that the delegate account intends to
+     * execute.
+     * @return tesSUCCESS if the transaction is allowed,
+     * terNO_DELEGATE_PERMISSION if not.
+     */
+    [[nodiscard]] NotTEC
+    checkTxPermission(STTx const& tx) const;
+
+    /**
+     * Load the granular permissions granted to this delegate account for the
+     * specified transaction type.
+     * @param type Used to determine which granted granular permissions to
+     * load, based on the transaction type.
+     * @return the granted granular permissions tied to the transaction type.
+     */
+    [[nodiscard]] std::unordered_set<GranularPermissionType>
+    granularPermission(TxType const& type) const;
+
+    /**
+     * Removes this Delegate object from the ledger: unlinks it from both
+     * the delegating and (if present) the authorized account's owner
+     * directories, restores the delegating account's owner count, and
+     * erases the entry.
+     * @param owner The account whose owner directory entry led here.
+     * @return tesSUCCESS on success; tecINTERNAL if the entry or the
+     * delegating account's root does not exist; tefBAD_LEDGER if removal
+     * from an owner directory fails.
+     */
+    TER
+    removeFromLedger(AccountID const& owner)
+        requires Base::kIsWritable;
 };
 
 using DelegateEntryR = DelegateEntry<ReadView>;

include/xrpl/ledger/helpers/DelegateHelpers.h

@@ -1,34 +0,0 @@
-#pragma once
-
-#include <xrpl/protocol/Permissions.h>
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/STTx.h>
-#include <xrpl/protocol/TER.h>
-#include <xrpl/protocol/TxFormats.h>
-
-#include <unordered_set>
-
-namespace xrpl {
-
-/**
- * Check if the delegate account has permission to execute the transaction.
- * @param delegate The delegate account.
- * @param tx The transaction that the delegate account intends to execute.
- * @return tesSUCCESS if the transaction is allowed, terNO_DELEGATE_PERMISSION
- * if not.
- */
-NotTEC
-checkTxPermission(SLE::ConstRef delegate, STTx const& tx);
-
-/**
- * Load the granular permissions granted to the delegate account for the
- * specified transaction type
- * @param delegate The delegate account.
- * @param type Used to determine which granted granular permissions to load,
- * based on the transaction type.
- * @return the granted granular permissions tied to the transaction type.
- */
-std::unordered_set<GranularPermissionType>
-getGranularPermission(SLE::ConstRef delegate, TxType const& type);
-
-}  // namespace xrpl

include/xrpl/protocol/STAmount.h

@@ -766,6 +766,21 @@ roundToAsset(
     return roundToScale(ret, scale);
 }
 
+/**
+ * Check whether an arbitrary precision Number can be converted to an STAmount
+ * of a given Asset without exceeding the asset's range.
+ *
+ * Use this before `roundToAsset` or `STAmount{asset, value}` on unchecked
+ * input, since those throw for out-of-range values, e.g. XRP above
+ * `STAmount::kMaxNativeN`.
+ *
+ * @param asset The relevant asset
+ * @param value The value to be checked
+ * @return true if the value is in range for the asset; false otherwise.
+ */
+[[nodiscard]] bool
+isRepresentable(Asset const& asset, Number const& value);
+
 //------------------------------------------------------------------------------
 
 inline bool

include/xrpl/shamap/SHAMapAddNode.h

@@ -14,35 +14,112 @@ private:
 
 public:
     SHAMapAddNode();
+
+    /**
+     * Record one node that was rejected.
+     */
     void
     incInvalid();
+
+    /**
+     * Record one node that produced a good result.
+     */
     void
     incUseful();
+
+    /**
+     * Record one node that was not needed: the map already held it, or the map
+     * or its acquisition was no longer taking nodes. isGood() counts it on the
+     * accepted side.
+     */
     void
     incDuplicate();
-    void
-    reset();
+
+    /**
+     * @return How many nodes produced a good result.
+     */
     [[nodiscard]] int
     getGood() const;
+
+    /**
+     * @return How many nodes this code rejected, which is not how many the
+     *         batch was given: a batch that stops at its first bad node counts
+     *         one, and a batch that carries on counts each.
+     */
+    [[nodiscard]] int
+    getBad() const;
+
+    /**
+     * @return How many nodes were not needed, whether already held or offered
+     *         to a map no longer taking nodes, tallied apart from the good and
+     *         bad counts.
+     */
+    [[nodiscard]] int
+    getDuplicate() const;
+
+    /**
+     * Whether nodes that produced a good result or were not needed outnumber
+     * the ones rejected.
+     *
+     * @return Whether the batch was good.
+     */
     [[nodiscard]] bool
     isGood() const;
+
+    /**
+     * @return Whether at least one node in the batch was rejected.
+     */
     [[nodiscard]] bool
     isInvalid() const;
+
+    /**
+     * @return Whether at least one node in the batch produced a good result.
+     */
     [[nodiscard]] bool
     isUseful() const;
+
+    /**
+     * @return A verdict recording one node that was not needed.
+     */
+    static SHAMapAddNode
+    duplicate();
+
+    /**
+     * @return A verdict recording one useful node.
+     */
+    static SHAMapAddNode
+    useful();
+
+    /**
+     * @return A verdict recording one invalid node.
+     */
+    static SHAMapAddNode
+    invalid();
+
+    /**
+     * Clear every count back to zero.
+     */
+    void
+    reset();
+
+    /**
+     * Render the tally as a log line.
+     *
+     * @return The tally, e.g. "good:2 bad:1 dupe:1", or "no nodes processed" if
+     *         every count is zero.
+     */
     [[nodiscard]] std::string
     get() const;
 
+    /**
+     * Add another verdict's counts into this one.
+     *
+     * @param n The verdict to add.
+     * @return This verdict, updated.
+     */
     SHAMapAddNode&
     operator+=(SHAMapAddNode const& n);
 
-    static SHAMapAddNode
-    duplicate();
-    static SHAMapAddNode
-    useful();
-    static SHAMapAddNode
-    invalid();
-
 private:
     SHAMapAddNode(int good, int bad, int duplicate);
 };
@@ -74,18 +151,30 @@ SHAMapAddNode::incDuplicate()
     ++duplicate_;
 }
 
-inline void
-SHAMapAddNode::reset()
-{
-    good_ = bad_ = duplicate_ = 0;
-}
-
 inline int
 SHAMapAddNode::getGood() const
 {
     return good_;
 }
 
+inline int
+SHAMapAddNode::getBad() const
+{
+    return bad_;
+}
+
+inline int
+SHAMapAddNode::getDuplicate() const
+{
+    return duplicate_;
+}
+
+inline bool
+SHAMapAddNode::isGood() const
+{
+    return (good_ + duplicate_) > bad_;
+}
+
 inline bool
 SHAMapAddNode::isInvalid() const
 {
@@ -98,22 +187,6 @@ SHAMapAddNode::isUseful() const
     return good_ > 0;
 }
 
-inline SHAMapAddNode&
-SHAMapAddNode::operator+=(SHAMapAddNode const& n)
-{
-    good_ += n.good_;
-    bad_ += n.bad_;
-    duplicate_ += n.duplicate_;
-
-    return *this;
-}
-
-inline bool
-SHAMapAddNode::isGood() const
-{
-    return (good_ + duplicate_) > bad_;
-}
-
 inline SHAMapAddNode
 SHAMapAddNode::duplicate()
 {
@@ -132,6 +205,12 @@ SHAMapAddNode::invalid()
     return SHAMapAddNode(0, 1, 0);
 }
 
+inline void
+SHAMapAddNode::reset()
+{
+    good_ = bad_ = duplicate_ = 0;
+}
+
 inline std::string
 SHAMapAddNode::get() const
 {
@@ -160,4 +239,14 @@ SHAMapAddNode::get() const
     return ret;
 }
 
+inline SHAMapAddNode&
+SHAMapAddNode::operator+=(SHAMapAddNode const& n)
+{
+    good_ += n.good_;
+    bad_ += n.bad_;
+    duplicate_ += n.duplicate_;
+
+    return *this;
+}
+
 }  // namespace xrpl

include/xrpl/tx/transactors/delegate/DelegateSet.h

@@ -2,7 +2,6 @@
 
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/core/ServiceRegistry.h>
-#include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/TER.h>
@@ -40,10 +39,6 @@ public:
         XRPAmount fee,
         ReadView const& view,
         beast::Journal const& j) override;
-
-    // Interface used by AccountDelete
-    static TER
-    deleteDelegate(ApplyView& view, SLE::Ref sle, beast::Journal j);
 };
 
 }  // namespace xrpl

src/libxrpl/ledger/entries/DelegateEntry.cpp

@@ -0,0 +1,113 @@
+#include <xrpl/ledger/entries/DelegateEntry.h>
+
+#include <xrpl/basics/Log.h>
+#include <xrpl/ledger/ApplyView.h>
+#include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/helpers/AccountRootHelpers.h>
+#include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Permissions.h>
+#include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STArray.h>  // IWYU pragma: keep
+#include <xrpl/protocol/STTx.h>
+#include <xrpl/protocol/TER.h>
+#include <xrpl/protocol/TxFormats.h>
+
+#include <unordered_set>
+
+namespace xrpl {
+
+template <typename ViewT>
+NotTEC
+DelegateEntry<ViewT>::checkTxPermission(STTx const& tx) const
+{
+    if (!this->exists())
+        return terNO_DELEGATE_PERMISSION;
+
+    auto const& sle = this->operator*();
+    auto const permissionArray = sle.getFieldArray(sfPermissions);
+    auto const txPermission = tx.getTxnType() + 1;
+
+    for (auto const& permission : permissionArray)
+    {
+        auto const permissionValue = permission[sfPermissionValue];
+        if (permissionValue == txPermission)
+            return tesSUCCESS;
+    }
+
+    return terNO_DELEGATE_PERMISSION;
+}
+
+template <typename ViewT>
+std::unordered_set<GranularPermissionType>
+DelegateEntry<ViewT>::granularPermission(TxType const& type) const
+{
+    std::unordered_set<GranularPermissionType> granularPermissions;
+    if (!this->exists())
+        return granularPermissions;
+
+    auto const& sle = this->operator*();
+    auto const permissionArray = sle.getFieldArray(sfPermissions);
+    for (auto const& permission : permissionArray)
+    {
+        auto const permissionValue = permission[sfPermissionValue];
+        auto const granularValue = static_cast<GranularPermissionType>(permissionValue);
+        auto const& permType = Permission::getInstance().getGranularTxType(granularValue);
+        if (permType && *permType == type)
+            granularPermissions.insert(granularValue);
+    }
+
+    return granularPermissions;
+}
+
+template <typename ViewT>
+TER
+DelegateEntry<ViewT>::removeFromLedger(AccountID const&)
+    requires Base::kIsWritable
+{
+    if (!this->exists())
+        return tecINTERNAL;  // LCOV_EXCL_LINE
+
+    auto& sle = this->operator*();
+    auto const delegator = sle[sfAccount];
+    auto const delegatee = sle[sfAuthorize];
+
+    // Remove from delegating account's owner directory
+    if (!this->applyView().dirRemove(
+            keylet::ownerDir(delegator), sle[sfOwnerNode], this->key(), false))
+    {
+        // LCOV_EXCL_START
+        JLOG(this->journal().fatal()) << "Unable to delete Delegate from owner.";
+        return tefBAD_LEDGER;
+        // LCOV_EXCL_STOP
+    }
+
+    // Remove from authorized account's owner directory, if present
+    if (auto const optPage = sle[~sfDestinationNode])
+    {
+        if (!this->applyView().dirRemove(keylet::ownerDir(delegatee), *optPage, this->key(), false))
+        {
+            // LCOV_EXCL_START
+            JLOG(this->journal().fatal()) << "Unable to delete Delegate from authorized account.";
+            return tefBAD_LEDGER;
+            // LCOV_EXCL_STOP
+        }
+    }
+
+    // Only the delegating account's owner count was incremented on creation
+    auto const sleOwner = this->applyView().peek(keylet::account(delegator));
+    if (!sleOwner)
+        return tecINTERNAL;  // LCOV_EXCL_LINE
+
+    decreaseOwnerCountForObject(
+        this->applyView(), sleOwner, this->mutableRawSle(), 1, this->journal());
+
+    this->erase();
+
+    return tesSUCCESS;
+}
+
+template class DelegateEntry<ReadView>;
+template class DelegateEntry<ApplyView>;
+
+}  // namespace xrpl

src/libxrpl/protocol/STAmount.cpp

@@ -443,6 +443,22 @@ getRate(STAmount const& offerOut, STAmount const& offerIn)
     }
 }
 
+bool
+isRepresentable(Asset const& asset, Number const& value)
+{
+    try
+    {
+        // Out-of-range values throw std::overflow_error from Number or
+        // IOUAmount, or std::runtime_error from STAmount::canonicalize.
+        [[maybe_unused]] STAmount const amount{asset, value};
+        return true;
+    }
+    catch (std::runtime_error const&)
+    {
+        return false;
+    }
+}
+
 /**
  * @brief Safely checks if two STAmount values can be added without overflow,
  * underflow, or precision loss.

src/libxrpl/tx/Transactor.cpp

@@ -11,9 +11,9 @@
 #include <xrpl/json/to_string.h>  // IWYU pragma: keep
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
-#include <xrpl/ledger/helpers/DelegateHelpers.h>
 #include <xrpl/ledger/helpers/NFTokenHelpers.h>
 #include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/ledger/helpers/RippleStateHelpers.h>
@@ -388,17 +388,17 @@ Transactor::checkPermission(
     if (!delegate)
         return tesSUCCESS;
 
-    auto const sle = view.read(keylet::delegate(tx[sfAccount], *delegate));
-    if (!sle)
+    DelegateEntryR const delegateEntry(tx[sfAccount], *delegate, view);
+    if (!delegateEntry)
         return terNO_DELEGATE_PERMISSION;
 
-    if (isTesSuccess(checkTxPermission(sle, tx)))
+    if (isTesSuccess(delegateEntry.checkTxPermission(tx)))
         return tesSUCCESS;
 
     if (!Permission::getInstance().hasGranularPermissions(tx.getTxnType()))
         return terNO_DELEGATE_PERMISSION;
 
-    heldGranularPermissions = getGranularPermission(sle, tx.getTxnType());
+    heldGranularPermissions = delegateEntry.granularPermission(tx.getTxnType());
     if (heldGranularPermissions.empty())
         return terNO_DELEGATE_PERMISSION;
 

src/libxrpl/tx/transactors/account/AccountDelete.cpp

@@ -9,6 +9,7 @@
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/View.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/ledger/entries/DepositPreauthEntry.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
@@ -27,7 +28,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/transactors/account/SignerListSet.h>
-#include <xrpl/tx/transactors/delegate/DelegateSet.h>
 #include <xrpl/tx/transactors/did/DIDDelete.h>
 #include <xrpl/tx/transactors/oracle/OracleDelete.h>
 #include <xrpl/tx/transactors/payment/DepositPreauth.h>
@@ -178,12 +178,13 @@ TER
 removeDelegateFromLedger(
     ServiceRegistry&,
     ApplyView& view,
-    AccountID const&,
-    UInt256 const&,
-    SLE::Ref sleDel,
+    AccountID const& account,
+    UInt256 const& delIndex,
+    SLE::Ref,
     beast::Journal j)
 {
-    return DelegateSet::deleteDelegate(view, sleDel, j);
+    DelegateEntryW entry(Keylet(ltDELEGATE, delIndex), view, j);
+    return entry.removeFromLedger(account);
 }
 
 // Return nullptr if the LedgerEntryType represents an obligation that can't

src/libxrpl/tx/transactors/delegate/DelegateSet.cpp

@@ -1,8 +1,8 @@
 #include <xrpl/tx/transactors/delegate/DelegateSet.h>
 
-#include <xrpl/basics/Log.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/core/ServiceRegistry.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/ledger/helpers/SponsorHelpers.h>
@@ -16,7 +16,6 @@
 #include <xrpl/tx/Transactor.h>
 
 #include <cstdint>
-#include <memory>
 #include <unordered_set>
 
 namespace xrpl {
@@ -77,20 +76,19 @@ DelegateSet::doApply()
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
     auto const& authAccount = ctx_.tx[sfAuthorize];
-    auto const delegateKey = keylet::delegate(accountID_, authAccount);
+    DelegateEntryW entry(accountID_, authAccount, ctx_.view(), j_);
 
-    auto sle = ctx_.view().peek(delegateKey);
-    if (sle)
+    if (entry.exists())
     {
         auto const& permissions = ctx_.tx.getFieldArray(sfPermissions);
         if (permissions.empty())
         {
             // if permissions array is empty, delete the ledger object.
-            return deleteDelegate(view(), sle, j_);
+            return entry.removeFromLedger(accountID_);
         }
 
-        sle->setFieldArray(sfPermissions, permissions);
-        ctx_.view().update(sle);
+        entry->setFieldArray(sfPermissions, permissions);
+        entry.update();
         return tesSUCCESS;
     }
 
@@ -107,76 +105,34 @@ DelegateSet::doApply()
         !isTesSuccess(ret))
         return ret;
 
-    sle = std::make_shared<SLE>(delegateKey);
-    sle->setAccountID(sfAccount, accountID_);
-    sle->setAccountID(sfAuthorize, authAccount);
+    entry.newSLE();
+    entry->setAccountID(sfAccount, accountID_);
+    entry->setAccountID(sfAuthorize, authAccount);
 
-    sle->setFieldArray(sfPermissions, permissions);
+    entry->setFieldArray(sfPermissions, permissions);
 
     // Add to delegating account's owner directory
     auto const page = ctx_.view().dirInsert(
-        keylet::ownerDir(accountID_), delegateKey, describeOwnerDir(accountID_));
+        keylet::ownerDir(accountID_), entry.keylet(), describeOwnerDir(accountID_));
 
     if (!page)
         return tecDIR_FULL;  // LCOV_EXCL_LINE
 
-    (*sle)[sfOwnerNode] = *page;
+    (*entry)[sfOwnerNode] = *page;
 
     // Add to authorized account's owner directory so AccountDelete can find
     // and clean up inbound delegations when the authorized account is deleted.
     auto const destPage = ctx_.view().dirInsert(
-        keylet::ownerDir(authAccount), delegateKey, describeOwnerDir(authAccount));
+        keylet::ownerDir(authAccount), entry.keylet(), describeOwnerDir(authAccount));
 
     if (!destPage)
         return tecDIR_FULL;  // LCOV_EXCL_LINE
 
-    (*sle)[sfDestinationNode] = *destPage;
+    (*entry)[sfDestinationNode] = *destPage;
 
-    ctx_.view().insert(sle);
+    entry.insert();
     increaseOwnerCount(ctx_.getApplyViewContext(), sleOwner, 1, ctx_.journal);
-    addSponsorToLedgerEntry(ctx_.getApplyViewContext(), sle);
-
-    return tesSUCCESS;
-}
-
-TER
-DelegateSet::deleteDelegate(ApplyView& view, SLE::Ref sle, beast::Journal j)
-{
-    if (!sle)
-        return tecINTERNAL;  // LCOV_EXCL_LINE
-
-    auto const delegator = (*sle)[sfAccount];
-    auto const delegatee = (*sle)[sfAuthorize];
-
-    // Remove from delegating account's owner directory
-    if (!view.dirRemove(keylet::ownerDir(delegator), (*sle)[sfOwnerNode], sle->key(), false))
-    {
-        // LCOV_EXCL_START
-        JLOG(j.fatal()) << "Unable to delete Delegate from owner.";
-        return tefBAD_LEDGER;
-        // LCOV_EXCL_STOP
-    }
-
-    // Remove from authorized account's owner directory, if present
-    if (auto const optPage = (*sle)[~sfDestinationNode])
-    {
-        if (!view.dirRemove(keylet::ownerDir(delegatee), *optPage, sle->key(), false))
-        {
-            // LCOV_EXCL_START
-            JLOG(j.fatal()) << "Unable to delete Delegate from authorized account.";
-            return tefBAD_LEDGER;
-            // LCOV_EXCL_STOP
-        }
-    }
-
-    // Only the delegating account's owner count was incremented on creation
-    auto const sleOwner = view.peek(keylet::account(delegator));
-    if (!sleOwner)
-        return tecINTERNAL;  // LCOV_EXCL_LINE
-
-    decreaseOwnerCountForObject(view, sleOwner, sle, 1, j);
-
-    view.erase(sle);
+    addSponsorToLedgerEntry(ctx_.getApplyViewContext(), entry.mutableRawSle());
 
     return tesSUCCESS;
 }

src/libxrpl/tx/transactors/delegate/DelegateUtils.cpp

@@ -1,52 +0,0 @@
-#include <xrpl/ledger/helpers/DelegateHelpers.h>
-#include <xrpl/protocol/Permissions.h>
-#include <xrpl/protocol/SField.h>
-#include <xrpl/protocol/STArray.h>
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/STTx.h>
-#include <xrpl/protocol/TER.h>
-#include <xrpl/protocol/TxFormats.h>
-
-#include <unordered_set>
-
-namespace xrpl {
-NotTEC
-checkTxPermission(SLE::ConstRef delegate, STTx const& tx)
-{
-    if (!delegate)
-        return terNO_DELEGATE_PERMISSION;
-
-    auto const permissionArray = delegate->getFieldArray(sfPermissions);
-    auto const txPermission = tx.getTxnType() + 1;
-
-    for (auto const& permission : permissionArray)
-    {
-        auto const permissionValue = permission[sfPermissionValue];
-        if (permissionValue == txPermission)
-            return tesSUCCESS;
-    }
-
-    return terNO_DELEGATE_PERMISSION;
-}
-
-std::unordered_set<GranularPermissionType>
-getGranularPermission(SLE::ConstRef delegate, TxType const& txType)
-{
-    std::unordered_set<GranularPermissionType> granularPermissions;
-    if (!delegate)
-        return granularPermissions;
-
-    auto const permissionArray = delegate->getFieldArray(sfPermissions);
-    for (auto const& permission : permissionArray)
-    {
-        auto const permissionValue = permission[sfPermissionValue];
-        auto const granularValue = static_cast<GranularPermissionType>(permissionValue);
-        auto const& type = Permission::getInstance().getGranularTxType(granularValue);
-        if (type && *type == txType)
-            granularPermissions.insert(granularValue);
-    }
-
-    return granularPermissions;
-}
-
-}  // namespace xrpl

src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp

@@ -95,20 +95,40 @@ EscrowFinish::preflightSigValidated(PreflightContext const& ctx)
         auto& router = ctx.registry.get().getHashRouter();
 
         auto const id = ctx.tx.getTransactionID();
-        auto const flags = router.getFlags(id);
+        auto flags = router.getFlags(id);
 
         // If we haven't checked the condition, check it
-        // now. Whether it passes or not isn't important
-        // in preflight.
+        // now. Other than a malformed condition, whether
+        // it passes or not isn't important in preflight.
         if (!any(flags & (kSfCfInvalid | kSfCfValid)))
         {
             if (checkCondition(*fb, *cb))
             {
                 router.setFlags(id, kSfCfValid);
+                flags = kSfCfValid;
             }
             else
             {
                 router.setFlags(id, kSfCfInvalid);
+                flags = kSfCfInvalid;
+            }
+        }
+
+        // The invalid flag also covers a bad fulfillment, so
+        // check the condition itself. This runs whether or not
+        // the flag was cached, so the result is the same on
+        // every node.
+        if (any(flags & kSfCfInvalid) && ctx.rules.enabled(fixCleanup3_5_0))
+        {
+            using namespace xrpl::cryptoconditions;
+
+            std::error_code ec;
+
+            auto const condition = Condition::deserialize(*cb, ec);
+            if (!condition)
+            {
+                JLOG(ctx.j.debug()) << "Malformed condition during escrow finish: " << ec.message();
+                return temMALFORMED;
             }
         }
     }

src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp

@@ -16,6 +16,7 @@
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/STTakesAsset.h>
@@ -223,10 +224,15 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx)
     }
 
     // Check that relevant values can be represented as the vault asset
-    // type. This is mostly only relevant for integral (non-IOU) types
+    // type. This is mostly only relevant for integral (non-IOU) types.
+    // fixCleanup3_5_0 also rejects out-of-range values, e.g. XRP above
+    // STAmount::kMaxNativeN, which would otherwise throw.
+    bool const fix350Enabled = ctx.view.rules().enabled(fixCleanup3_5_0);
     for (auto const& field : getValueFields())
     {
-        if (auto const value = tx[field]; value && STAmount{asset, *value} != *value)
+        if (auto const value = tx[field]; value &&
+            ((fix350Enabled && !isRepresentable(asset, *value)) ||
+             STAmount{asset, *value} != *value))
         {
             JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value
                                << ") can not be represented as a(n) " << to_string(asset) << ".";

src/libxrpl/tx/transactors/lending/LoanSet.cpp

@@ -20,6 +20,7 @@
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STNumber.h>
 #include <xrpl/protocol/STObject.h>
@@ -366,10 +367,14 @@ LoanSet::preclaim(PreclaimContext const& ctx)
     // Check that relevant values can be represented as the vault asset type.
     // This check is almost duplicated in doApply, but that check is done after
     // the overall loan scale is known. This is mostly only relevant for
-    // integral (non-IOU) types
+    // integral (non-IOU) types. fixCleanup3_5_0 also rejects out-of-range
+    // values, e.g. XRP above STAmount::kMaxNativeN, which would otherwise throw.
+    bool const fix350Enabled = ctx.view.rules().enabled(fixCleanup3_5_0);
     for (auto const& field : getValueFields())
     {
-        if (auto const value = tx[field]; value && STAmount{asset, *value} != *value)
+        if (auto const value = tx[field]; value &&
+            ((fix350Enabled && !isRepresentable(asset, *value)) ||
+             STAmount{asset, *value} != *value))
         {
             JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value
                                << ") can not be represented as a(n) " << to_string(asset) << ".";

src/libxrpl/tx/transactors/vault/VaultCreate.cpp

@@ -18,6 +18,7 @@
 #include <xrpl/protocol/MPTIssue.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STNumber.h>  // IWYU pragma: keep
 #include <xrpl/protocol/STTakesAsset.h>
@@ -87,6 +88,11 @@ VaultCreate::preflight(PreflightContext const& ctx)
     {
         if (*assetMax < beast::kZero)
             return temMALFORMED;
+
+        // An AssetsMaximum out of range for the asset, e.g. XRP above
+        // STAmount::kMaxNativeN, would throw when rounded to the asset.
+        if (ctx.rules.enabled(fixCleanup3_5_0) && !isRepresentable(ctx.tx[sfAsset], *assetMax))
+            return temMALFORMED;
     }
 
     if (auto const metadata = ctx.tx[~sfMPTokenMetadata])

src/libxrpl/tx/transactors/vault/VaultSet.cpp

@@ -7,6 +7,7 @@
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STNumber.h>  // IWYU pragma: keep
 #include <xrpl/protocol/STTakesAsset.h>
@@ -74,6 +75,16 @@ VaultSet::preclaim(PreclaimContext const& ctx)
         return tecNO_PERMISSION;
     }
 
+    // An AssetsMaximum out of range for the vault asset, e.g. XRP above
+    // STAmount::kMaxNativeN, would throw when rounded to the asset.
+    if (auto const assetMax = ctx.tx[~sfAssetsMaximum]; assetMax &&
+        ctx.view.rules().enabled(fixCleanup3_5_0) &&
+        !isRepresentable(vault->at(sfAsset), *assetMax))
+    {
+        JLOG(ctx.j.debug()) << "VaultSet: max assets out of range for the asset.";
+        return tecPRECISION_LOSS;
+    }
+
     auto const mptIssuanceID = (*vault)[sfShareMPTID];
     auto const sleIssuance = ctx.view.read(keylet::mptokenIssuance(mptIssuanceID));
     if (!sleIssuance)

src/test/app/Delegate_test.cpp

@@ -37,13 +37,14 @@
 #include <xrpl/json/json_value.h>
 #include <xrpl/json/to_string.h>
 #include <xrpl/ledger/Dir.h>
-#include <xrpl/ledger/helpers/DelegateHelpers.h>
+#include <xrpl/ledger/entries/DelegateEntry.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/KeyType.h>
 #include <xrpl/protocol/Permissions.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STObject.h>
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/SecretKey.h>
@@ -3111,12 +3112,16 @@ class Delegate_test : public beast::unit_test::Suite
     {
         testcase("DelegateUtils nullptr check");
 
+        using namespace jtx;
+        Env const env(*this);
+        DelegateEntryR const delegate(SLE::const_pointer(), *env.current());
+
         // checkTxPermission nullptr check
         STTx const tx{ttPAYMENT, [](STObject&) {}};
-        BEAST_EXPECT(checkTxPermission(nullptr, tx) == terNO_DELEGATE_PERMISSION);
+        BEAST_EXPECT(delegate.checkTxPermission(tx) == terNO_DELEGATE_PERMISSION);
 
-        // getGranularPermission nullptr check
-        auto const granularPermissions = getGranularPermission(nullptr, ttPAYMENT);
+        // granularPermission nullptr check
+        auto const granularPermissions = delegate.granularPermission(ttPAYMENT);
         BEAST_EXPECT(granularPermissions.empty());
     }
 

src/test/app/Escrow_test.cpp

@@ -941,41 +941,43 @@ struct Escrow_test : public beast::unit_test::Suite
 
             // Now, try to fulfill using the same sequence of
             // malformed conditions.
+            TER const malformedCondTer =
+                features[fixCleanup3_5_0] ? TER{temMALFORMED} : TER{tecCRYPTOCONDITION_ERROR};
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp, cs}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp, cs - 1}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp, cs - 2}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp + 1, cs - 1}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp + 1, cs - 3}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp + 2, cs - 2}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{cp + 2, cs - 3}),
                 escrow::kFulfillment(Slice{fp, fs}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(malformedCondTer));
 
             // Now, using the correct kCondition, try malformed fulfillments:
             env(escrow::finish("bob", "alice", seq),
@@ -1043,11 +1045,13 @@ struct Escrow_test : public beast::unit_test::Suite
                 escrow::kCondition(escrow::kCb3),
                 escrow::kCancelTime(env.now() + 1s));
 
+            TER const emptyCondTer =
+                features[fixCleanup3_5_0] ? TER{temMALFORMED} : TER{tecCRYPTOCONDITION_ERROR};
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(Slice{}),
                 escrow::kFulfillment(Slice{}),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(emptyCondTer));
             env(escrow::finish("bob", "alice", seq),
                 escrow::kCondition(escrow::kCb3),
                 escrow::kFulfillment(Slice{}),
@@ -1057,7 +1061,7 @@ struct Escrow_test : public beast::unit_test::Suite
                 escrow::kCondition(Slice{}),
                 escrow::kFulfillment(escrow::kFb3),
                 Fee(150 * baseFee),
-                Ter(tecCRYPTOCONDITION_ERROR));
+                Ter(emptyCondTer));
 
             // Assemble finish that is missing the Condition or the Fulfillment
             // since either both must be present, or neither can:
@@ -1097,6 +1101,41 @@ struct Escrow_test : public beast::unit_test::Suite
                 escrow::kCancelTime(env.now() + 1s),
                 Ter(temMALFORMED));
         }
+        {  // Test finish with a malformed condition
+            Env env(*this, features);
+            env.fund(XRP(5000), "alice", "bob", "carol");
+            env.close();
+
+            auto const seq = env.seq("alice");
+            env(escrow::create("alice", "carol", XRP(1000)),
+                escrow::kCondition(escrow::kCb1),
+                escrow::kCancelTime(env.now() + 100s));
+            env.close();
+
+            // Does not deserialize as a condition.
+            std::array<std::uint8_t, 3> const malformed = {{0x78, 0x78, 0x78}};
+
+            auto const baseFee = env.current()->fees().base;
+            auto const bobBalance = env.balance("bob");
+            bool const fixEnabled = features[fixCleanup3_5_0];
+            auto const jt = env.jt(
+                escrow::finish("bob", "alice", seq),
+                escrow::kCondition(malformed),
+                escrow::kFulfillment(escrow::kFb1),
+                Fee(150 * baseFee));
+            env(jt, Ter(fixEnabled ? TER{temMALFORMED} : TER{tecCRYPTOCONDITION_ERROR}));
+            // Run preflight on the same transaction again, now with the
+            // HashRouter flag cached. The result must not change.
+            auto const pf =
+                preflight(env.app(), env.current()->rules(), *jt.stx, TapNone, env.journal);
+            BEAST_EXPECT(pf.ter == (fixEnabled ? TER{temMALFORMED} : TER{tesSUCCESS}));
+            env.close();
+
+            // Before the fix, the transaction is included and charges a fee.
+            env.require(
+                Balance("bob", fixEnabled ? bobBalance : bobBalance - drops(150 * baseFee)));
+            BEAST_EXPECT(env.le(keylet::escrow(Account("alice").id(), SeqProxy::rawSequence(seq))));
+        }
     }
 
     void
@@ -1642,6 +1681,7 @@ public:
         FeatureBitset const all{testableAmendments()};
         testWithFeats(all);
         testWithFeats(all - featureTokenEscrow);
+        testWithFeats(all - fixCleanup3_5_0);
         testTags(all - fixIncludeKeyletFields);
     }
 };

src/test/app/lending/LoanBroker_test.cpp

@@ -3369,6 +3369,42 @@ class LoanBroker_test : public beast::unit_test::Suite
     }
 
 public:
+    void
+    testDebtMaximumOutOfRange(FeatureBitset features)
+    {
+        using namespace jtx;
+        using namespace loan_broker;
+
+        bool const withFix = features[fixCleanup3_5_0];
+        testcase(
+            std::string("DebtMaximum out of range for the asset (") + (withFix ? "post" : "pre") +
+            "-fixCleanup3_5_0)");
+
+        Env env(*this, features);
+        Account const alice{"alice"};
+        env.fund(XRP(100'000), alice);
+        env.close();
+
+        Vault const vault{env};
+        auto const [tx, keylet] = vault.create({.owner = alice, .asset = xrpIssue()});
+        env(tx);
+        env.close();
+
+        // Without the fix, the representability check in preclaim throws for
+        // XRP above STAmount::kMaxNativeN.
+        auto const ownerCount = env.ownerCount(alice);
+        Number const maxXrp{static_cast<std::int64_t>(STAmount::kMaxNativeN)};
+        env(set(alice, keylet.key),
+            kDebtMaximum(maxXrp + 1),
+            Ter(withFix ? TER{tecPRECISION_LOSS} : TER{tefEXCEPTION}));
+        env.close();
+        BEAST_EXPECT(env.ownerCount(alice) == ownerCount);
+
+        env(set(alice, keylet.key), kDebtMaximum(maxXrp));
+        env.close();
+        BEAST_EXPECT(env.ownerCount(alice) > ownerCount);
+    }
+
     void
     run() override
     {
@@ -3409,6 +3445,9 @@ public:
         testLoanBrokerDeleteConfidentialCOA(all_);
         testLoanBrokerDeleteConfidentialCOA(all_ - fixCleanup3_5_0);
 
+        testDebtMaximumOutOfRange(all_);
+        testDebtMaximumOutOfRange(all_ - fixCleanup3_5_0);
+
         // featureMPTokensV2 independently makes ValidMPTTransfer enforcing,
         // but it's Supported::No (never enabled on real networks); exclude
         // it here so fixCleanup3_4_0 alone is the deciding amendment, as it

src/test/app/lending/LoanSet_test.cpp

@@ -22,6 +22,7 @@
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/SeqProxy.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
@@ -932,6 +933,38 @@ private:
         run(all_, tesSUCCESS);
     }
 
+    void
+    testPrincipalRequestedOutOfRange(FeatureBitset features)
+    {
+        using namespace jtx;
+        using namespace loan;
+
+        bool const withFix = features[fixCleanup3_5_0];
+        testcase(
+            std::string("LoanSet PrincipalRequested out of range for the asset (") +
+            (withFix ? "post" : "pre") + "-fixCleanup3_5_0)");
+
+        Env env(*this, features);
+        Account const lender{"lender"};
+        Account const borrower{"borrower"};
+        env.fund(XRP(10'000'000), lender, borrower);
+        env.close();
+
+        PrettyAsset const xrp{xrpIssue(), 1'000'000};
+        auto const broker = createVaultAndBroker(env, xrp, lender);
+
+        // Without the fix, the representability check in preclaim throws for
+        // XRP above STAmount::kMaxNativeN.
+        auto const ownerCount = env.ownerCount(borrower);
+        Number const maxXrp{static_cast<std::int64_t>(STAmount::kMaxNativeN)};
+        env(set(borrower, broker.brokerID, maxXrp + 1),
+            Sig(sfCounterpartySignature, lender),
+            Fee(env.current()->fees().base * 2),
+            Ter(withFix ? TER{tecPRECISION_LOSS} : TER{tefEXCEPTION}));
+        env.close();
+        BEAST_EXPECT(env.ownerCount(borrower) == ownerCount);
+    }
+
 public:
     void
     run() override
@@ -944,6 +977,9 @@ public:
         testLoanSetExistingLineAfterIssuerClearsDefaultRipple();
         testLoanSetOriginationFeeTwoMptCreates(all_);
         testLoanSetOriginationFeeTwoMptCreates(all_ - fixCleanup3_4_0);
+
+        testPrincipalRequestedOutOfRange(all_);
+        testPrincipalRequestedOutOfRange(all_ - fixCleanup3_5_0);
     }
 };
 

src/test/app/vault/VaultScale_test.cpp

@@ -1052,13 +1052,15 @@ private:
     }
 
     void
-    testAssetsMaximum()
+    testAssetsMaximum(FeatureBitset features)
     {
-        testcase("Assets Maximum");
+        bool const withFix = features[fixCleanup3_5_0];
+        testcase(
+            std::string("Assets Maximum (") + (withFix ? "post" : "pre") + "-fixCleanup3_5_0)");
 
         using namespace test::jtx;
 
-        Env env{*this, testableAmendments()};
+        Env env{*this, features};
         Account const owner{"owner"};
         Account const issuer{"issuer"};
 
@@ -1084,6 +1086,10 @@ private:
         auto const initialXRPPlus1 = to_string(kInitialXrp + 1);
         BEAST_EXPECT(initialXRPPlus1 == "100000000000000001");
 
+        // Without the fix, rounding an out-of-range AssetsMaximum to the vault
+        // asset throws.
+        TER const outOfRangeErr = withFix ? TER{temMALFORMED} : TER{tefEXCEPTION};
+
         {
             testcase("Assets Maximum: XRP");
 
@@ -1093,11 +1099,11 @@ private:
             tx[sfData] = "4D65746144617461";
 
             tx[sfAssetsMaximum] = maxInt64;
-            env(tx, Ter(tefEXCEPTION));
+            env(tx, Ter(outOfRangeErr));
             env.close();
 
             tx[sfAssetsMaximum] = initialXRPPlus1;
-            env(tx, Ter(tefEXCEPTION));
+            env(tx, Ter(outOfRangeErr));
             env.close();
 
             tx[sfAssetsMaximum] = initialXRP;
@@ -1346,7 +1352,7 @@ private:
             // What _can't_ IOUs do?
             // 1. Exceed maximum exponent / offset
             tx[sfAssetsMaximum] = "1000000000000000e81";
-            env(tx, Ter(tefEXCEPTION));
+            env(tx, Ter(outOfRangeErr));
             env.close();
 
             // 2. Mantissa larger than uint64 max
@@ -1371,7 +1377,8 @@ public:
     run() override
     {
         testScaleIOU();
-        testAssetsMaximum();
+        testAssetsMaximum(all_ - fixCleanup3_5_0);
+        testAssetsMaximum(all_);
     }
 };
 

src/test/app/vault/VaultValidation_test.cpp

@@ -30,11 +30,13 @@
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STAmount.h>
+#include <xrpl/protocol/STNumber.h>  // IWYU pragma: keep
 #include <xrpl/protocol/SeqProxy.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
 #include <xrpl/protocol/jss.h>
 
+#include <cstdint>
 #include <functional>
 #include <string>
 #include <tuple>
@@ -1179,6 +1181,62 @@ private:
         }
     }
 
+    void
+    testAssetsMaximumOutOfRange(FeatureBitset features)
+    {
+        using namespace test::jtx;
+
+        bool const withFix = features[fixCleanup3_5_0];
+        testcase(
+            std::string("AssetsMaximum out of range for the asset (") + (withFix ? "post" : "pre") +
+            "-fixCleanup3_5_0)");
+
+        Env env{*this, features};
+        Account const issuer{"issuer"};
+        Account const owner{"owner"};
+        env.fund(XRP(1'000'000), issuer, owner);
+        env.close();
+
+        Vault const vault{env};
+
+        // Without the fix, rounding an out-of-range AssetsMaximum to the vault
+        // asset throws.
+        TER const createErr = withFix ? TER{temMALFORMED} : TER{tefEXCEPTION};
+        TER const setErr = withFix ? TER{tecPRECISION_LOSS} : TER{tefEXCEPTION};
+
+        auto const test = [&](Asset const& asset, Number const& maxValid, Number const& overMax) {
+            {
+                auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+                tx[sfAssetsMaximum] = to_string(overMax);
+                env(tx, Ter(createErr));
+                env.close();
+                BEAST_EXPECT(!env.le(keylet));
+            }
+
+            auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+            tx[sfAssetsMaximum] = to_string(maxValid);
+            env(tx);
+            env.close();
+
+            auto setTx = vault.set({.owner = owner, .id = keylet.key});
+            setTx[sfAssetsMaximum] = to_string(overMax);
+            env(setTx, Ter(setErr));
+            env.close();
+
+            if (auto const sle = env.le(keylet); BEAST_EXPECT(sle))
+                BEAST_EXPECT(sle->at(sfAssetsMaximum) == maxValid);
+        };
+
+        Number const maxXrp{static_cast<std::int64_t>(STAmount::kMaxNativeN)};
+        test(xrpIssue(), maxXrp, maxXrp + 1);
+
+        MPT const btc = MPTTester({.env = env, .issuer = issuer, .holders = {owner}});
+        test(btc.asset(), Number{static_cast<std::int64_t>(kMaxMpTokenAmount)}, Number{1, 19});
+
+        Number const maxIou{static_cast<std::int64_t>(STAmount::kMaxValue), STAmount::kMaxOffset};
+        test(issuer["USD"], maxIou, Number{1, STAmount::kMaxOffset + 16});
+    }
+
 public:
     void
     run() override
@@ -1192,6 +1250,9 @@ public:
 
         testVaultWithdrawPseudoAccountDestination(all_ - fixCleanup3_4_0);
         testVaultWithdrawPseudoAccountDestination(all_);
+
+        testAssetsMaximumOutOfRange(all_ - fixCleanup3_5_0);
+        testAssetsMaximumOutOfRange(all_);
     }
 };
 

src/tests/libxrpl/shamap/SHAMapAddNode.cpp

@@ -0,0 +1,76 @@
+#include <xrpl/shamap/SHAMapAddNode.h>
+
+#include <gtest/gtest.h>
+
+namespace xrpl::tests {
+
+// get() is a log format, so its wording is pinned here, once. Every other site reads the same tally
+// by value, through the count and verdict accessors.
+TEST(SHAMapAddNode, get_names_every_non_empty_count)
+{
+    EXPECT_EQ(SHAMapAddNode{}.get(), "no nodes processed");
+    EXPECT_EQ(SHAMapAddNode::useful().get(), "good:1");
+    EXPECT_EQ(SHAMapAddNode::invalid().get(), "bad:1");
+    EXPECT_EQ(SHAMapAddNode::duplicate().get(), "dupe:1");
+
+    // Several of a kind are counted, and the counts are joined in a fixed order with a single
+    // space, whichever order they were recorded in.
+    SHAMapAddNode san;
+    san.incInvalid();
+    san.incUseful();
+    san.incUseful();
+    san.incDuplicate();
+    EXPECT_EQ(san.get(), "good:2 bad:1 dupe:1");
+
+    san.reset();
+    EXPECT_EQ(san.get(), "no nodes processed");
+}
+
+// The three counts and the verdicts derived from them.
+TEST(SHAMapAddNode, counts_and_verdicts_agree)
+{
+    SHAMapAddNode san;
+    EXPECT_EQ(san.getGood(), 0);
+    EXPECT_EQ(san.getBad(), 0);
+    EXPECT_EQ(san.getDuplicate(), 0);
+    EXPECT_FALSE(san.isInvalid());
+    EXPECT_FALSE(san.isUseful());
+
+    // Good counts what produced a good result, and useful is that count being non-zero.
+    san.incUseful();
+    EXPECT_EQ(san.getGood(), 1);
+    EXPECT_TRUE(san.isUseful());
+    EXPECT_TRUE(san.isGood());
+
+    // A duplicate counts toward good. isUseful() here reflects the incUseful() above.
+    san.incDuplicate();
+    EXPECT_EQ(san.getDuplicate(), 1);
+    EXPECT_FALSE(san.isInvalid());
+    EXPECT_TRUE(san.isGood());
+
+    // Bad is a count, so a batch that carries on past a rejected node reports one per node, which
+    // distinguishes "stopped on the first" from "rejected several".
+    san.incInvalid();
+    EXPECT_EQ(san.getBad(), 1);
+    EXPECT_TRUE(san.isInvalid());
+    EXPECT_TRUE(san.isGood()) << "one bad node among two accepted ones is still a good batch";
+
+    san.incInvalid();
+    san.incInvalid();
+    EXPECT_EQ(san.getGood(), 1);
+    EXPECT_EQ(san.getBad(), 3);
+    EXPECT_EQ(san.getDuplicate(), 1);
+    EXPECT_FALSE(san.isGood()) << "more bad nodes than accepted ones is not a good batch";
+
+    // Adding one verdict to another sums every count.
+    SHAMapAddNode total;
+    total += SHAMapAddNode::useful();
+    total += SHAMapAddNode::invalid();
+    total += SHAMapAddNode::invalid();
+    total += SHAMapAddNode::duplicate();
+    EXPECT_EQ(total.getGood(), 1);
+    EXPECT_EQ(total.getBad(), 2);
+    EXPECT_EQ(total.getDuplicate(), 1);
+}
+
+}  // namespace xrpl::tests

src/tests/libxrpl/shamap/SHAMapSync.cpp

@@ -180,4 +180,50 @@ TEST_F(SHAMapSyncTest, sync)
     destination.invariants();
 }
 
+// The duplicate verdict also answers for a node that did not need to be added, which is the half
+// of incDuplicate()'s contract that "already held" does not cover. addKnownNode() reaches it when
+// the map has stopped taking nodes, and returns there before looking at the offer at all.
+//
+// An A/B on one offer and two maps of the same shape, so the synching state is the only difference
+// between the two verdicts. Neither map holds the offered node, so a count that meant "already
+// held" would be wrong for it.
+TEST_F(SHAMapSyncTest, add_known_node_reports_duplicate_once_a_map_stops_synching)
+{
+    TestNodeFamily f{j_};
+
+    // A well-formed inner node with one child. Its contents do not matter: both verdicts below
+    // are reached without the map reading them.
+    auto const makeOffer = [] {
+        Serializer s;
+        s.addBitString(UInt256{1});
+        s.add8(0);
+        s.add8(kWireTypeCompressedInner);
+        return SHAMapTreeNode::makeFromWire(makeSlice(s.peekData()));
+    };
+    ASSERT_TRUE(makeOffer());
+
+    SHAMapNodeID const target{1, UInt256{}};
+
+    // Synching, so the offer is examined, and refused because the empty root has no branch to
+    // hook it onto. The point is that the map looked.
+    SHAMap synching{SHAMapType::FREE, f};
+    synching.setSynching();
+
+    auto const examined = synching.addKnownNode(target, makeOffer(), nullptr);
+    EXPECT_TRUE(examined.isInvalid());
+    EXPECT_EQ(examined.getDuplicate(), 0);
+
+    // The same offer to a map that has stopped taking nodes.
+    SHAMap stopped{SHAMapType::FREE, f};
+    stopped.setSynching();
+    stopped.clearSynching();
+    ASSERT_FALSE(stopped.isSynching());
+
+    auto const notNeeded = stopped.addKnownNode(target, makeOffer(), nullptr);
+    EXPECT_EQ(notNeeded.getGood(), 0);
+    EXPECT_EQ(notNeeded.getBad(), 0);
+    EXPECT_EQ(notNeeded.getDuplicate(), 1);
+    EXPECT_TRUE(notNeeded.isGood()) << "a node that was not needed counts on the accepted side";
+}
+
 }  // namespace xrpl::tests

src/xrpld/app/ledger/ConsensusTransSetSF.cpp

@@ -42,7 +42,7 @@ ConsensusTransSetSF::gotNode(
 
     nodeCache_.insert(nodeHash, nodeData);
 
-    if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() > 16))
+    if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() >= kMinTxNodeBytesToParse))
     {
         // this is a transaction, and we didn't have it
         JLOG(j_.debug()) << "Node on our acquiring TX set is TXN we may not have";

src/xrpld/app/ledger/ConsensusTransSetSF.h

@@ -9,6 +9,7 @@
 #include <xrpl/shamap/SHAMapSyncFilter.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
+#include <cstddef>
 #include <cstdint>
 #include <optional>
 
@@ -24,6 +25,15 @@ class ConsensusTransSetSF : public SHAMapSyncFilter
 public:
     using NodeCache = TaggedCache<SHAMapHash, Blob>;
 
+    /**
+     * The size a node's hash-prefixed wire data must reach before gotNode()
+     * tries to parse and resubmit it as a transaction. One byte past the
+     * smallest a hash-prefixed SHAMap leaf can be, which a signed transaction
+     * clears.
+     */
+    static constexpr std::size_t kMinTxNodeBytesToParse =
+        sizeof(std::uint32_t) + kMinShaMapItemBytes + 1;
+
     ConsensusTransSetSF(Application& app, NodeCache& nodeCache);
 
     // Note that the nodeData is overwritten by this call

src/xrpld/app/ledger/InboundLedger.h

@@ -30,9 +30,9 @@
 namespace xrpl {
 
 // A ledger we are trying to acquire
-class InboundLedger final : public TimeoutCounter,
-                            public std::enable_shared_from_this<InboundLedger>,
-                            public CountedObject<InboundLedger>
+class InboundLedger : public TimeoutCounter,
+                      public std::enable_shared_from_this<InboundLedger>,
+                      public CountedObject<InboundLedger>
 {
 public:
     using ClockType = beast::AbstractClock<std::chrono::steady_clock>;
@@ -44,13 +44,29 @@ public:
         CONSENSUS,  // We believe the consensus round requires this ledger
     };
 
+    /**
+     * How long to wait between retries, and so how long one timeout takes.
+     */
+    static constexpr std::chrono::milliseconds kRetryInterval{3000};
+
+    /**
+     * @param app The application to run in.
+     * @param hash The ledger to acquire.
+     * @param seq Its sequence, or zero if not known yet.
+     * @param reason Why it is being acquired.
+     * @param clock The clock touch() records against.
+     * @param peerSet Which peers to ask, and how to reach them.
+     * @param retryInterval How long to wait between retries. TimeoutCounter
+     *        requires more than 10ms and less than 30s.
+     */
     InboundLedger(
         Application& app,
         UInt256 const& hash,
         std::uint32_t seq,
         Reason reason,
-        ClockType&,
-        std::unique_ptr<PeerSet> peerSet);
+        ClockType& clock,
+        std::unique_ptr<PeerSet> peerSet,
+        std::chrono::milliseconds retryInterval = kRetryInterval);
 
     ~InboundLedger() override;
 
@@ -119,15 +135,33 @@ public:
         return lastAction_;
     }
 
-private:
+protected:
+    // Protected so a test subclass can drive an acquisition through trigger() and done().
+
+    // Why trigger() is being run, which decides how deep a request goes and whether an
+    // aggressive retry applies.
     enum class TriggerReason { Added, Reply, Timeout };
 
+    /**
+     * Ask for more nodes, or judge what has been collected.
+     *
+     * @param peer The peer to ask, or nullptr to ask everyone being tracked.
+     * @param reason Why the acquisition is being triggered.
+     */
+    void
+    trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason);
+
+    /**
+     * Settle the acquisition and signal whatever is waiting on it. Runs at most
+     * once. Callers hold mtx_, except trigger(), which releases it first.
+     */
+    void
+    done();
+
+private:
     void
     filterNodes(std::vector<std::pair<SHAMapNodeID, UInt256>>& nodes, TriggerReason reason);
 
-    void
-    trigger(std::shared_ptr<Peer> const&, TriggerReason);
-
     std::vector<NeededHashT>
     getNeededHashes();
 
@@ -138,10 +172,7 @@ private:
     tryDB(node_store::Database& srcDB);
 
     void
-    done();
-
-    void
-    onTimer(bool progress, ScopedLockType& peerSetLock) override;
+    onTimer(bool progress, ScopedLockType& sl) override;
 
     std::size_t
     getPeerCount() const;

src/xrpld/app/ledger/detail/InboundLedger.cpp

@@ -55,8 +55,6 @@
 
 namespace xrpl {
 
-using namespace std::chrono_literals;
-
 static constexpr auto kPeerCountStart = 5;           // Number of peers to start with
 static constexpr auto kPeerCountAdd = 3;             // Number of peers to add on a timeout
 static constexpr auto kLedgerTimeoutRetriesMax = 6;  // how many timeouts before we give up
@@ -66,20 +64,18 @@ static constexpr auto kMissingNodesFind = 256;  // Number of nodes to find initi
 static constexpr auto kReqNodesReply = 128;     // Number of nodes to request for a reply
 static constexpr auto kReqNodes = 12;           // Number of nodes to request blindly
 
-// millisecond for each ledger timeout
-constexpr auto kLedgerAcquireTimeout = 3000ms;
-
 InboundLedger::InboundLedger(
     Application& app,
     UInt256 const& hash,
     std::uint32_t seq,
     Reason reason,
     ClockType& clock,
-    std::unique_ptr<PeerSet> peerSet)
+    std::unique_ptr<PeerSet> peerSet,
+    std::chrono::milliseconds retryInterval)
     : TimeoutCounter(
           app,
           hash,
-          kLedgerAcquireTimeout,
+          retryInterval,
           {.jobType = JtLedgerData, .jobName = "InboundLedger", .jobLimit = 5},
           app.getJournal("InboundLedger"))
     , clock_(clock)

src/xrpld/app/ledger/detail/TimeoutCounter.h

@@ -98,9 +98,14 @@ protected:
 
     /**
      * Hook called from invokeOnTimer().
+     *
+     * @param progress Whether the subtype recorded progress since the
+     *        last call.
+     * @param sl Proof mtx_ is held. mtx_ is recursive, so a nested lock taken
+     *        inside the call does not release it.
      */
     virtual void
-    onTimer(bool progress, ScopedLockType&) = 0;
+    onTimer(bool progress, ScopedLockType& sl) = 0;
 
     /**
      * Return a weak pointer to this.

src/xrpld/app/ledger/detail/TransactionAcquire.cpp

@@ -18,6 +18,7 @@
 #include <xrpl.pb.h>
 
 #include <algorithm>
+#include <chrono>
 #include <cstddef>
 #include <exception>
 #include <memory>
@@ -26,22 +27,18 @@
 
 namespace xrpl {
 
-using namespace std::chrono_literals;
-
-// Timeout interval in milliseconds
-constexpr auto kTxAcquireTimeout = 250ms;
-
 static constexpr auto kNormTimeouts = 4;
 static constexpr auto kMaxTimeouts = 20;
 
 TransactionAcquire::TransactionAcquire(
     Application& app,
     UInt256 const& hash,
-    std::unique_ptr<PeerSet> peerSet)
+    std::unique_ptr<PeerSet> peerSet,
+    std::chrono::milliseconds retryInterval)
     : TimeoutCounter(
           app,
           hash,
-          kTxAcquireTimeout,
+          retryInterval,
           {.jobType = JtTxnData, .jobName = "TxAcq", .jobLimit = {}},
           app.getJournal("TransactionAcquire"))
     , peerSet_(std::move(peerSet))
@@ -79,7 +76,7 @@ TransactionAcquire::done()
 }
 
 void
-TransactionAcquire::onTimer(bool progress, ScopedLockType& psl)
+TransactionAcquire::onTimer(bool progress, ScopedLockType&)
 {
     if (timeouts_ > kMaxTimeouts)
     {

src/xrpld/app/ledger/detail/TransactionAcquire.h

@@ -12,6 +12,7 @@
 #include <xrpl/shamap/SHAMapNodeID.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
 
+#include <chrono>
 #include <cstddef>
 #include <memory>
 #include <utility>
@@ -21,14 +22,30 @@ namespace xrpl {
 
 // VFALCO TODO rename to PeerTxRequest
 // A transaction set we are trying to acquire
-class TransactionAcquire final : public TimeoutCounter,
-                                 public std::enable_shared_from_this<TransactionAcquire>,
-                                 public CountedObject<TransactionAcquire>
+class TransactionAcquire : public TimeoutCounter,
+                           public std::enable_shared_from_this<TransactionAcquire>,
+                           public CountedObject<TransactionAcquire>
 {
 public:
     using pointer = std::shared_ptr<TransactionAcquire>;
 
-    TransactionAcquire(Application& app, UInt256 const& hash, std::unique_ptr<PeerSet> peerSet);
+    /**
+     * How long to wait between retries, and so how long one timeout takes.
+     */
+    static constexpr std::chrono::milliseconds kRetryInterval{250};
+
+    /**
+     * @param app The application to run in.
+     * @param hash The set to acquire.
+     * @param peerSet Which peers to ask, and how to reach them.
+     * @param retryInterval How long to wait between retries. TimeoutCounter
+     *        requires more than 10ms and less than 30s.
+     */
+    TransactionAcquire(
+        Application& app,
+        UInt256 const& hash,
+        std::unique_ptr<PeerSet> peerSet,
+        std::chrono::milliseconds retryInterval = kRetryInterval);
     ~TransactionAcquire() override = default;
 
     SHAMapAddNode
@@ -42,14 +59,23 @@ public:
     void
     stillNeed();
 
-private:
+protected:
+    // Kept protected so a test subclass can read the map's state.
+    // Production callers reach a set through InboundTransactions.
     std::shared_ptr<SHAMap> map_;
+
+private:
     bool haveRoot_{false};
     std::unique_ptr<PeerSet> peerSet_;
 
     void
-    onTimer(bool progress, ScopedLockType& peerSetLock) override;
+    onTimer(bool progress, ScopedLockType& sl) override;
 
+    /**
+     * Settle the acquired set and hand it on, or report the failure. Call under
+     * mtx_. Runs once per outcome, and a stillNeed() revival gives one object a
+     * second outcome.
+     */
     void
     done();