From 7612fd0ba6daae63f4e406d429fef44fc26acb82 Mon Sep 17 00:00:00 2001
From: Ed Hennis <ed@ripple.com>
Date: Fri, 26 Sep 2025 19:23:13 -0400
Subject: [PATCH] Review feedback from @gregtatcam

- Amendment checking correctness.
- Sync some changes between LoanBrokerCoverWithdraw and CoverDeposit.
- Check for nulls.
- Add documentation.
---
 src/xrpld/app/tx/detail/InvariantCheck.cpp    | 13 ++++----
 .../app/tx/detail/LoanBrokerCoverDeposit.cpp  |  6 +++-
 .../app/tx/detail/LoanBrokerCoverWithdraw.cpp |  2 ++
 src/xrpld/overlay/detail/PeerImp.cpp          | 32 +++++++++++++++++++
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp
index 9b08887b76..20a647821f 100644
--- a/src/xrpld/app/tx/detail/InvariantCheck.cpp
+++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp
@@ -1522,15 +1522,14 @@ ValidMPTIssuance::finalize(
             return mptIssuancesCreated_ == 0 && mptIssuancesDeleted_ == 1;
         }
 
+        bool const lendingProtocolEnabled =
+            view.rules().enabled(featureLendingProtocol);
         // ttESCROW_FINISH may authorize an MPT, but it can't have the
         // mayAuthorizeMPT privilege, because that may cause
         // non-amendment-gated side effects.
         bool const enforceEscrowFinish = (tx.getTxnType() == ttESCROW_FINISH) &&
-            (view.rules().enabled(featureSingleAssetVault)
-             /*
-               TODO: Uncomment when LendingProtocol is defined
-               || view.rules().enabled(featureLendingProtocol)*/
-            );
+            (view.rules().enabled(featureSingleAssetVault) ||
+             lendingProtocolEnabled);
         if (hasPrivilege(tx, mustAuthorizeMPT | mayAuthorizeMPT) ||
             enforceEscrowFinish)
         {
@@ -1548,7 +1547,9 @@ ValidMPTIssuance::finalize(
                                    "succeeded but deleted issuances";
                 return false;
             }
-            else if (mptokensCreated_ + mptokensDeleted_ > 1)
+            else if (
+                lendingProtocolEnabled &&
+                mptokensCreated_ + mptokensDeleted_ > 1)
             {
                 JLOG(j.fatal()) << "Invariant failed: MPT authorize succeeded "
                                    "but created/deleted bad number mptokens";
diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp
index 5aa3aba2cd..c75dc95f98 100644
--- a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp
+++ b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp
@@ -35,7 +35,11 @@ LoanBrokerCoverDeposit::preflight(PreflightContext const& ctx)
     if (ctx.tx[sfLoanBrokerID] == beast::zero)
         return temINVALID;
 
-    if (ctx.tx[sfAmount] <= beast::zero)
+    auto const dstAmount = ctx.tx[sfAmount];
+    if (dstAmount <= beast::zero)
+        return temBAD_AMOUNT;
+
+    if (!isLegalNet(dstAmount))
         return temBAD_AMOUNT;
 
     return tesSUCCESS;
diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp
index cb47c1953f..0505218dcb 100644
--- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp
+++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp
@@ -91,6 +91,8 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
         return tecNO_PERMISSION;
     }
     auto const vault = ctx.view.read(keylet::vault(sleBroker->at(sfVaultID)));
+    if (!vault)
+        return tefBAD_LEDGER;  // LCOV_EXCL_LINE
     auto const vaultAsset = vault->at(sfAsset);
 
     if (amount.asset() != vaultAsset)
diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp
index c09d0be6a9..127ab4655e 100644
--- a/src/xrpld/overlay/detail/PeerImp.cpp
+++ b/src/xrpld/overlay/detail/PeerImp.cpp
@@ -1387,6 +1387,22 @@ PeerImp::handleTransaction(
 
         // Charge strongly for attempting to relay a txn with tfInnerBatchTxn
         // LCOV_EXCL_START
+        /*
+           There is no need to check whether the featureBatch amendment is
+           enabled.
+
+           * If the `tfInnerBatchTxn` flag is set, and the amendment is
+           enabled, then it's an invalid transaction because inner batch
+           transactions should not be relayed.
+           * If the `tfInnerBatchTxn` flag is set, and the amendment is *not*
+           enabled, then the transaction is malformed because it's using an
+           "unknown" flag. There's no need to waste the resources to send it
+           to the transaction engine.
+
+           We don't normally check transaction validity at this level, but
+           since we _need_ to check it when the amendment is enabled, we may as
+           well drop it if the flag is set regardless.
+        */
         if (stx->isFlag(tfInnerBatchTxn))
         {
             JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing "
@@ -2951,6 +2967,22 @@ PeerImp::checkTransaction(
     {
         // charge strongly for relaying batch txns
         // LCOV_EXCL_START
+        /*
+           There is no need to check whether the featureBatch amendment is
+           enabled.
+
+           * If the `tfInnerBatchTxn` flag is set, and the amendment is
+           enabled, then it's an invalid transaction because inner batch
+           transactions should not be relayed.
+           * If the `tfInnerBatchTxn` flag is set, and the amendment is *not*
+           enabled, then the transaction is malformed because it's using an
+           "unknown" flag. There's no need to waste the resources to send it
+           to the transaction engine.
+
+           We don't normally check transaction validity at this level, but
+           since we _need_ to check it when the amendment is enabled, we may as
+           well drop it if the flag is set regardless.
+        */
         if (stx->isFlag(tfInnerBatchTxn))
         {
             JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing "