Merge remote-tracking branch 'origin/develop' into a1q123456/default-cover-optimisation

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

include/xrpl/basics/Slice.h

@@ -11,7 +11,6 @@
 #include <limits>
 #include <stdexcept>
 #include <string>
-#include <string_view>
 #include <type_traits>
 #include <vector>
 
@@ -232,11 +231,4 @@ makeSlice(std::basic_string<char, Traits, Alloc> const& s)
     return Slice(s.data(), s.size());
 }
 
-template <class Traits>
-Slice
-makeSlice(std::basic_string_view<char, Traits> s)
-{
-    return Slice(s.data(), s.size());
-}
-
 }  // namespace xrpl

include/xrpl/ledger/helpers/LendingHelpers.h

@@ -264,6 +264,49 @@ computeFullPaymentInterest(
     std::uint32_t startDate,
     TenthBips32 closeInterestRate);
 
+/** Whether to use the proportional (new) default cover formula.
+ *
+ * Returns true when featureDefaultCoverOptimization is enabled AND the broker
+ * does not carry the deprecated sfCoverRateLiquidation field.
+ */
+inline bool
+useProportionalDefaultCover(Rules const& rules, std::shared_ptr<SLE const> const& brokerSle)
+{
+    return rules.enabled(featureDefaultCoverOptimization) &&
+        !brokerSle->isFieldPresent(sfCoverRateLiquidation);
+}
+
+/** Compute the amount of First-Loss Capital seized to cover a defaulted loan.
+ *
+ * Selects between the old (global) and new (proportional) formula based on
+ * whether featureDefaultCoverOptimization is enabled and whether the broker still
+ * carries the deprecated sfCoverRateLiquidation value.
+ *
+ * @param useProportionalFormula  true when featureDefaultCoverOptimization is
+ *                                enabled AND the broker has no
+ *                                sfCoverRateLiquidation.
+ * @param coverRateLiquidation    The broker's CoverRateLiquidation in 1/10
+ *                                bips.  Only used by the old formula; ignored
+ *                                when \p useProportionalFormula is true.
+ * @param coverAvailable          The broker's current CoverAvailable.
+ * @param vaultAsset              The Vault's asset type (for rounding).
+ * @param totalDefaultAmount      The loan's default amount (owed to the vault).
+ * @param brokerDebtTotal         The broker's total debt before this default.
+ * @param coverRateMinimum        The broker's CoverRateMinimum in 1/10 bips.
+ * @param loanScale               The loan's rounding scale.
+ * @return The amount of cover seized, capped at \p coverAvailable.
+ */
+Number
+computeDefaultCovered(
+    bool useProportionalFormula,
+    std::uint32_t coverRateLiquidation,
+    Number const& coverAvailable,
+    Asset const& vaultAsset,
+    Number const& totalDefaultAmount,
+    Number const& brokerDebtTotal,
+    TenthBips32 coverRateMinimum,
+    std::int32_t loanScale);
+
 namespace detail {
 // These classes and functions should only be accessed by LendingHelper
 // functions and unit tests

include/xrpl/proto/xrpl.proto

@@ -246,15 +246,7 @@ message TMGetObjectByHash {
 
 message TMLedgerNode {
   required bytes nodedata = 1;
-
-  // Used when protocol version <2.3. Not set for ledger base data.
-  optional bytes nodeid = 2;
-
-  // Used when protocol version >=2.3. Neither value is set for ledger base data.
-  oneof reference {
-    bytes id = 3;      // Set for inner nodes.
-    uint32 depth = 4;  // Set for leaf nodes.
-  }
+  optional bytes nodeid = 2;  // missing for ledger base data
 }
 
 enum TMLedgerInfoType {

include/xrpl/protocol/detail/features.macro

@@ -15,6 +15,7 @@
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
 
+XRPL_FEATURE(DefaultCoverOptimization,    Supported::No, VoteBehavior::DefaultNo)
 XRPL_FIX    (Cleanup3_2_0,                Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(MPTokensV2,                  Supported::No,  VoteBehavior::DefaultNo)
 XRPL_FIX    (Cleanup3_1_3,                Supported::Yes, VoteBehavior::DefaultYes)

include/xrpl/protocol/detail/ledger_entries.macro

@@ -518,6 +518,7 @@ LEDGER_ENTRY(ltLOAN_BROKER, 0x0088, LoanBroker, loan_broker, ({
     {sfDebtMaximum,          SoeDefault},
     {sfCoverAvailable,       SoeDefault},
     {sfCoverRateMinimum,     SoeDefault},
+    // Deprecated by featureDefaultCoverOptimization
     {sfCoverRateLiquidation, SoeDefault},
 }))
 

include/xrpl/protocol/detail/sfields.macro

@@ -107,6 +107,7 @@ TYPED_SFIELD(sfPaymentRemaining,         UINT32,    59)
 TYPED_SFIELD(sfPaymentTotal,             UINT32,    60)
 TYPED_SFIELD(sfLoanSequence,             UINT32,    61)
 TYPED_SFIELD(sfCoverRateMinimum,         UINT32,    62) // 1/10 basis points (bips)
+// Deprecated by featureLendingProtocolV1_1
 TYPED_SFIELD(sfCoverRateLiquidation,     UINT32,    63) // 1/10 basis points (bips)
 TYPED_SFIELD(sfOverpaymentFee,           UINT32,    64) // 1/10 basis points (bips)
 TYPED_SFIELD(sfInterestRate,             UINT32,    65) // 1/10 basis points (bips)

include/xrpl/protocol/detail/transactions.macro

@@ -960,6 +960,7 @@ TRANSACTION(ttLOAN_BROKER_SET, 74, LoanBrokerSet,
     {sfManagementFeeRate, SoeOptional},
     {sfDebtMaximum, SoeOptional},
     {sfCoverRateMinimum, SoeOptional},
+    // Deprecated by featureDefaultCoverOptimization
     {sfCoverRateLiquidation, SoeOptional},
 }))
 

include/xrpl/shamap/SHAMap.h

@@ -16,7 +16,6 @@
 
 #include <set>
 #include <stack>
-#include <tuple>
 #include <vector>
 
 namespace xrpl {
@@ -74,17 +73,6 @@ enum class SHAMapState {
 
     See https://en.wikipedia.org/wiki/Merkle_tree
  */
-
-/** Holds a SHAMap node's identity, serialized data, and leaf status.
-    Used by getNodeFat to return node data for peer synchronization.
-*/
-struct SHAMapNodeData
-{
-    SHAMapNodeID nodeID;
-    bool isLeaf;
-    Blob data;  // Placed last, so `isLeaf` can fit into the alignment padding of `nodeID`.
-};
-
 class SHAMap
 {
 private:
@@ -262,10 +250,10 @@ public:
     std::vector<std::pair<SHAMapNodeID, uint256>>
     getMissingNodes(int maxNodes, SHAMapSyncFilter* filter);
 
-    [[nodiscard]] bool
+    bool
     getNodeFat(
         SHAMapNodeID const& wanted,
-        std::vector<SHAMapNodeData>& data,
+        std::vector<std::pair<SHAMapNodeID, Blob>>& data,
         bool fatLeaves,
         std::uint32_t depth) const;
 
@@ -292,43 +280,10 @@ public:
     void
     serializeRoot(Serializer& s) const;
 
-    /** Add a root node to the SHAMap during synchronization.
-     *
-     * This function is used when receiving the root node of a SHAMap from a peer during ledger
-     * synchronization. The node must already have been deserialized.
-     *
-     * @param hash The expected hash of the root node.
-     * @param rootNode A deserialized root node to add.
-     * @param filter Optional sync filter to track received nodes.
-     * @return Status indicating whether the node was useful, duplicate, or invalid.
-     *
-     * @note This function expects the rootNode to be a valid, deserialized SHAMapTreeNode. The
-     *       caller is responsible for deserialization and basic validation before calling this
-     *       function.
-     */
     SHAMapAddNode
-    addRootNode(SHAMapHash const& hash, SHAMapTreeNodePtr rootNode, SHAMapSyncFilter const* filter);
-
-    /** Add a known node at a specific position in the SHAMap during synchronization.
-     *
-     * This function is used when receiving nodes from peers during ledger synchronization. The node
-     * is inserted at the position specified by nodeID. The node must already have been
-     * deserialized.
-     *
-     * @param nodeID The position in the tree where this node belongs.
-     * @param treeNode A deserialized tree node to add.
-     * @param filter Optional sync filter to track received nodes.
-     * @return Status indicating whether the node was useful, duplicate, or invalid.
-     *
-     * @note This function expects the treeNode to be a valid, deserialized SHAMapTreeNode. The
-     *       caller is responsible for deserialization and basic validation before calling this
-     *       function. This also means that the nodeID must be consistent with the node's content.
-     */
+    addRootNode(SHAMapHash const& hash, Slice const& rootNode, SHAMapSyncFilter* filter);
     SHAMapAddNode
-    addKnownNode(
-        SHAMapNodeID const& nodeID,
-        SHAMapTreeNodePtr treeNode,
-        SHAMapSyncFilter const* filter);
+    addKnownNode(SHAMapNodeID const& nodeID, Slice const& rawNode, SHAMapSyncFilter* filter);
 
     // status functions
     void
@@ -388,11 +343,11 @@ private:
     SHAMapTreeNodePtr
     fetchNodeNT(SHAMapHash const& hash) const;
     SHAMapTreeNodePtr
-    fetchNodeNT(SHAMapHash const& hash, SHAMapSyncFilter const* filter) const;
+    fetchNodeNT(SHAMapHash const& hash, SHAMapSyncFilter* filter) const;
     SHAMapTreeNodePtr
     fetchNode(SHAMapHash const& hash) const;
     SHAMapTreeNodePtr
-    checkFilter(SHAMapHash const& hash, SHAMapSyncFilter const* filter) const;
+    checkFilter(SHAMapHash const& hash, SHAMapSyncFilter* filter) const;
 
     /** Update hashes up to the root */
     void
@@ -456,7 +411,7 @@ private:
     descendAsync(
         SHAMapInnerNode* parent,
         int branch,
-        SHAMapSyncFilter const* filter,
+        SHAMapSyncFilter* filter,
         bool& pending,
         descendCallback&&) const;
 
@@ -465,7 +420,7 @@ private:
         SHAMapInnerNode* parent,
         SHAMapNodeID const& parentID,
         int branch,
-        SHAMapSyncFilter const* filter) const;
+        SHAMapSyncFilter* filter) const;
 
     // Non-storing
     // Does not hook the returned node to its parent

src/libxrpl/ledger/helpers/LendingHelpers.cpp

@@ -130,6 +130,50 @@ isRounded(Asset const& asset, Number const& value, std::int32_t scale)
         roundToAsset(asset, value, scale, Number::RoundingMode::Upward);
 }
 
+Number
+computeDefaultCovered(
+    bool useProportionalFormula,
+    std::uint32_t coverRateLiquidation,
+    Number const& coverAvailable,
+    Asset const& vaultAsset,
+    Number const& totalDefaultAmount,
+    Number const& brokerDebtTotal,
+    TenthBips32 coverRateMinimum,
+    std::int32_t loanScale)
+{
+    // Always round the minimum required up.
+    NumberRoundModeGuard const mg(Number::RoundingMode::Upward);
+    Number covered;
+
+    if (useProportionalFormula)
+    {
+        // New formula: DefaultCovered = min(DefaultAmount × CoverRateMinimum,
+        //                                   CoverAvailable)
+        covered = roundToAsset(
+            vaultAsset, tenthBipsOfValue(totalDefaultAmount, coverRateMinimum), loanScale);
+    }
+    else
+    {
+        // Old formula (deprecated by featureLendingProtocolV1_1):
+        // Kept for backwards compatibility with brokers that still carry
+        // sfCoverRateLiquidation.
+        auto const minimumCover = tenthBipsOfValue(brokerDebtTotal, coverRateMinimum);
+        covered = roundToAsset(
+            vaultAsset,
+            /*
+             * This formula is from the XLS-66 spec, section 3.2.3.2 (State
+             * Changes), specifically "if the `tfLoanDefault` flag is set" /
+             * "Apply the First-Loss Capital to the Default Amount"
+             */
+            std::min(
+                tenthBipsOfValue(minimumCover, TenthBips32{coverRateLiquidation}),
+                totalDefaultAmount),
+            loanScale);
+    }
+
+    return std::min(covered, coverAvailable);
+}
+
 namespace detail {
 
 void

src/libxrpl/shamap/SHAMap.cpp

@@ -206,7 +206,7 @@ SHAMap::finishFetch(SHAMapHash const& hash, std::shared_ptr<NodeObject> const& o
 
 // See if a sync filter has a node
 SHAMapTreeNodePtr
-SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter const* filter) const
+SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter* filter) const
 {
     if (auto nodeData = filter->getNode(hash))
     {
@@ -232,7 +232,7 @@ SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter const* filter) cons
 // Get a node without throwing
 // Used on maps where missing nodes are expected
 SHAMapTreeNodePtr
-SHAMap::fetchNodeNT(SHAMapHash const& hash, SHAMapSyncFilter const* filter) const
+SHAMap::fetchNodeNT(SHAMapHash const& hash, SHAMapSyncFilter* filter) const
 {
     auto node = cacheLookup(hash);
     if (node)
@@ -345,7 +345,7 @@ SHAMap::descend(
     SHAMapInnerNode* parent,
     SHAMapNodeID const& parentID,
     int branch,
-    SHAMapSyncFilter const* filter) const
+    SHAMapSyncFilter* filter) const
 {
     XRPL_ASSERT(parent->isInner(), "xrpl::SHAMap::descend : valid parent input");
     XRPL_ASSERT(
@@ -374,7 +374,7 @@ SHAMapTreeNode*
 SHAMap::descendAsync(
     SHAMapInnerNode* parent,
     int branch,
-    SHAMapSyncFilter const* filter,
+    SHAMapSyncFilter* filter,
     bool& pending,
     descendCallback&& callback) const
 {

src/libxrpl/shamap/SHAMapNodeID.cpp

@@ -129,8 +129,7 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash)
 SHAMapNodeID
 SHAMapNodeID::createID(int depth, uint256 const& key)
 {
-    XRPL_ASSERT(
-        depth >= 0 && depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth");
+    XRPL_ASSERT((depth >= 0) && (depth < 65), "xrpl::SHAMapNodeID::createID : valid branch input");
     return SHAMapNodeID(depth, key & depthMask(depth));
 }
 

src/libxrpl/shamap/SHAMapSync.cpp

@@ -413,7 +413,7 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter* filter)
 bool
 SHAMap::getNodeFat(
     SHAMapNodeID const& wanted,
-    std::vector<SHAMapNodeData>& data,
+    std::vector<std::pair<SHAMapNodeID, Blob>>& data,
     bool fatLeaves,
     std::uint32_t depth) const
 {
@@ -449,16 +449,17 @@ SHAMap::getNodeFat(
     std::stack<std::tuple<SHAMapTreeNode*, SHAMapNodeID, int>> stack;
     stack.emplace(node, nodeID, depth);
 
+    Serializer s(8192);
+
     while (!stack.empty())
     {
         std::tie(node, nodeID, depth) = stack.top();
         stack.pop();
 
-        // Use a fresh Serializer per node and move its buffer into `data` rather than copying it
-        // via Serializer::getData(): the move is O(1) whereas the copy was O(node size).
-        Serializer s(256);
+        // Add this node to the reply
+        s.erase();
         node->serializeForWire(s);
-        data.emplace_back(nodeID, node->isLeaf(), std::move(s.modData()));
+        data.emplace_back(nodeID, s.getData());
 
         if (node->isInner())
         {
@@ -486,10 +487,9 @@ SHAMap::getNodeFat(
                         else if (childNode->isInner() || fatLeaves)
                         {
                             // Just include this node
-                            Serializer cs(256);
-                            childNode->serializeForWire(cs);
-                            data.emplace_back(
-                                childID, childNode->isLeaf(), std::move(cs.modData()));
+                            s.erase();
+                            childNode->serializeForWire(s);
+                            data.emplace_back(childID, s.getData());
                         }
                     }
                 }
@@ -507,32 +507,25 @@ SHAMap::serializeRoot(Serializer& s) const
 }
 
 SHAMapAddNode
-SHAMap::addRootNode(
-    SHAMapHash const& hash,
-    SHAMapTreeNodePtr rootNode,
-    SHAMapSyncFilter const* filter)
+SHAMap::addRootNode(SHAMapHash const& hash, Slice const& rootNode, SHAMapSyncFilter* filter)
 {
-    XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid");
-    XRPL_ASSERT(rootNode, "xrpl::SHAMap::addRootNode : non-null root node");
-
     // we already have a root_ node
     if (root_->getHash().isNonZero())
     {
-        JLOG(journal_.trace()) << "Got root node, already have one";
-        XRPL_ASSERT(root_->getHash() == hash, "xrpl::SHAMap::addRootNode : valid hash");
+        JLOG(journal_.trace()) << "got root node, already have one";
+        XRPL_ASSERT(root_->getHash() == hash, "xrpl::SHAMap::addRootNode : valid hash input");
         return SHAMapAddNode::duplicate();
     }
 
-    if (rootNode->getHash() != hash)
-    {
-        JLOG(journal_.warn()) << "Corrupt node received";
+    XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid");
+    auto node = SHAMapTreeNode::makeFromWire(rootNode);
+    if (!node || node->getHash() != hash)
         return SHAMapAddNode::invalid();
-    }
 
     if (backed_)
-        canonicalize(hash, rootNode);
+        canonicalize(hash, node);
 
-    root_ = std::move(rootNode);
+    root_ = node;
 
     if (root_->isLeaf())
         clearSynching();
@@ -549,20 +542,9 @@ SHAMap::addRootNode(
 }
 
 SHAMapAddNode
-SHAMap::addKnownNode(
-    SHAMapNodeID const& nodeID,
-    SHAMapTreeNodePtr treeNode,
-    SHAMapSyncFilter const* filter)
+SHAMap::addKnownNode(SHAMapNodeID const& node, Slice const& rawNode, SHAMapSyncFilter* filter)
 {
-    XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node");
-    XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node");
-    XRPL_ASSERT(
-        !treeNode->isLeaf() ||
-            SHAMapNodeID::createID(
-                nodeID.getDepth(),
-                safeDowncast<SHAMapLeafNode const*>(treeNode.get())->peekItem()->key())
-                    .getNodeID() == nodeID.getNodeID(),
-        "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID");
+    XRPL_ASSERT(!node.isRoot(), "xrpl::SHAMap::addKnownNode : valid node input");
 
     if (!isSynching())
     {
@@ -576,14 +558,14 @@ SHAMap::addKnownNode(
 
     while (currNode->isInner() &&
            !safeDowncast<SHAMapInnerNode*>(currNode)->isFullBelow(generation) &&
-           (currNodeID.getDepth() < nodeID.getDepth()))
+           (currNodeID.getDepth() < node.getDepth()))
     {
-        int const branch = selectBranch(currNodeID, nodeID.getNodeID());
+        int const branch = selectBranch(currNodeID, node.getNodeID());
         XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::addKnownNode : valid branch");
         auto inner = safeDowncast<SHAMapInnerNode*>(currNode);
         if (inner->isEmptyBranch(branch))
         {
-            JLOG(journal_.warn()) << "Add known node for empty branch" << nodeID;
+            JLOG(journal_.warn()) << "Add known node for empty branch" << node;
             return SHAMapAddNode::invalid();
         }
 
@@ -599,44 +581,67 @@ SHAMap::addKnownNode(
         if (currNode != nullptr)
             continue;
 
-        if (childHash != treeNode->getHash())
+        auto newNode = SHAMapTreeNode::makeFromWire(rawNode);
+
+        if (!newNode || childHash != newNode->getHash())
         {
             JLOG(journal_.warn()) << "Corrupt node received";
             return SHAMapAddNode::invalid();
         }
 
+        // In rare cases, a node can still be corrupt even after hash
+        // validation. For leaf nodes, we perform an additional check to
+        // ensure the node's position in the tree is consistent with its
+        // content to prevent inconsistencies that could
+        // propagate further down the line.
+        if (newNode->isLeaf())
+        {
+            auto const& actualKey =
+                safeDowncast<SHAMapLeafNode const*>(newNode.get())->peekItem()->key();
+
+            // Validate that this leaf belongs at the target position
+            auto const expectedNodeID = SHAMapNodeID::createID(node.getDepth(), actualKey);
+            if (expectedNodeID.getNodeID() != node.getNodeID())
+            {
+                JLOG(journal_.debug())
+                    << "Leaf node position mismatch: "
+                    << "expected=" << expectedNodeID.getNodeID() << ", actual=" << node.getNodeID();
+                return SHAMapAddNode::invalid();
+            }
+        }
+
         // Inner nodes must be at a level strictly less than 64
         // but leaf nodes (while notionally at level 64) can be
         // at any depth up to and including 64:
         if ((currNodeID.getDepth() > kLeafDepth) ||
-            (treeNode->isInner() && currNodeID.getDepth() == kLeafDepth))
+            (newNode->isInner() && currNodeID.getDepth() == kLeafDepth))
         {
             // Map is provably invalid
             state_ = SHAMapState::Invalid;
             return SHAMapAddNode::useful();
         }
 
-        if (currNodeID != nodeID)
+        if (currNodeID != node)
         {
             // Either this node is broken or we didn't request it (yet)
-            JLOG(journal_.warn()) << "unable to hook node " << nodeID;
+            JLOG(journal_.warn()) << "unable to hook node " << node;
             JLOG(journal_.info()) << " stuck at " << currNodeID;
-            JLOG(journal_.info()) << "got depth=" << nodeID.getDepth()
+            JLOG(journal_.info()) << "got depth=" << node.getDepth()
                                   << ", walked to= " << currNodeID.getDepth();
             return SHAMapAddNode::useful();
         }
 
         if (backed_)
-            canonicalize(childHash, treeNode);
+            canonicalize(childHash, newNode);
 
-        treeNode = prevNode->canonicalizeChild(branch, std::move(treeNode));
+        newNode = prevNode->canonicalizeChild(branch, std::move(newNode));
 
         if (filter != nullptr)
         {
             Serializer s;
-            treeNode->serializeWithPrefix(s);
+            newNode->serializeWithPrefix(s);
             filter->gotNode(
-                false, childHash, ledgerSeq_, std::move(s.modData()), treeNode->getType());
+                false, childHash, ledgerSeq_, std::move(s.modData()), newNode->getType());
         }
 
         return SHAMapAddNode::useful();

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

@@ -67,6 +67,9 @@ LoanBrokerSet::preflight(PreflightContext const& ctx)
             return temINVALID;
     }
 
+    // sfCoverRateLiquidation is deprecated by featureDefaultCoverOptimization;
+    // only enforce consistency when the amendment is not enabled.
+    if (!ctx.rules.enabled(featureDefaultCoverOptimization))
     {
         auto const minimumZero = tx[~sfCoverRateMinimum].value_or(0) == 0;
         auto const liquidationZero = tx[~sfCoverRateLiquidation].value_or(0) == 0;
@@ -267,7 +270,8 @@ LoanBrokerSet::doApply()
             broker->at(sfDebtMaximum) = *debtMax;
         if (auto const coverMin = tx[~sfCoverRateMinimum])
             broker->at(sfCoverRateMinimum) = *coverMin;
-        if (auto const coverLiq = tx[~sfCoverRateLiquidation])
+        if (auto const coverLiq = tx[~sfCoverRateLiquidation];
+            coverLiq && !view.rules().enabled(featureDefaultCoverOptimization))
             broker->at(sfCoverRateLiquidation) = *coverLiq;
 
         view.insert(broker);

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

@@ -162,25 +162,16 @@ LoanManage::defaultLoan(
 
     // Apply the First-Loss Capital to the Default Amount
     TenthBips32 const coverRateMinimum{brokerSle->at(sfCoverRateMinimum)};
-    TenthBips32 const coverRateLiquidation{brokerSle->at(sfCoverRateLiquidation)};
-    auto const defaultCovered = [&]() {
-        // Always round the minimum required up.
-        NumberRoundModeGuard const mg(Number::RoundingMode::Upward);
-        auto const minimumCover = tenthBipsOfValue(brokerDebtTotalProxy.value(), coverRateMinimum);
-        // Round the liquidation amount up, too
-        auto const covered = roundToAsset(
-            vaultAsset,
-            /*
-             * This formula is from the XLS-66 spec, section 3.2.3.2 (State
-             * Changes), specifically "if the `tfLoanDefault` flag is set" /
-             * "Apply the First-Loss Capital to the Default Amount"
-             */
-            std::min(tenthBipsOfValue(minimumCover, coverRateLiquidation), totalDefaultAmount),
-            loanScale);
-        auto const coverAvailable = *brokerSle->at(sfCoverAvailable);
 
-        return std::min(covered, coverAvailable);
-    }();
+    auto const defaultCovered = computeDefaultCovered(
+        useProportionalDefaultCover(view.rules(), brokerSle),
+        brokerSle->at(sfCoverRateLiquidation),
+        *brokerSle->at(sfCoverAvailable),
+        vaultAsset,
+        totalDefaultAmount,
+        brokerDebtTotalProxy.value(),
+        coverRateMinimum,
+        loanScale);
 
     auto const vaultDefaultAmount = totalDefaultAmount - defaultCovered;
 

src/test/app/LedgerNodeHelpers_test.cpp

@@ -1,261 +0,0 @@
-#include <xrpld/app/ledger/LedgerNodeHelpers.h>
-
-#include <xrpl/basics/IntrusivePointer.h>
-#include <xrpl/basics/base_uint.h>
-#include <xrpl/beast/unit_test/suite.h>
-#include <xrpl/protocol/Serializer.h>
-#include <xrpl/shamap/SHAMap.h>
-#include <xrpl/shamap/SHAMapAccountStateLeafNode.h>
-#include <xrpl/shamap/SHAMapInnerNode.h>
-#include <xrpl/shamap/SHAMapItem.h>
-#include <xrpl/shamap/SHAMapTreeNode.h>
-
-#include <boost/smart_ptr/intrusive_ptr.hpp>
-
-#include <xrpl.pb.h>
-
-#include <bit>
-#include <cstdint>
-#include <string>
-
-namespace xrpl::tests {
-
-class LedgerNodeHelpers_test : public beast::unit_test::Suite
-{
-    static boost::intrusive_ptr<SHAMapItem>
-    makeTestItem(std::uint32_t seed)
-    {
-        Serializer s;
-        s.add32(seed);
-        s.add32(seed + 1);
-        s.add32(seed + 2);
-        return makeShamapitem(s.getSHA512Half(), s.slice());
-    }
-
-    static std::string
-    serializeNode(SHAMapTreeNodePtr const& node)
-    {
-        Serializer s;
-        node->serializeForWire(s);
-        auto const slice = s.slice();
-        return std::string(std::bit_cast<char const*>(slice.data()), slice.size());
-    }
-
-    void
-    testGetTreeNode()
-    {
-        testcase("getTreeNode");
-
-        // Valid: inner node. It must have at least one child for `serializeNode` to work.
-        {
-            auto const innerNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
-            auto const childNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
-            innerNode->setChild(0, childNode);
-            auto const innerData = serializeNode(innerNode);
-            auto const result = getTreeNode(innerData);
-            BEAST_EXPECT(result && result->isInner());
-        }
-
-        // Valid: leaf node.
-        {
-            auto const leafItem = makeTestItem(12345);
-            auto const leafNode = intr_ptr::makeShared<SHAMapAccountStateLeafNode>(leafItem, 1);
-            auto const leafData = serializeNode(leafNode);
-            auto const result = getTreeNode(leafData);
-            BEAST_EXPECT(result && result->isLeaf());
-        }
-
-        // Invalid: empty data.
-        {
-            auto const result = getTreeNode("");
-            BEAST_EXPECT(!result);
-        }
-
-        // Invalid: garbage data.
-        {
-            auto const result = getTreeNode("invalid");
-            BEAST_EXPECT(!result);
-        }
-
-        // Invalid: truncated data.
-        {
-            auto const leafItem = makeTestItem(54321);
-            auto const leafNode = intr_ptr::makeShared<SHAMapAccountStateLeafNode>(leafItem, 1);
-            // Truncate the data to trigger an exception in SHAMapTreeNode::makeAccountState when
-            // the data is used to deserialize the node.
-            uint256 const tag;
-            auto const leafData = serializeNode(leafNode).substr(0, tag.kBytes - 1);
-            auto const result = getTreeNode(leafData);
-            BEAST_EXPECT(!result);
-        }
-    }
-
-    void
-    testGetSHAMapNodeID()
-    {
-        testcase("getSHAMapNodeID");
-
-        {
-            // Tests using inner nodes at various depths.
-            auto const innerNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
-            auto const childNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
-            innerNode->setChild(0, childNode);
-            auto const innerData = serializeNode(innerNode);
-
-            // Valid: legacy `nodeid` field at arbitrary depth.
-            {
-                auto const innerDepth = 3;
-                auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(innerData);
-                ledgerNode.set_nodeid(innerID.getRawString());
-                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
-                BEAST_EXPECT(result == innerID);
-            }
-
-            // Valid: new `id` field at minimum depth.
-            {
-                auto const innerDepth = 0;
-                auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(innerData);
-                ledgerNode.set_id(innerID.getRawString());
-                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
-                BEAST_EXPECT(result == innerID);
-            }
-
-            // Invalid: new `depth` field should not be used for inner nodes.
-            {
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(innerData);
-                ledgerNode.set_depth(10);
-                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
-                BEAST_EXPECT(!result);
-            }
-
-            // Invalid: both legacy `nodeid` and new `id` fields set for an inner node.
-            {
-                auto const innerDepth = 9;
-                auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(innerData);
-                ledgerNode.set_nodeid(innerID.getRawString());
-                ledgerNode.set_id(innerID.getRawString());
-                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
-                BEAST_EXPECT(!result);
-            }
-        }
-
-        {
-            // Tests using leaf nodes at various depths.
-            auto const leafItem = makeTestItem(12345);
-            auto const leafNode = intr_ptr::makeShared<SHAMapAccountStateLeafNode>(leafItem, 1);
-            auto const leafData = serializeNode(leafNode);
-            auto const leafKey = leafItem->key();
-
-            // Valid: legacy `nodeid` field at arbitrary depth.
-            {
-                auto const kLeafDepth = 5;
-                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(leafData);
-                ledgerNode.set_nodeid(leafID.getRawString());
-                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
-                BEAST_EXPECT(result == leafID);
-            }
-
-            // Invalid: new `id` field should not be used for leaf nodes.
-            {
-                auto const kLeafDepth = 5;
-                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(leafData);
-                ledgerNode.set_id(leafID.getRawString());
-                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
-                BEAST_EXPECT(!result);
-            }
-
-            // Valid: new `depth` field at minimum depth.
-            {
-                auto const kLeafDepth = 0;
-                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(leafData);
-                ledgerNode.set_depth(kLeafDepth);
-                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
-                BEAST_EXPECT(result == leafID);
-            }
-
-            // Valid: new `depth` field at arbitrary depth between minimum and maximum.
-            {
-                auto const kLeafDepth = 10;
-                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(leafData);
-                ledgerNode.set_depth(kLeafDepth);
-                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
-                BEAST_EXPECT(result == leafID);
-            }
-
-            // Valid: new `depth` field at maximum depth.
-            // Note that we do not test a depth greater than the maximum depth, because the proto
-            // message is assumed to have been validated by the time the getSHAMapNodeID function is
-            // called.
-            {
-                auto const kLeafDepth = SHAMap::kLeafDepth;
-                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(leafData);
-                ledgerNode.set_depth(kLeafDepth);
-                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
-                BEAST_EXPECT(result == leafID);
-            }
-
-            // Invalid: legacy `nodeid` field where the node ID is inconsistent with the key.
-            {
-                auto const otherItem = makeTestItem(54321);
-                auto const otherNode =
-                    intr_ptr::makeShared<SHAMapAccountStateLeafNode>(otherItem, 1);
-                auto const otherData = serializeNode(otherNode);
-                auto const otherKey = otherItem->key();
-                auto const otherDepth = 1;
-                auto const otherID = SHAMapNodeID::createID(otherDepth, otherKey);
-
-                protocol::TMLedgerNode ledgerNode;
-                ledgerNode.set_nodedata(otherData);
-                ledgerNode.set_nodeid(otherID.getRawString());
-                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
-                BEAST_EXPECT(!result);
-            }
-        }
-
-        // Invalid: no field set.
-        {
-            auto const innerNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
-            protocol::TMLedgerNode ledgerNode;
-            ledgerNode.set_nodedata("test_data");
-            auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
-            BEAST_EXPECT(!result);
-        }
-    }
-
-public:
-    void
-    run() override
-    {
-        testGetTreeNode();
-        testGetSHAMapNodeID();
-    }
-};
-
-BEAST_DEFINE_TESTSUITE(LedgerNodeHelpers, app, xrpl);
-
-}  // namespace xrpl::tests

src/test/app/LendingHelpers_test.cpp

@@ -1470,6 +1470,199 @@ class LendingHelpers_test : public beast::unit_test::Suite
              Number{-18304, -5}));
     }
 
+    void
+    testComputeDefaultCovered()
+    {
+        testcase("computeDefaultCovered");
+
+        using namespace jtx;
+
+        // ---- Common parameters ----
+        Asset const asset{xrpIssue()};
+        std::int32_t const loanScale = 1;
+        // coverRateLiquidation value used by old-formula tests (100%).
+        std::uint32_t const covRateLiq = 100'000;
+
+        // ---- Test 1: New formula basic ----
+        // DefaultCovered = min(DefaultAmount × CoverRateMinimum,
+        //                      CoverAvailable)
+        // 100,000 × 20% = 20,000; min(20,000, 50,000) = 20,000
+        {
+            auto result = computeDefaultCovered(
+                true,            // useProportionalFormula
+                0,               // coverRateLiquidation (unused)
+                Number{50'000},  // coverAvailable
+                asset,
+                Number{100'000},      // totalDefaultAmount
+                Number{200'000},      // brokerDebtTotal (unused)
+                TenthBips32{20'000},  // 20%
+                loanScale);
+            BEAST_EXPECT(result == Number{20'000});
+        }
+
+        // ---- Test 2: New formula capped by CoverAvailable ----
+        // 100,000 × 50% = 50,000; min(50,000, 10,000) = 10,000
+        {
+            auto result = computeDefaultCovered(
+                true,
+                0,
+                Number{10'000},
+                asset,
+                Number{100'000},
+                Number{200'000},
+                TenthBips32{50'000},  // 50%
+                loanScale);
+            BEAST_EXPECT(result == Number{10'000});
+        }
+
+        // ---- Test 3: Old formula basic ----
+        // min(CovRateLiq × (CovRateMin × BrokerDebtTotal), DefaultAmount)
+        // minimumCover = 200,000 × 20% = 40,000
+        // covered = min(100% × 40,000, 50,000) = 40,000
+        // min(40,000, 100,000) = 40,000
+        {
+            auto result = computeDefaultCovered(
+                false,       // old formula
+                covRateLiq,  // 100%
+                Number{100'000},
+                asset,
+                Number{50'000},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+            BEAST_EXPECT(result == Number{40'000});
+        }
+
+        // ---- Test 4: Old formula capped by DefaultAmount ----
+        // minimumCover = 200,000 × 50% = 100,000
+        // covered = min(100% × 100,000, 30,000) = 30,000
+        // min(30,000, 500,000) = 30,000
+        {
+            auto result = computeDefaultCovered(
+                false,
+                covRateLiq,
+                Number{500'000},
+                asset,
+                Number{30'000},
+                Number{200'000},
+                TenthBips32{50'000},
+                loanScale);
+            BEAST_EXPECT(result == Number{30'000});
+        }
+
+        // ---- Test 5: Old formula capped by CoverAvailable ----
+        // minimumCover = 200,000 × 20% = 40,000
+        // covered = min(100% × 40,000, 100,000) = 40,000
+        // min(40,000, 5,000) = 5,000
+        {
+            auto result = computeDefaultCovered(
+                false,
+                covRateLiq,
+                Number{5'000},  // small CoverAvailable
+                asset,
+                Number{100'000},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+            BEAST_EXPECT(result == Number{5'000});
+        }
+
+        // ---- Test 6: Backwards compatibility ----
+        // useProportionalFormula = false even though the amendment is
+        // enabled, because the broker was created before the amendment
+        // and still carries sfCoverRateLiquidation.  The caller passes
+        // false in this case; we verify the old formula is used.
+        // minimumCover = 200,000 × 20% = 40,000
+        // covered = min(100% × 40,000, 50,000) = 40,000
+        // min(40,000, 100,000) = 40,000
+        {
+            auto result = computeDefaultCovered(
+                false,  // old formula (backwards compat)
+                covRateLiq,
+                Number{100'000},
+                asset,
+                Number{50'000},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+            BEAST_EXPECT(result == Number{40'000});
+        }
+
+        // ---- Test 7: New vs old produce different results ----
+        // Same inputs, different formula selection → different outputs.
+        {
+            auto resultNew = computeDefaultCovered(
+                true,
+                0,
+                Number{100'000},
+                asset,
+                Number{50'000},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+
+            auto resultOld = computeDefaultCovered(
+                false,
+                covRateLiq,
+                Number{100'000},
+                asset,
+                Number{50'000},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+
+            // New: 50,000 × 20% = 10,000
+            BEAST_EXPECT(resultNew == Number{10'000});
+            // Old: min(100% × (20% × 200,000), 50,000)
+            //    = min(40,000, 50,000) = 40,000
+            BEAST_EXPECT(resultOld == Number{40'000});
+            BEAST_EXPECT(resultNew != resultOld);
+        }
+
+        // ---- Test 8: Zero CoverAvailable ----
+        {
+            auto result = computeDefaultCovered(
+                true,
+                0,
+                Number{0},
+                asset,
+                Number{100'000},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+            BEAST_EXPECT(result == Number{0});
+        }
+
+        // ---- Test 9: Zero DefaultAmount ----
+        {
+            auto result = computeDefaultCovered(
+                true,
+                0,
+                Number{50'000},
+                asset,
+                Number{0},
+                Number{200'000},
+                TenthBips32{20'000},
+                loanScale);
+            BEAST_EXPECT(result == Number{0});
+        }
+
+        // ---- Test 10: Zero CoverRateMinimum (new formula) ----
+        // 100,000 × 0% = 0
+        {
+            auto result = computeDefaultCovered(
+                true,
+                0,
+                Number{50'000},
+                asset,
+                Number{100'000},
+                Number{200'000},
+                TenthBips32{0},
+                loanScale);
+            BEAST_EXPECT(result == Number{0});
+        }
+    }
+
 public:
     void
     testCanApplyToBrokerCover()
@@ -1572,6 +1765,7 @@ public:
         testComputePaymentFactorNearZeroRate();
         testComputeOverpaymentComponents();
         testComputeInterestAndFeeParts();
+        testComputeDefaultCovered();
         testCanApplyToBrokerCover();
     }
 };

src/test/app/LoanBroker_test.cpp

@@ -29,6 +29,7 @@
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/OpenView.h>
+#include <xrpl/ledger/helpers/LendingHelpers.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
@@ -538,7 +539,7 @@ class LoanBroker_test : public beast::unit_test::Suite
     }
 
     void
-    testLifecycle()
+    testLifecycle(FeatureBitset features)
     {
         testcase("Lifecycle");
         using namespace jtx;
@@ -688,18 +689,26 @@ class LoanBroker_test : public beast::unit_test::Suite
                 Ter(temINVALID));
             // Cover: zero min, non-zero liquidation - implicit and
             // explicit zero values.
-            env(set(evan, vault.vaultID), kCoverRateLiquidation(kMaxCoverRate), Ter(temINVALID));
+            env(set(evan, vault.vaultID),
+                kCoverRateLiquidation(kMaxCoverRate),
+                env.enabled(featureDefaultCoverOptimization) ? Ter(tecNO_PERMISSION)
+                                                             : Ter(temINVALID));
             env(set(evan, vault.vaultID),
                 kCoverRateMinimum(tenthBipsZero),
                 kCoverRateLiquidation(kMaxCoverRate),
-                Ter(temINVALID));
+                env.enabled(featureDefaultCoverOptimization) ? Ter(tecNO_PERMISSION)
+                                                             : Ter(temINVALID));
             // Cover: non-zero min, zero liquidation - implicit and
             // explicit zero values.
-            env(set(evan, vault.vaultID), kCoverRateMinimum(kMaxCoverRate), Ter(temINVALID));
+            env(set(evan, vault.vaultID),
+                kCoverRateMinimum(kMaxCoverRate),
+                env.enabled(featureDefaultCoverOptimization) ? Ter(tecNO_PERMISSION)
+                                                             : Ter(temINVALID));
             env(set(evan, vault.vaultID),
                 kCoverRateMinimum(kMaxCoverRate),
                 kCoverRateLiquidation(tenthBipsZero),
-                Ter(temINVALID));
+                env.enabled(featureDefaultCoverOptimization) ? Ter(tecNO_PERMISSION)
+                                                             : Ter(temINVALID));
             // sfDebtMaximum: good value, bad account
             env(set(evan, vault.vaultID), kDebtMaximum(Number(0)), Ter(tecNO_PERMISSION));
             // sfDebtMaximum: overflow
@@ -827,7 +836,15 @@ class LoanBroker_test : public beast::unit_test::Suite
                     // Extra checks
                     BEAST_EXPECT(broker->at(sfManagementFeeRate) == 123);
                     BEAST_EXPECT(broker->at(sfCoverRateMinimum) == 100);
-                    BEAST_EXPECT(broker->at(sfCoverRateLiquidation) == 200);
+                    if (useProportionalDefaultCover(env.current()->rules(), broker))
+                    {
+                        BEAST_EXPECT(broker->at(sfCoverRateLiquidation) == 0);
+                    }
+                    else
+                    {
+                        BEAST_EXPECT(broker->at(sfCoverRateLiquidation) == 200);
+                    }
+
                     BEAST_EXPECT(broker->at(sfDebtMaximum) == Number(9));
                     BEAST_EXPECT(checkVL(broker->at(sfData), testData));
                 },
@@ -2250,7 +2267,8 @@ public:
         testLoanBrokerCoverDepositNullVault();
 
         testDisabled();
-        testLifecycle();
+        testLifecycle(all_);
+        testLifecycle(all_ - featureDefaultCoverOptimization);
         testInvalidLoanBrokerCoverClawback();
         testInvalidLoanBrokerCoverDeposit();
         testInvalidLoanBrokerCoverWithdraw();

src/test/app/Loan_test.cpp

@@ -2041,14 +2041,15 @@ protected:
                          : std::max(
                                broker.vaultScale(env), state.principalOutstanding.exponent())));
                 NumberRoundModeGuard const mg(Number::RoundingMode::Upward);
-                auto const defaultAmount = roundToAsset(
+                auto const totalDefaultAmount = state.totalValue - state.managementFeeOutstanding;
+                auto const defaultAmount = computeDefaultCovered(
+                    useProportionalDefaultCover(env.current()->rules(), brokerSle),
+                    brokerSle->at(~sfCoverRateLiquidation).value_or(0),
+                    brokerSle->at(sfCoverAvailable),
                     broker.asset,
-                    std::min(
-                        tenthBipsOfValue(
-                            tenthBipsOfValue(
-                                brokerSle->at(sfDebtTotal), broker.params.coverRateMin),
-                            broker.params.coverRateLiquidation),
-                        state.totalValue - state.managementFeeOutstanding),
+                    totalDefaultAmount,
+                    brokerSle->at(sfDebtTotal),
+                    broker.params.coverRateMin,
                     state.loanScale);
                 return std::make_pair(defaultAmount, brokerSle->at(sfOwner));
             }
@@ -7232,9 +7233,11 @@ protected:
 
         // Create two identical loans: each 50,000 XRP principal (scaled down to
         // avoid funding issues) Total DebtTotal will be ~100,000 XRP (principal
-        // + interest) Formula will calculate cover as: 100% × (20% × 100,000) =
+        // + interest). Old Formula will calculate cover as: 100% × (20% × 100,000) =
         // 20,000 XRP So we need FLC = 20,000 XRP to be fully consumed by first
         // default
+        // New formula: seizure = DefaultAmount × 20% ≈ 10,027 — splitting
+        //   FLC equitably across both defaults.
         auto const principalAmount = Number(50'000);
         auto const loanPaymentInterval = 2592000;  // 30 days
         auto const loanGracePeriod = 604800;       // 7 days
@@ -7293,11 +7296,38 @@ protected:
         auto const afterFirstDebtTotal = brokerSle->at(sfDebtTotal);
         auto const afterFirstCoverAvailable = brokerSle->at(sfCoverAvailable);
 
-        // DebtTotal should have decreased by Loan A's debt
-        BEAST_EXPECT(afterFirstDebtTotal == 50'134);
+        if (useProportionalDefaultCover(env.current()->rules(), brokerSle))
+        {
+            // Proportional default cover (new formula):
+            //   DefaultCovered = min(DefaultAmount × CoverRateMinimum,
+            //                        CoverAvailable)
+            // Loan A's DefaultAmount (~52,067) × 20% = 10,027 seizure
+            // Result: CoverAvailable = 21,000 - 10,027 = 10,973
 
-        // CoverAvailable should have decreased significantly
-        BEAST_EXPECT(afterFirstCoverAvailable == 946);
+            // DebtTotal should have decreased by Loan A's debt (~52,067),
+            // leaving only Loan B's debt (~50,134).
+            BEAST_EXPECT(afterFirstDebtTotal == 50'134);
+
+            // CoverAvailable should have decreased proportionally
+            BEAST_EXPECT(afterFirstCoverAvailable == 10'973);
+        }
+        else
+        {
+            // Global default cover (old formula):
+            //   DefaultCovered = min(CoverRateLiquidation ×
+            //       (CoverRateMinimum × BrokerDebtTotal), DefaultAmount)
+            // Pre-default BrokerDebtTotal (~104,201) × 20% = 20,840
+            //   then 100% × 20,840 = 20,840
+            //   but capped at DefaultAmount (~52,067), seizure = 20,054
+            // Result: CoverAvailable = 21,000 - 20,054 = 946
+
+            // DebtTotal should have decreased by Loan A's debt (~52,067),
+            // leaving only Loan B's debt (~50,134).
+            BEAST_EXPECT(afterFirstDebtTotal == 50'134);
+
+            // CoverAvailable should have decreased significantly
+            BEAST_EXPECT(afterFirstCoverAvailable == 946);
+        }
 
         env(manage(lender, loanBKeylet.key, tfLoanDefault), Ter(tesSUCCESS));
 
@@ -7309,7 +7339,22 @@ protected:
 
         BEAST_EXPECT(afterSecondDebtTotal == 0);
 
-        BEAST_EXPECT(afterSecondCoverAvailable == 0);
+        if (useProportionalDefaultCover(env.current()->rules(), brokerSle))
+        {
+            // Proportional default cover (new formula):
+            // Loan B's DefaultAmount (~50,134) × 20% = 10,027 seizure
+            // Result: CoverAvailable = 10,973 - 10,027 = 946
+            //
+            // Both loans are covered equitably with a safety buffer remaining.
+            BEAST_EXPECT(afterSecondCoverAvailable == 946);
+        }
+        else
+        {
+            // Global default cover (old formula):
+            // Only 946 remains to cover Loan B's DefaultAmount (~50,134)
+            // Result: CoverAvailable = 0 (fully depleted)
+            BEAST_EXPECT(afterSecondCoverAvailable == 0);
+        }
     }
 
     void
@@ -8702,6 +8747,200 @@ protected:
             });
     }
 
+    void
+    testCoverRateLiquidationAmendmentGating(FeatureBitset const& features)
+    {
+        testcase("CoverRateLiquidation amendment gating");
+
+        using namespace jtx;
+        using namespace loanBroker;
+
+        auto const coverRateLiqValue = percentageToTenthBips(25);
+
+        {
+            Env env(*this, features);
+
+            Account const lender{"lender"};
+            env.fund(XRP(10'000'000), lender);
+            env.close();
+
+            PrettyAsset const xrpAsset{xrpIssue(), 1'000'000};
+
+            BrokerParameters brokerParams{.coverRateLiquidation = coverRateLiqValue};
+            BrokerInfo broker{createVaultAndBroker(env, xrpAsset, lender, brokerParams)};
+
+            auto const brokerSle = env.le(keylet::loanbroker(broker.brokerID));
+            if (BEAST_EXPECT(brokerSle))
+            {
+                bool const withAmendment = env.enabled(featureDefaultCoverOptimization);
+                if (withAmendment)
+                {
+                    // When featureDefaultCoverOptimization IS enabled,
+                    // sfCoverRateLiquidation should NOT be recorded on the broker.
+                    BEAST_EXPECT(!brokerSle->isFieldPresent(sfCoverRateLiquidation));
+                }
+                else
+                {
+                    // When featureDefaultCoverOptimization is NOT enabled,
+                    // sfCoverRateLiquidation should be recorded on the broker.
+                    BEAST_EXPECT(brokerSle->isFieldPresent(sfCoverRateLiquidation));
+                    BEAST_EXPECT(
+                        brokerSle->at(sfCoverRateLiquidation) == coverRateLiqValue.value());
+                }
+            }
+        }
+    }
+
+    void
+    testCoverRateLiquidationBackwardsCompat()
+    {
+        testcase("CoverRateLiquidation backwards compatibility on default");
+
+        // Verify that the default cover formula honours whether
+        // sfCoverRateLiquidation is present on the broker SLE,
+        // regardless of the amendment state:
+        //
+        //   * A broker created BEFORE the amendment has the field →
+        //     old formula (global) is used even after the amendment
+        //     is enabled.
+        //
+        //   * A broker created AFTER the amendment lacks the field →
+        //     new formula (proportional) is used.
+
+        using namespace jtx;
+        using namespace loan;
+        using namespace loanBroker;
+
+        // ---- helpers shared by both sub-tests ----
+        auto const coverRateMin = TenthBips32(20'000);        // 20 %
+        auto const coverRateLiq = percentageToTenthBips(25);  // 25 % (default)
+        auto const principalAmount = Number(50'000);
+        auto const loanPaymentInterval = 2'592'000;  // 30 days
+        auto const loanGracePeriod = 604'800;        //  7 days
+
+        // Lambda that creates one loan, advances past the grace period,
+        // defaults it, and returns the CoverAvailable after default.
+        auto defaultOneLoan = [&](Env& env,
+                                  BrokerInfo const& brokerInfo,
+                                  Account const& lender,
+                                  Account const& borrower) -> Number {
+            auto const brokerKeylet = brokerInfo.brokerKeylet();
+
+            auto loanTx = env.jt(
+                set(borrower, brokerKeylet.key, principalAmount),
+                Sig(sfCounterpartySignature, lender),
+                kInterestRate(TenthBips32(500)),  // 5 %
+                kPaymentTotal(12),
+                loan::kPaymentInterval(loanPaymentInterval),
+                loan::kGracePeriod(loanGracePeriod),
+                Fee(XRP(10)));
+            env(loanTx);
+            env.close();
+
+            auto const loanKeylet = keylet::loan(brokerKeylet.key, 1);
+            auto loanSle = env.le(loanKeylet);
+            if (!BEAST_EXPECT(loanSle))
+                return Number{-1};
+
+            auto const nextDue = loanSle->at(sfNextPaymentDueDate);
+            auto const grace = loanSle->at(sfGracePeriod);
+            env.close(std::chrono::seconds{nextDue + grace + 60});
+
+            env(manage(lender, loanKeylet.key, tfLoanDefault), Ter(tesSUCCESS));
+            env.close();
+
+            auto brokerSle = env.le(brokerKeylet);
+            if (!BEAST_EXPECT(brokerSle))
+                return Number{-1};
+            return brokerSle->at(sfCoverAvailable);
+        };
+
+        // ---- Sub-test A: pre-amendment broker (old formula) ----
+        Number coverAfterOld;
+        {
+            // Start WITHOUT the amendment so the broker stores the field.
+            Env env(*this, all_ - featureDefaultCoverOptimization);
+
+            Account const lender{"lender"};
+            Account const borrower{"borrower"};
+            env.fund(XRP(1'000'000), lender, borrower);
+            env.close();
+
+            PrettyAsset const asset = xrpIssue();
+            auto const brokerInfo = createVaultAndBroker(
+                env,
+                asset,
+                lender,
+                {
+                    .vaultDeposit = Number(200'000),
+                    .debtMax = 0,
+                    .coverRateMin = coverRateMin,
+                    .coverDeposit = 21'000,
+                    .managementFeeRate = TenthBips16(100),
+                    .coverRateLiquidation = coverRateLiq,
+                });
+
+            // Confirm the field was stored.
+            {
+                auto sle = env.le(brokerInfo.brokerKeylet());
+                BEAST_EXPECT(sle && sle->isFieldPresent(sfCoverRateLiquidation));
+            }
+
+            // Now enable the amendment – the broker keeps its field.
+            env.enableFeature(featureDefaultCoverOptimization);
+            env.close();
+
+            BEAST_EXPECT(env.enabled(featureDefaultCoverOptimization));
+
+            coverAfterOld = defaultOneLoan(env, brokerInfo, lender, borrower);
+        }
+
+        // ---- Sub-test B: post-amendment broker (new formula) ----
+        Number coverAfterNew;
+        {
+            Env env(*this, all_);  // amendment already enabled
+
+            Account const lender{"lender"};
+            Account const borrower{"borrower"};
+            env.fund(XRP(1'000'000), lender, borrower);
+            env.close();
+
+            PrettyAsset const asset = xrpIssue();
+            // Pass coverRateLiquidation in BrokerParameters, but the
+            // amendment-gated code will NOT store it on the SLE.
+            auto const brokerInfo = createVaultAndBroker(
+                env,
+                asset,
+                lender,
+                {
+                    .vaultDeposit = Number(200'000),
+                    .debtMax = 0,
+                    .coverRateMin = coverRateMin,
+                    .coverDeposit = 21'000,
+                    .managementFeeRate = TenthBips16(100),
+                    .coverRateLiquidation = coverRateLiq,
+                });
+
+            // Confirm the field was NOT stored.
+            {
+                auto sle = env.le(brokerInfo.brokerKeylet());
+                BEAST_EXPECT(sle && !sle->isFieldPresent(sfCoverRateLiquidation));
+            }
+
+            coverAfterNew = defaultOneLoan(env, brokerInfo, lender, borrower);
+        }
+
+        // Old (global) formula with 25% CoverRateLiquidation:
+        //   min(25% × (20% × 50,134), 50,134) = min(2,507, 50,134)
+        //   = 2,507 seized → CoverAvailable = 21,000 - 2,507 = 18,493
+        BEAST_EXPECT(coverAfterOld == Number{18'493});
+
+        // New (proportional) formula:
+        //   min(20% × 50,134, 21,000) = min(10,027, 21,000)
+        //   = 10,027 seized → CoverAvailable = 21,000 - 10,027 = 10,973
+        BEAST_EXPECT(coverAfterNew == Number{10'973});
+    }
+
     void
     runAmendmentIndependent()
     {
@@ -8726,6 +8965,9 @@ protected:
         testBugInterestDueDeltaCrash();
         testFullLifecycleVaultPnLNearZeroRate();
         testLoanSetNearZeroInterestRateSucceeds();
+
+        // Default cover optimization
+        testCoverRateLiquidationBackwardsCompat();
     }
 
     // Tests run under each entry in amendmentCombinations().
@@ -8780,6 +9022,9 @@ protected:
         testLoanPayBrokerOwnerUnauthorizedMPT(features);
         testLoanPayBrokerOwnerNoPermissionedDomainMPT(features);
         testLoanSetBrokerOwnerNoPermissionedDomainMPT(features);
+
+        // Default cover optimization
+        testCoverRateLiquidationAmendmentGating(features);
     }
 
 public:

src/test/overlay/ProtocolVersion_test.cpp

@@ -63,8 +63,8 @@ public:
                 negotiateProtocolVersion("RTXP/1.2, XRPL/2.0, XRPL/2.1") == makeProtocol(2, 1));
             BEAST_EXPECT(negotiateProtocolVersion("XRPL/2.2") == makeProtocol(2, 2));
             BEAST_EXPECT(
-                negotiateProtocolVersion("RTXP/1.2, XRPL/2.3, XRPL/2.4, XRPL/999.999") ==
-                makeProtocol(2, 3));
+                negotiateProtocolVersion("RTXP/1.2, XRPL/2.2, XRPL/2.3, XRPL/999.999") ==
+                makeProtocol(2, 2));
             BEAST_EXPECT(negotiateProtocolVersion("XRPL/999.999, WebSocket/1.0") == std::nullopt);
             BEAST_EXPECT(negotiateProtocolVersion("") == std::nullopt);
         }

src/test/shamap/SHAMapSync_test.cpp

@@ -1,6 +1,7 @@
 #include <test/shamap/common.h>
 #include <test/unit_test/SuiteJournal.h>
 
+#include <xrpl/basics/Blob.h>
 #include <xrpl/basics/SHAMapHash.h>
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
@@ -115,18 +116,14 @@ public:
         destination.setSynching();
 
         {
-            std::vector<SHAMapNodeData> a;
+            std::vector<std::pair<SHAMapNodeID, Blob>> a;
 
             BEAST_EXPECT(source.getNodeFat(SHAMapNodeID(), a, randBool(eng), randInt(eng, 2)));
 
             unexpected(a.empty(), "NodeSize");
 
-            auto node = SHAMapTreeNode::makeFromWire(makeSlice(a[0].data));
-            if (!node)
-                fail("", __FILE__, __LINE__);
-            BEAST_EXPECT(a[0].isLeaf == node->isLeaf());
-            BEAST_EXPECT(
-                destination.addRootNode(source.getHash(), std::move(node), nullptr).isGood());
+            BEAST_EXPECT(destination.addRootNode(source.getHash(), makeSlice(a[0].second), nullptr)
+                             .isGood());
         }
 
         do
@@ -140,7 +137,7 @@ public:
                 break;
 
             // get as many nodes as possible based on this information
-            std::vector<SHAMapNodeData> b;
+            std::vector<std::pair<SHAMapNodeID, Blob>> b;
 
             for (auto& it : nodesMissing)
             {
@@ -162,12 +159,8 @@ public:
                 // Don't use BEAST_EXPECT here b/c it will be called a
                 // non-deterministic number of times and the number of tests run
                 // should be deterministic
-                auto node = SHAMapTreeNode::makeFromWire(makeSlice(b[i].data));
-                if (!node)
-                    fail("", __FILE__, __LINE__);
-                if (b[i].isLeaf != node->isLeaf())
-                    fail("", __FILE__, __LINE__);
-                if (!destination.addKnownNode(b[i].nodeID, std::move(node), nullptr).isUseful())
+                if (!destination.addKnownNode(b[i].first, makeSlice(b[i].second), nullptr)
+                         .isUseful())
                     fail("", __FILE__, __LINE__);
             }
         } while (true);

src/xrpld/app/ledger/InboundLedger.h

@@ -9,7 +9,6 @@
 
 #include <mutex>
 #include <set>
-#include <string_view>
 #include <utility>
 
 namespace xrpl {
@@ -132,19 +131,16 @@ private:
     processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData& data);
 
     bool
-    takeHeader(std::string_view data);
+    takeHeader(std::string const& data);
 
     void
-    receiveNode(
-        std::shared_ptr<Peer> const& peer,
-        protocol::TMLedgerData& packet,
-        SHAMapAddNode& san);
+    receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode&);
 
     bool
-    takeTxRootNode(std::string_view data, SHAMapAddNode& san);
+    takeTxRootNode(Slice const& data, SHAMapAddNode&);
 
     bool
-    takeAsRootNode(std::string_view data, SHAMapAddNode& san);
+    takeAsRootNode(Slice const& data, SHAMapAddNode&);
 
     std::vector<uint256>
     neededTxHashes(int max, SHAMapSyncFilter* filter) const;

src/xrpld/app/ledger/LedgerNodeHelpers.h

@@ -1,53 +0,0 @@
-#pragma once
-
-#include <xrpl/basics/IntrusivePointer.h>
-#include <xrpl/shamap/SHAMapNodeID.h>
-#include <xrpl/shamap/SHAMapTreeNode.h>
-
-#include <optional>
-#include <string_view>
-
-namespace protocol {
-class TMLedgerNode;
-}  // namespace protocol
-
-namespace xrpl {
-
-/**
- * @brief Deserializes a SHAMapTreeNode from wire format data.
- *
- * This function attempts to create a SHAMapTreeNode from the provided data string. If the data is
- * malformed or deserialization fails, the function returns a nullptr instead of throwing an
- * exception.
- *
- * @param data The serialized node data in wire format.
- * @return The deserialized tree node if successful, or a nullptr if deserialization fails.
- */
-[[nodiscard]] SHAMapTreeNodePtr
-getTreeNode(std::string_view data);
-
-/**
- * @brief Extracts or reconstructs the SHAMapNodeID from a ledger node proto message.
- *
- * This function retrieves the SHAMapNodeID for a tree node, with behavior that depends on which
- * field is set and the node type (inner vs. leaf).
- *
- * When the legacy `nodeid` field is set in the message:
- * - For all nodes: Deserializes the node ID from the field.
- * - For leaf nodes: Validates that the node ID is consistent with the leaf's key.
- *
- * When the new `id` or `depth` field is set in the message:
- * - For inner nodes: Deserializes the node ID from the `id` field.
- * - For leaf nodes: Reconstructs the node ID using both the depth from the `depth` field and the
- *   key from the leaf node's item.
- * Note that root nodes may be inner nodes or leaf nodes.
- *
- * @param ledgerNode The validated protocol message containing the ledger node data.
- * @param treeNode The deserialized tree node (inner or leaf node).
- * @return An optional containing the node ID if extraction/reconstruction succeeds, or std::nullopt
- *         if the required fields are missing or validation fails.
- */
-[[nodiscard]] std::optional<SHAMapNodeID>
-getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& treeNode);
-
-}  // namespace xrpl

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

@@ -3,7 +3,6 @@
 #include <xrpld/app/ledger/AccountStateSF.h>
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
-#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/TransactionStateSF.h>
 #include <xrpld/app/ledger/detail/TimeoutCounter.h>
 #include <xrpld/app/main/Application.h>
@@ -45,8 +44,8 @@
 #include <mutex>
 #include <random>
 #include <sstream>
+#include <stdexcept>
 #include <string>
-#include <string_view>
 #include <tuple>
 #include <unordered_map>
 #include <utility>
@@ -776,7 +775,7 @@ InboundLedger::filterNodes(
 */
 // data must not have hash prefix
 bool
-InboundLedger::takeHeader(std::string_view data)
+InboundLedger::takeHeader(std::string const& data)
 {
     // Return value: true=normal, false=bad data
     JLOG(journal_.trace()) << "got header acquiring ledger " << hash_;
@@ -821,10 +820,7 @@ InboundLedger::takeHeader(std::string_view data)
     Call with a lock
 */
 void
-InboundLedger::receiveNode(
-    std::shared_ptr<Peer> const& peer,
-    protocol::TMLedgerData& packet,
-    SHAMapAddNode& san)
+InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san)
 {
     if (!haveHeader_)
     {
@@ -867,47 +863,32 @@ InboundLedger::receiveNode(
     {
         auto const f = filter.get();
 
-        for (auto const& ledgerNode : packet.nodes())
+        for (auto const& node : packet.nodes())
         {
-            auto treeNode = getTreeNode(ledgerNode.nodedata());
-            if (!treeNode)
-            {
-                JLOG(journal_.warn()) << "Got invalid node data";
-                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_data invalid");
-                san.incInvalid();
-                return;
-            }
+            auto const nodeID = deserializeSHAMapNodeID(node.nodeid());
 
-            auto const nodeID = getSHAMapNodeID(ledgerNode, *treeNode);
             if (!nodeID)
-            {
-                JLOG(journal_.warn()) << "Got invalid node id";
-                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_id invalid");
-                san.incInvalid();
-                return;
-            }
+                throw std::runtime_error("data does not properly deserialize");
 
             if (nodeID->isRoot())
             {
-                san += map.addRootNode(rootHash, std::move(treeNode), f);
+                san += map.addRootNode(rootHash, makeSlice(node.nodedata()), f);
             }
             else
             {
-                san += map.addKnownNode(*nodeID, std::move(treeNode), f);
+                san += map.addKnownNode(*nodeID, makeSlice(node.nodedata()), f);
             }
 
             if (!san.isGood())
             {
-                JLOG(journal_.warn()) << "Got invalid node";
-                peer->charge(Resource::kFeeInvalidData, "ledger_node invalid");
+                JLOG(journal_.warn()) << "Received bad node data";
                 return;
             }
         }
     }
     catch (std::exception const& e)
     {
-        // If we get here it is not necessarily because the node was bad, so don't charge the peer.
-        JLOG(journal_.error()) << "Could not process node: " << e.what();
+        JLOG(journal_.error()) << "Received bad node data: " << e.what();
         san.incInvalid();
         return;
     }
@@ -935,7 +916,7 @@ InboundLedger::receiveNode(
     Call with a lock
 */
 bool
-InboundLedger::takeAsRootNode(std::string_view data, SHAMapAddNode& san)
+InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san)
 {
     if (failed_ || haveState_)
     {
@@ -951,17 +932,9 @@ InboundLedger::takeAsRootNode(std::string_view data, SHAMapAddNode& san)
         // LCOV_EXCL_STOP
     }
 
-    auto treeNode = getTreeNode(data);
-    if (!treeNode)
-    {
-        JLOG(journal_.warn()) << "Got invalid node data";
-        san.incInvalid();
-        return false;
-    }
-
     AccountStateSF filter(ledger_->stateMap().family().db(), app_.getLedgerMaster());
-    san += ledger_->stateMap().addRootNode(
-        SHAMapHash{ledger_->header().accountHash}, std::move(treeNode), &filter);
+    san +=
+        ledger_->stateMap().addRootNode(SHAMapHash{ledger_->header().accountHash}, data, &filter);
     return san.isGood();
 }
 
@@ -969,7 +942,7 @@ InboundLedger::takeAsRootNode(std::string_view data, SHAMapAddNode& san)
     Call with a lock
 */
 bool
-InboundLedger::takeTxRootNode(std::string_view data, SHAMapAddNode& san)
+InboundLedger::takeTxRootNode(Slice const& data, SHAMapAddNode& san)
 {
     if (failed_ || haveTransactions_)
     {
@@ -985,17 +958,8 @@ InboundLedger::takeTxRootNode(std::string_view data, SHAMapAddNode& san)
         // LCOV_EXCL_STOP
     }
 
-    auto treeNode = getTreeNode(data);
-    if (!treeNode)
-    {
-        JLOG(journal_.warn()) << "Got invalid node data";
-        san.incInvalid();
-        return false;
-    }
-
     TransactionStateSF filter(ledger_->txMap().family().db(), app_.getLedgerMaster());
-    san += ledger_->txMap().addRootNode(
-        SHAMapHash{ledger_->header().txHash}, std::move(treeNode), &filter);
+    san += ledger_->txMap().addRootNode(SHAMapHash{ledger_->header().txHash}, data, &filter);
     return san.isGood();
 }
 
@@ -1092,25 +1056,15 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData& p
             }
 
             if (!haveState_ && (packet.nodes().size() > 1) &&
-                !takeAsRootNode(packet.nodes(1).nodedata(), san))
+                !takeAsRootNode(makeSlice(packet.nodes(1).nodedata()), san))
             {
                 JLOG(journal_.warn()) << "Included AS root invalid";
-                if (san.isInvalid())
-                {
-                    peer->charge(Resource::kFeeInvalidData, "ledger_data invalid AS root");
-                    return -1;
-                }
             }
 
             if (!haveTransactions_ && (packet.nodes().size() > 2) &&
-                !takeTxRootNode(packet.nodes(2).nodedata(), san))
+                !takeTxRootNode(makeSlice(packet.nodes(2).nodedata()), san))
             {
                 JLOG(journal_.warn()) << "Included TX root invalid";
-                if (san.isInvalid())
-                {
-                    peer->charge(Resource::kFeeInvalidData, "ledger_data invalid TX root");
-                    return -1;
-                }
             }
         }
         catch (std::exception const& ex)
@@ -1139,18 +1093,24 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData& p
 
         ScopedLockType const sl(mtx_);
 
+        // Verify node IDs and data are complete
+        for (auto const& node : packet.nodes())
+        {
+            if (!node.has_nodeid() || !node.has_nodedata())
+            {
+                JLOG(journal_.warn()) << "Got bad node";
+                peer->charge(Resource::kFeeMalformedRequest, "ledger_data bad node");
+                return -1;
+            }
+        }
+
         SHAMapAddNode san;
-        receiveNode(peer, packet, san);
+        receiveNode(packet, san);
 
         JLOG(journal_.debug()) << "Ledger "
                                << ((packet.type() == protocol::liTX_NODE) ? "TX" : "AS")
                                << " node stats: " << san.get();
 
-        // Note: Peer charges for invalid/malformed data are issued from within receiveNode at the
-        // exact failure site, so the peer is only charged for problems they are responsible for.
-        if (san.isInvalid())
-            return -1;
-
         if (san.isUseful())
             progress_ = true;
 

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

@@ -2,13 +2,13 @@
 
 #include <xrpld/app/ledger/InboundLedger.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
-#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Blob.h>
 #include <xrpl/basics/DecayingSample.h>
 #include <xrpl/basics/Log.h>
+#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/UnorderedContainers.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/scope.h>
@@ -248,17 +248,23 @@ public:
         Serializer s;
         try
         {
-            for (auto const& ledgerNode : packetPtr->nodes())
+            for (int i = 0; i < packetPtr->nodes().size(); ++i)
             {
-                auto const treeNode = getTreeNode(ledgerNode.nodedata());
-                if (!treeNode)
+                auto const& node = packetPtr->nodes(i);
+
+                if (!node.has_nodeid() || !node.has_nodedata())
+                    return;
+
+                auto newNode = SHAMapTreeNode::makeFromWire(makeSlice(node.nodedata()));
+
+                if (!newNode)
                     return;
 
                 s.erase();
-                treeNode->serializeWithPrefix(s);
+                newNode->serializeWithPrefix(s);
 
                 app_.getLedgerMaster().addFetchPack(
-                    treeNode->getHash().asUInt256(), std::make_shared<Blob>(s.begin(), s.end()));
+                    newNode->getHash().asUInt256(), std::make_shared<Blob>(s.begin(), s.end()));
             }
         }
         catch (std::exception const&)  // NOLINT(bugprone-empty-catch)

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

@@ -1,11 +1,11 @@
 #include <xrpld/app/ledger/InboundTransactions.h>
 
-#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/detail/TransactionAcquire.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Log.h>
+#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/UnorderedContainers.h>
 #include <xrpl/beast/insight/Collector.h>
 #include <xrpl/protocol/RippleLedgerHash.h>
@@ -14,7 +14,6 @@
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
-#include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <xrpl.pb.h>
 
@@ -137,43 +136,34 @@ public:
 
         if (ta == nullptr)
         {
-            peer->charge(Resource::kFeeUselessData, "ledger_data useless");
+            peer->charge(Resource::kFeeUselessData, "ledger_data");
             return;
         }
 
-        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data;
+        std::vector<std::pair<SHAMapNodeID, Slice>> data;
         data.reserve(packet.nodes().size());
 
-        for (auto const& ledgerNode : packet.nodes())
+        for (auto const& node : packet.nodes())
         {
-            auto treeNode = getTreeNode(ledgerNode.nodedata());
-            if (!treeNode)
+            if (!node.has_nodeid() || !node.has_nodedata())
             {
-                JLOG(j_.warn()) << "Got invalid node data";
-                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_data invalid");
+                peer->charge(Resource::kFeeMalformedRequest, "ledger_data");
                 return;
             }
 
-            auto const nodeID = getSHAMapNodeID(ledgerNode, *treeNode);
-            if (!nodeID)
+            auto const id = deserializeSHAMapNodeID(node.nodeid());
+
+            if (!id)
             {
-                JLOG(j_.warn()) << "Got invalid node id";
-                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_id invalid");
+                peer->charge(Resource::kFeeInvalidData, "ledger_data");
                 return;
             }
 
-            data.emplace_back(*nodeID, std::move(treeNode));
+            data.emplace_back(*id, makeSlice(node.nodedata()));
         }
 
-        auto const san = ta->takeNodes(std::move(data), peer);
-        if (san.isInvalid())
-        {
-            peer->charge(Resource::kFeeInvalidData, "ledger_data invalid");
-        }
-        else if (!san.isUseful())
-        {
-            peer->charge(Resource::kFeeUselessData, "ledger_data useless");
-        }
+        if (!ta->takeNodes(data, peer).isUseful())
+            peer->charge(Resource::kFeeUselessData, "ledger_data not useful");
     }
 
     void

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

@@ -1,81 +0,0 @@
-#include <xrpld/app/ledger/LedgerNodeHelpers.h>
-
-#include <xrpl/basics/Slice.h>
-#include <xrpl/basics/safe_cast.h>
-#include <xrpl/beast/utility/instrumentation.h>
-#include <xrpl/shamap/SHAMap.h>
-#include <xrpl/shamap/SHAMapLeafNode.h>
-#include <xrpl/shamap/SHAMapNodeID.h>
-#include <xrpl/shamap/SHAMapTreeNode.h>
-
-#include <xrpl.pb.h>
-
-#include <exception>
-#include <optional>
-#include <string_view>
-
-namespace xrpl {
-
-SHAMapTreeNodePtr
-getTreeNode(std::string_view data)
-{
-    auto const slice = makeSlice(data);
-    try
-    {
-        return SHAMapTreeNode::makeFromWire(slice);
-    }
-    catch (std::exception const&)
-    {
-        return {};
-    }
-}
-
-std::optional<SHAMapNodeID>
-getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& treeNode)
-{
-    if (ledgerNode.has_id() || ledgerNode.has_depth())
-    {
-        // Reject ambiguous messages that mix the legacy and new reference fields.
-        if (ledgerNode.has_nodeid())
-            return std::nullopt;
-
-        if (treeNode.isInner())
-        {
-            if (!ledgerNode.has_id())
-                return std::nullopt;
-
-            return deserializeSHAMapNodeID(ledgerNode.id());
-        }
-
-        if (treeNode.isLeaf())
-        {
-            if (!ledgerNode.has_depth() || ledgerNode.depth() > SHAMap::kLeafDepth)
-                return std::nullopt;
-
-            auto const key = safeDowncast<SHAMapLeafNode const*>(&treeNode)->peekItem()->key();
-            return SHAMapNodeID::createID(ledgerNode.depth(), key);
-        }
-
-        UNREACHABLE("xrpl::getSHAMapNodeID : tree node is neither inner nor leaf");
-        return std::nullopt;
-    }
-
-    if (!ledgerNode.has_nodeid())
-        return std::nullopt;
-
-    auto nodeID = deserializeSHAMapNodeID(ledgerNode.nodeid());
-    if (!nodeID.has_value())
-        return std::nullopt;
-
-    if (treeNode.isLeaf())
-    {
-        auto const key = safeDowncast<SHAMapLeafNode const*>(&treeNode)->peekItem()->key();
-        auto const expectedID = SHAMapNodeID::createID(static_cast<int>(nodeID->getDepth()), key);
-        if (nodeID->getNodeID() != expectedID.getNodeID())
-            return std::nullopt;
-    }
-
-    return nodeID;
-}
-
-}  // namespace xrpl

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

@@ -7,13 +7,13 @@
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Log.h>
+#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/core/Job.h>
 #include <xrpl/server/NetworkOPs.h>
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapAddNode.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
-#include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <xrpl.pb.h>
 
@@ -171,7 +171,7 @@ TransactionAcquire::trigger(std::shared_ptr<Peer> const& peer)
 
 SHAMapAddNode
 TransactionAcquire::takeNodes(
-    std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
+    std::vector<std::pair<SHAMapNodeID, Slice>> const& data,
     std::shared_ptr<Peer> const& peer)
 {
     ScopedLockType const sl(mtx_);
@@ -195,7 +195,7 @@ TransactionAcquire::takeNodes(
 
         ConsensusTransSetSF sf(app_, app_.getTempNodeCache());
 
-        for (auto& d : data)
+        for (auto const& d : data)
         {
             if (d.first.isRoot())
             {
@@ -203,8 +203,7 @@ TransactionAcquire::takeNodes(
                 {
                     JLOG(journal_.debug()) << "Got root TXS node, already have it";
                 }
-                else if (!map_->addRootNode(SHAMapHash{hash_}, std::move(d.second), nullptr)
-                              .isGood())
+                else if (!map_->addRootNode(SHAMapHash{hash_}, d.second, nullptr).isGood())
                 {
                     JLOG(journal_.warn()) << "TX acquire got bad root node";
                 }
@@ -213,7 +212,7 @@ TransactionAcquire::takeNodes(
                     haveRoot_ = true;
                 }
             }
-            else if (!map_->addKnownNode(d.first, std::move(d.second), &sf).isGood())
+            else if (!map_->addKnownNode(d.first, d.second, &sf).isGood())
             {
                 JLOG(journal_.warn()) << "TX acquire got bad non-root node";
                 return SHAMapAddNode::invalid();

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

@@ -21,8 +21,8 @@ public:
 
     SHAMapAddNode
     takeNodes(
-        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
-        std::shared_ptr<Peer> const& peer);
+        std::vector<std::pair<SHAMapNodeID, Slice>> const& data,
+        std::shared_ptr<Peer> const&);
 
     void
     init(int startPeers);

src/xrpld/overlay/Peer.h

@@ -17,7 +17,6 @@ enum class ProtocolFeature {
     ValidatorListPropagation,
     ValidatorList2Propagation,
     LedgerReplay,
-    LedgerNodeDepth,
 };
 
 /** Represents a peer connection in the overlay. */

src/xrpld/overlay/detail/PeerImp.cpp

@@ -5,7 +5,6 @@
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/InboundTransactions.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
-#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/TransactionMaster.h>
 #include <xrpld/app/misc/Transaction.h>
 #include <xrpld/app/misc/ValidatorList.h>
@@ -62,7 +61,6 @@
 #include <xrpl/server/Handoff.h>
 #include <xrpl/server/LoadFeeTrack.h>
 #include <xrpl/server/NetworkOPs.h>
-#include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
 #include <xrpl/tx/apply.h>
 
@@ -551,8 +549,6 @@ PeerImp::supportsFeature(ProtocolFeature f) const
             return protocol_ >= makeProtocol(2, 1);
         case ProtocolFeature::ValidatorList2Propagation:
             return protocol_ >= makeProtocol(2, 2);
-        case ProtocolFeature::LedgerNodeDepth:
-            return protocol_ >= makeProtocol(2, 3);
         case ProtocolFeature::LedgerReplay:
             return ledgerReplayEnabled_;
     }
@@ -1497,12 +1493,23 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
         }
     }
 
-    // Cheap structural checks on node IDs; full parsing is deferred to the job
-    // so the IO thread is not burdened with SHAMapNodeID deserialization.
-    if (itype != protocol::liBASE && m->nodeids_size() <= 0)
+    // Verify ledger node IDs
+    if (itype != protocol::liBASE)
     {
-        badData("Invalid ledger node IDs");
-        return;
+        if (m->nodeids_size() <= 0)
+        {
+            badData("Invalid ledger node IDs");
+            return;
+        }
+
+        for (auto const& nodeId : m->nodeids())
+        {
+            if (deserializeSHAMapNodeID(nodeId) == std::nullopt)
+            {
+                badData("Invalid SHAMap node ID");
+                return;
+            }
+        }
     }
 
     // Verify query type
@@ -1522,32 +1529,11 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
         }
     }
 
-    // Queue a job to process the request. Full parsing of the node IDs is
-    // performed inside the job so the IO thread is not burdened with
-    // SHAMapNodeID deserialization for every TMGetLedger.
+    // Queue a job to process the request
     std::weak_ptr<PeerImp> const weak = shared_from_this();
-    app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m, itype]() {
-        auto peer = weak.lock();
-        if (!peer)
-            return;
-
-        std::vector<SHAMapNodeID> nodeIDs;
-        if (itype != protocol::liBASE)
-        {
-            nodeIDs.reserve(m->nodeids_size());
-            for (auto const& nodeId : m->nodeids())
-            {
-                auto parsed = deserializeSHAMapNodeID(nodeId);
-                if (!parsed)
-                {
-                    peer->charge(Resource::kFeeInvalidData, "get_ledger invalid node ID");
-                    return;
-                }
-                nodeIDs.push_back(std::move(*parsed));
-            }
-        }
-
-        peer->processLedgerRequest(m, std::move(nodeIDs));
+    app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m]() {
+        if (auto peer = weak.lock())
+            peer->processLedgerRequest(m);
     });
 }
 
@@ -1712,44 +1698,12 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMLedgerData> const& m)
         return;
     }
 
-    // If there is a request cookie, attempt to relay the message.
+    // If there is a request cookie, attempt to relay the message
     if (m->has_requestcookie())
     {
         if (auto peer = overlay_.findPeerByShortID(m->requestcookie()))
         {
             m->clear_requestcookie();
-
-            // If the original requester doesn't support the new depth-based format, rewrite any
-            // nodes that use it back to the legacy nodeid format before relaying. Once all nodes
-            // have upgraded, the old protocol version and this code can be removed.
-            if (!peer->supportsFeature(ProtocolFeature::LedgerNodeDepth))
-            {
-                for (int i = 0; i < m->nodes_size(); ++i)
-                {
-                    auto* ledgerNode = m->mutable_nodes(i);
-                    if (ledgerNode->reference_case() != ledgerNode->REFERENCE_NOT_SET)
-                    {
-                        auto treeNode = getTreeNode(ledgerNode->nodedata());
-                        if (!treeNode)
-                        {
-                            JLOG(pJournal_.warn()) << "Unable to get tree node";
-                            return;
-                        }
-
-                        auto const nodeID = getSHAMapNodeID(*ledgerNode, *treeNode);
-                        if (!nodeID)
-                        {
-                            JLOG(pJournal_.warn()) << "Unable to get node ID";
-                            return;
-                        }
-
-                        ledgerNode->set_nodeid(nodeID->getRawString());
-                        ledgerNode->clear_id();
-                        ledgerNode->clear_depth();
-                    }
-                }
-            }
-
             peer->send(std::make_shared<Message>(*m, protocol::mtLEDGER_DATA));
         }
         else
@@ -3288,9 +3242,7 @@ PeerImp::getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const
 }
 
 void
-PeerImp::processLedgerRequest(
-    std::shared_ptr<protocol::TMGetLedger> const& m,
-    std::vector<SHAMapNodeID> nodeIDs)
+PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
 {
     // Do not resource charge a peer responding to a relay
     if (!m->has_requestcookie())
@@ -3375,25 +3327,26 @@ PeerImp::processLedgerRequest(
     }
 
     // Add requested node data to reply
-    if (!nodeIDs.empty())
+    if (m->nodeids_size() > 0)
     {
         std::uint32_t const defaultDepth = isHighLatency() ? 2 : 1;
         auto const queryDepth{m->has_querydepth() ? m->querydepth() : defaultDepth};
 
-        std::vector<SHAMapNodeData> data;
-        data.reserve(Tuning::kSoftMaxReplyNodes);
-        auto const useLedgerNodeDepth = supportsFeature(ProtocolFeature::LedgerNodeDepth);
+        std::vector<std::pair<SHAMapNodeID, Blob>> data;
 
-        for (auto const& nodeID : nodeIDs)
+        for (int i = 0;
+             i < m->nodeids_size() && ledgerData.nodes_size() < Tuning::kSoftMaxReplyNodes;
+             ++i)
         {
-            if (ledgerData.nodes_size() >= Tuning::kSoftMaxReplyNodes)
-                break;
+            auto const shaMapNodeId{deserializeSHAMapNodeID(m->nodeids(i))};
 
             data.clear();
+            data.reserve(Tuning::kSoftMaxReplyNodes);
 
             try
             {
-                if (map->getNodeFat(nodeID, data, fatLeaves, queryDepth))
+                // NOLINTNEXTLINE(bugprone-unchecked-optional-access) nodeids checked in onGetLedger
+                if (map->getNodeFat(*shaMapNodeId, data, fatLeaves, queryDepth))
                 {
                     JLOG(pJournal_.trace())
                         << "processLedgerRequest: getNodeFat got " << data.size() << " nodes";
@@ -3402,26 +3355,9 @@ PeerImp::processLedgerRequest(
                     {
                         if (ledgerData.nodes_size() >= Tuning::kHardMaxReplyNodes)
                             break;
-
                         protocol::TMLedgerNode* node{ledgerData.add_nodes()};
-
-                        node->set_nodedata(d.data.data(), d.data.size());
-
-                        // When the LedgerNodeDepth protocol feature is not supported by the peer,
-                        // we always set the `nodeid` field. However, when it is supported then we
-                        // set the `id` field for inner nodes and the `depth` field for leaf nodes.
-                        if (!useLedgerNodeDepth)
-                        {
-                            node->set_nodeid(d.nodeID.getRawString());
-                        }
-                        else if (d.isLeaf)
-                        {
-                            node->set_depth(d.nodeID.getDepth());
-                        }
-                        else
-                        {
-                            node->set_id(d.nodeID.getRawString());
-                        }
+                        node->set_nodeid(d.first.getRawString());
+                        node->set_nodedata(d.second.data(), d.second.size());
                     }
                 }
                 else
@@ -3460,7 +3396,7 @@ PeerImp::processLedgerRequest(
                     info += ", no hash specified";
 
                 JLOG(pJournal_.warn())
-                    << "processLedgerRequest: getNodeFat with nodeId " << nodeID
+                    << "processLedgerRequest: getNodeFat with nodeId " << *shaMapNodeId
                     << " and ledger info type " << info << " throws exception: " << e.what();
             }
         }

src/xrpld/overlay/detail/PeerImp.h

@@ -14,7 +14,6 @@
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/STValidation.h>
 #include <xrpl/resource/Fees.h>
-#include <xrpl/shamap/SHAMapNodeID.h>
 
 #include <boost/circular_buffer.hpp>
 #include <boost/endian/conversion.hpp>
@@ -624,9 +623,7 @@ private:
     getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const;
 
     void
-    processLedgerRequest(
-        std::shared_ptr<protocol::TMGetLedger> const& m,
-        std::vector<SHAMapNodeID> nodeIDs);
+    processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m);
 };
 
 //------------------------------------------------------------------------------

src/xrpld/overlay/detail/ProtocolVersion.cpp

@@ -28,7 +28,6 @@ namespace xrpl {
 constexpr ProtocolVersion const kSupportedProtocolList[]{
     {2, 1},
     {2, 2},
-    {2, 3},
 };
 
 // This ugly construct ensures that supportedProtocolList is sorted in strictly