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

Co-authored-by: Vito Tumas <[email protected]>
Co-authored-by: Ayaz Salikhov <[email protected]>
Co-authored-by: Ed Hennis <[email protected]>

.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/OfferEntry.h

@@ -9,6 +9,7 @@
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/SeqProxy.h>
+#include <xrpl/protocol/TER.h>
 
 namespace xrpl {
 
@@ -38,6 +39,18 @@ public:
         : Base(keylet::offer(offerID), view, j)
     {
     }
+
+    /**
+     * Delete this offer from the ledger.
+     *
+     * Requirements:
+     *     The caller must have already checked permissions.
+     *
+     * @return tesSUCCESS on success, otherwise an error code.
+     */
+    TER
+    removeFromLedger()
+        requires Base::kIsWritable;
 };
 
 using OfferEntryR = OfferEntry<ReadView>;

include/xrpl/ledger/helpers/OfferHelpers.h

@@ -1,27 +0,0 @@
-#pragma once
-
-#include <xrpl/beast/utility/Journal.h>
-#include <xrpl/ledger/ApplyView.h>
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/TER.h>
-
-namespace xrpl {
-
-/**
- * Delete an offer.
- *
- * Requirements:
- *     The offer must exist.
- *     The caller must have already checked permissions.
- *
- * @param view The ApplyView to modify.
- * @param sle The offer to delete.
- * @param j Journal for logging.
- *
- * @return tesSUCCESS on success, otherwise an error code.
- */
-// [[nodiscard]] // nodiscard commented out so Flow, BookTip and others compile.
-TER
-offerDelete(ApplyView& view, SLE::Ref sle, beast::Journal j);
-
-}  // 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/paths/detail/StrandFlow.h

@@ -6,12 +6,11 @@
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/beast/utility/instrumentation.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/ledger/helpers/AMMHelpers.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/protocol/Concepts.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/IOUAmount.h>
-#include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/MPTAmount.h>
 #include <xrpl/protocol/Quality.h>
 #include <xrpl/protocol/QualityFunction.h>
@@ -794,8 +793,8 @@ flow(
             setUnion(ofrsToRmOnFail, ofrsToRm);
             for (auto const& o : ofrsToRm)
             {
-                if (auto const ok = sb.peek(keylet::offer(o)))
-                    offerDelete(sb, ok, j);
+                if (OfferEntryW ok(o, sb, j); ok)
+                    ok.removeFromLedger();
             }
         }
 

include/xrpl/tx/transactors/dex/OfferCreate.h

@@ -5,6 +5,7 @@
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Keylet.h>
@@ -108,7 +109,7 @@ private:
     TER
     applyHybrid(
         Sandbox& sb,
-        STLedgerEntry::pointer sleOffer,
+        OfferEntryW& sleOffer,
         Keylet const& offerIndex,
         STAmount const& saTakerPays,
         STAmount const& saTakerGets,

src/libxrpl/ledger/entries/OfferEntry.cpp

@@ -0,0 +1,74 @@
+#include <xrpl/ledger/entries/OfferEntry.h>
+
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/utility/instrumentation.h>
+#include <xrpl/ledger/ApplyView.h>
+#include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/helpers/AccountRootHelpers.h>
+#include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/LedgerFormats.h>
+#include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STArray.h>  // IWYU pragma: keep
+#include <xrpl/protocol/TER.h>
+
+namespace xrpl {
+
+template <typename ViewT>
+TER
+OfferEntry<ViewT>::removeFromLedger()
+    requires Base::kIsWritable
+{
+    if (!this->exists())
+        return tesSUCCESS;
+
+    auto const offerIndex = this->key();
+    auto const owner = (*this)->getAccountID(sfAccount);
+
+    // Detect legacy directories.
+    UInt256 const uDirectory = (*this)->getFieldH256(sfBookDirectory);
+
+    ApplyView& view = this->applyView();
+
+    if (!view.dirRemove(
+            keylet::ownerDir(owner), (*this)->getFieldU64(sfOwnerNode), offerIndex, false))
+    {
+        return tefBAD_LEDGER;  // LCOV_EXCL_LINE
+    }
+
+    if (!view.dirRemove(
+            keylet::page(uDirectory), (*this)->getFieldU64(sfBookNode), offerIndex, false))
+    {
+        return tefBAD_LEDGER;  // LCOV_EXCL_LINE
+    }
+
+    if ((*this)->isFieldPresent(sfAdditionalBooks))
+    {
+        XRPL_ASSERT(
+            (*this)->isFlag(lsfHybrid) && (*this)->isFieldPresent(sfDomainID),
+            "xrpl::OfferEntry::removeFromLedger : should be a hybrid domain offer");
+
+        auto const& additionalBookDirs = (*this)->getFieldArray(sfAdditionalBooks);
+
+        for (auto const& bookDir : additionalBookDirs)
+        {
+            auto const& dirIndex = bookDir.getFieldH256(sfBookDirectory);
+            auto const& dirNode = bookDir.getFieldU64(sfBookNode);
+
+            if (!view.dirRemove(keylet::page(dirIndex), dirNode, offerIndex, false))
+            {
+                return tefBAD_LEDGER;  // LCOV_EXCL_LINE
+            }
+        }
+    }
+
+    decreaseOwnerCountForObject(view, owner, this->mutableRawSle(), 1, this->journal());
+
+    this->erase();
+
+    return tesSUCCESS;
+}
+
+template class OfferEntry<ReadView>;
+template class OfferEntry<ApplyView>;
+
+}  // namespace xrpl

src/libxrpl/ledger/helpers/OfferHelpers.cpp

@@ -1,65 +0,0 @@
-#include <xrpl/ledger/helpers/OfferHelpers.h>
-
-#include <xrpl/basics/base_uint.h>
-#include <xrpl/beast/utility/Journal.h>
-#include <xrpl/beast/utility/instrumentation.h>
-#include <xrpl/ledger/ApplyView.h>
-#include <xrpl/ledger/helpers/AccountRootHelpers.h>
-#include <xrpl/protocol/Indexes.h>
-#include <xrpl/protocol/LedgerFormats.h>  // IWYU pragma: keep
-#include <xrpl/protocol/SField.h>
-#include <xrpl/protocol/STArray.h>  // IWYU pragma: keep
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/TER.h>
-
-namespace xrpl {
-
-TER
-offerDelete(ApplyView& view, SLE::Ref sle, beast::Journal j)
-{
-    if (!sle)
-        return tesSUCCESS;
-    auto const offerIndex = sle->key();
-    auto const owner = sle->getAccountID(sfAccount);
-
-    // Detect legacy directories.
-    UInt256 const uDirectory = sle->getFieldH256(sfBookDirectory);
-
-    if (!view.dirRemove(keylet::ownerDir(owner), sle->getFieldU64(sfOwnerNode), offerIndex, false))
-    {
-        return tefBAD_LEDGER;  // LCOV_EXCL_LINE
-    }
-
-    if (!view.dirRemove(keylet::page(uDirectory), sle->getFieldU64(sfBookNode), offerIndex, false))
-    {
-        return tefBAD_LEDGER;  // LCOV_EXCL_LINE
-    }
-
-    if (sle->isFieldPresent(sfAdditionalBooks))
-    {
-        XRPL_ASSERT(
-            sle->isFlag(lsfHybrid) && sle->isFieldPresent(sfDomainID),
-            "xrpl::offerDelete : should be a hybrid domain offer");
-
-        auto const& additionalBookDirs = sle->getFieldArray(sfAdditionalBooks);
-
-        for (auto const& bookDir : additionalBookDirs)
-        {
-            auto const& dirIndex = bookDir.getFieldH256(sfBookDirectory);
-            auto const& dirNode = bookDir.getFieldU64(sfBookNode);
-
-            if (!view.dirRemove(keylet::page(dirIndex), dirNode, offerIndex, false))
-            {
-                return tefBAD_LEDGER;  // LCOV_EXCL_LINE
-            }
-        }
-    }
-
-    decreaseOwnerCountForObject(view, owner, sle, 1, j);
-
-    view.erase(sle);
-
-    return tesSUCCESS;
-}
-
-}  // 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,11 +11,11 @@
 #include <xrpl/json/to_string.h>  // IWYU pragma: keep
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/entries/OfferEntry.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>
 #include <xrpl/ledger/helpers/SponsorHelpers.h>
 #include <xrpl/protocol/AccountID.h>
@@ -1216,10 +1216,10 @@ removeUnfundedOffers(ApplyView& view, std::vector<UInt256> const& offers, beast:
 
     for (auto const& index : offers)
     {
-        if (auto const sleOffer = view.peek(keylet::offer(index)))
+        if (OfferEntryW sleOffer(index, view, viewJ); sleOffer)
         {
             // offer is unfunded
-            offerDelete(view, sleOffer, viewJ);
+            sleOffer.removeFromLedger();
             if (++removed == kUnfundedOfferRemoveLimit)
                 return;
         }

src/libxrpl/tx/paths/BookTip.cpp

@@ -2,10 +2,12 @@
 
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/ledger/ApplyView.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/protocol/Book.h>
 #include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Keylet.h>
+#include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/STLedgerEntry.h>
 
 namespace xrpl {
@@ -22,7 +24,7 @@ BookTip::step(beast::Journal j)
     {
         if (entry_)
         {
-            offerDelete(view_, entry_, j);
+            OfferEntryW(Keylet(ltOFFER, entry_->key()), view_, j).removeFromLedger();
             entry_ = nullptr;
         }
     }

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

@@ -10,10 +10,10 @@
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/View.h>
 #include <xrpl/ledger/entries/DepositPreauthEntry.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/ledger/helpers/NFTokenHelpers.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
@@ -79,12 +79,12 @@ TER
 offerDelete(
     ServiceRegistry&,
     ApplyView& view,
-    AccountID const& account,
+    AccountID const&,
     UInt256 const& delIndex,
-    SLE::Ref sleDel,
+    SLE::Ref,
     beast::Journal j)
 {
-    return offerDelete(view, sleDel, j);
+    return OfferEntryW(Keylet(ltOFFER, delIndex), view, j).removeFromLedger();
 }
 
 TER

src/libxrpl/tx/transactors/dex/OfferCancel.cpp

@@ -1,7 +1,7 @@
 #include <xrpl/tx/transactors/dex/OfferCancel.h>
 
 #include <xrpl/basics/Log.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STLedgerEntry.h>
@@ -59,10 +59,11 @@ OfferCancel::doApply()
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
     auto const seqProxy = SeqProxy::rawSequence(offerSequence);
-    if (auto const sleOffer = view().peek(keylet::offer(accountID_, seqProxy)))
+    if (OfferEntryW sleOffer(accountID_, seqProxy, view(), ctx_.registry.get().getJournal("View"));
+        sleOffer)
     {
         JLOG(j_.debug()) << "Trying to cancel offer #" << offerSequence;
-        return offerDelete(view(), sleOffer, ctx_.registry.get().getJournal("View"));
+        return sleOffer.removeFromLedger();
     }
 
     JLOG(j_.debug()) << "Offer #" << offerSequence << " can't be found.";

src/libxrpl/tx/transactors/dex/OfferCreate.cpp

@@ -9,11 +9,11 @@
 #include <xrpl/ledger/PaymentSandbox.h>
 #include <xrpl/ledger/Sandbox.h>
 #include <xrpl/ledger/View.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/ledger/helpers/MPTokenHelpers.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/ledger/helpers/PermissionedDEXHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
 #include <xrpl/protocol/AccountID.h>
@@ -495,10 +495,10 @@ OfferCreate::flowCross(
         // If stale offers were found remove them.
         for (auto const& toRemove : result.removableOffers)
         {
-            if (auto const otr = psb.peek(keylet::offer(toRemove)))
-                offerDelete(psb, otr, j_);
-            if (auto const otr = psbCancel.peek(keylet::offer(toRemove)))
-                offerDelete(psbCancel, otr, j_);
+            if (OfferEntryW otr(toRemove, psb, j_); otr)
+                otr.removeFromLedger();
+            if (OfferEntryW otr(toRemove, psbCancel, j_); otr)
+                otr.removeFromLedger();
         }
 
         // Determine the size of the final offer after crossing.
@@ -595,7 +595,7 @@ OfferCreate::formatAmount(STAmount const& amount)
 TER
 OfferCreate::applyHybrid(
     Sandbox& sb,
-    STLedgerEntry::pointer sleOffer,
+    OfferEntryW& sleOffer,
     Keylet const& offerKey,
     STAmount const& saTakerPays,
     STAmount const& saTakerGets,
@@ -673,7 +673,7 @@ OfferCreate::applyGuts(Sandbox& sb, Sandbox& sbCancel)
     if (cancelSequence)
     {
         auto const seqProxy = SeqProxy::rawSequence(*cancelSequence);
-        auto const sleCancel = sb.peek(keylet::offer(accountID_, seqProxy));
+        OfferEntryW sleCancel(accountID_, seqProxy, sb, viewJ);
 
         // It's not an error to not find the offer to cancel: it might have
         // been consumed or removed. If it is found, however, it's an error
@@ -681,7 +681,7 @@ OfferCreate::applyGuts(Sandbox& sb, Sandbox& sbCancel)
         if (sleCancel)
         {
             JLOG(j_.debug()) << "Create cancels order " << *cancelSequence;
-            result = offerDelete(sb, sleCancel, viewJ);
+            result = sleCancel.removeFromLedger();
         }
     }
 
@@ -976,7 +976,8 @@ OfferCreate::applyGuts(Sandbox& sb, Sandbox& sbCancel)
         // LCOV_EXCL_STOP
     }
 
-    auto const sleOffer = std::make_shared<SLE>(offerIndex);
+    OfferEntryW sleOffer(offerIndex, sb, j_);
+    sleOffer.newSLE();
     sleOffer->setAccountID(sfAccount, accountID_);
     sleOffer->setFieldU32(sfSequence, offerSequence.value());
     sleOffer->setFieldH256(sfBookDirectory, dir.key);
@@ -1010,7 +1011,7 @@ OfferCreate::applyGuts(Sandbox& sb, Sandbox& sbCancel)
             return {res, true};  // LCOV_EXCL_LINE
     }
 
-    sb.insert(sleOffer);
+    sleOffer.insert();
 
     if (!bookExisted)
         ctx_.addOrderBook(book);

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/AMMExtendedMPT_test.cpp

@@ -31,7 +31,7 @@
 #include <xrpl/ledger/OpenView.h>
 #include <xrpl/ledger/PaymentSandbox.h>
 #include <xrpl/ledger/Sandbox.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
@@ -1982,9 +1982,9 @@ private:
                 Sandbox sb(&view, TapNone);
                 for (auto const& o : flowResult.removableOffers)
                 {
-                    if (auto const ok = sb.peek(keylet::offer(o)))
+                    if (OfferEntryW ok(o, sb, flowJournal); ok)
                     {
-                        offerDelete(sb, ok, flowJournal);
+                        ok.removeFromLedger();
                     }
                 }
                 sb.apply(view);

src/test/app/AMMExtended_test.cpp

@@ -35,7 +35,7 @@
 #include <xrpl/ledger/OpenView.h>
 #include <xrpl/ledger/PaymentSandbox.h>
 #include <xrpl/ledger/Sandbox.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
@@ -2090,8 +2090,8 @@ private:
                 Sandbox sb(&view, TapNone);
                 for (auto const& o : flowResult.removableOffers)
                 {
-                    if (auto const ok = sb.peek(keylet::offer(o)))
-                        offerDelete(sb, ok, flowJournal);
+                    if (OfferEntryW ok(o, sb, flowJournal); ok)
+                        ok.removeFromLedger();
                 }
                 sb.apply(view);
                 return true;

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/FlowMPT_test.cpp

@@ -21,8 +21,8 @@
 #include <xrpl/ledger/OpenView.h>
 #include <xrpl/ledger/PaymentSandbox.h>
 #include <xrpl/ledger/Sandbox.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Feature.h>
@@ -377,8 +377,8 @@ struct FlowMPT_test : public beast::unit_test::Suite
                     Sandbox sb(&view, TapNone);
                     for (auto const& o : flowResult.removableOffers)
                     {
-                        if (auto const ok = sb.peek(keylet::offer(o)))
-                            offerDelete(sb, ok, flowJournal);
+                        if (OfferEntryW ok(o, sb, flowJournal); ok)
+                            ok.removeFromLedger();
                     }
                     sb.apply(view);
                     return true;

src/test/app/Flow_test.cpp

@@ -24,8 +24,8 @@
 #include <xrpl/ledger/OpenView.h>
 #include <xrpl/ledger/PaymentSandbox.h>
 #include <xrpl/ledger/Sandbox.h>
+#include <xrpl/ledger/entries/OfferEntry.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
-#include <xrpl/ledger/helpers/OfferHelpers.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
@@ -517,8 +517,8 @@ struct Flow_test : public beast::unit_test::Suite
                 Sandbox sb(&view, TapNone);
                 for (auto const& o : flowResult.removableOffers)
                 {
-                    if (auto const ok = sb.peek(keylet::offer(o)))
-                        offerDelete(sb, ok, flowJournal);
+                    if (OfferEntryW ok(o, sb, flowJournal); ok)
+                        ok.removeFromLedger();
                 }
                 sb.apply(view);
                 return true;

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();