refactor: assert messages, preflight2 defense-in-depth, signer-check order

include/xrpl/core/HashRouter.h

@@ -19,14 +19,17 @@ enum class HashRouterFlags : std::uint16_t {
     HELD = 0x08,     // Held by LedgerMaster after potential processing failure
     TRUSTED = 0x10,  // Comes from a trusted source
 
-    // Private flags (used internally in apply.cpp)
-    // Do not attempt to read, set, or reuse.
+    // Private flags. Each group is owned by one file; do not read, set, or
+    // reuse a flag outside the file noted.
+    // Used in apply.cpp
     PRIVATE1 = 0x0100,
     PRIVATE2 = 0x0200,
     PRIVATE3 = 0x0400,
     PRIVATE4 = 0x0800,
+    // Used in EscrowFinish.cpp
     PRIVATE5 = 0x1000,
     PRIVATE6 = 0x2000,
+    // Used in Batch.cpp
     PRIVATE7 = 0x4000
 };
 

src/libxrpl/protocol/STTx.cpp

@@ -436,7 +436,7 @@ STTx::checkSingleSign(STObject const& sigObject) const
 std::expected<void, std::string>
 STTx::checkBatchSingleSign(STObject const& batchSigner) const
 {
-    XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchSingleSign : not a batch transaction");
+    XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchSingleSign : batch transaction");
     Serializer msg;
     serializeBatch(
         msg, getAccountID(sfAccount), getSeqValue(), getFlags(), getBatchTransactionIDs());
@@ -524,7 +524,7 @@ multiSignHelper(
 std::expected<void, std::string>
 STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const
 {
-    XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchMultiSign : not a batch transaction");
+    XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchMultiSign : batch transaction");
     // We can ease the computational load inside the loop a bit by
     // pre-constructing part of the data that we hash.  Fill a Serializer
     // with the stuff that stays constant from signature to signature.
@@ -576,7 +576,7 @@ STTx::buildBatchTxnIds()
     // (including sfRawTransactions) are canonical before the inner txns are
     // hashed. The constructors call this immediately after applying the
     // template; isFree() being false confirms a template is set.
-    XRPL_ASSERT(!isFree(), "STTx::buildBatchTxnIds : template not applied");
+    XRPL_ASSERT(!isFree(), "STTx::buildBatchTxnIds : template applied");
     if (getTxnType() != ttBATCH || !isFieldPresent(sfRawTransactions))
         return;
 
@@ -593,7 +593,7 @@ STTx::buildBatchTxnIds()
 std::vector<uint256> const&
 STTx::getBatchTransactionIDs() const
 {
-    XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : not a batch transaction");
+    XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : batch transaction");
     XRPL_ASSERT(
         !batchTxnIds_.empty(), "STTx::getBatchTransactionIDs : batch transaction IDs not built");
     XRPL_ASSERT(
@@ -744,7 +744,7 @@ isBatchRawTransactionOkay(STObject const& st, std::string& reason)
     // sfRawTransactions only appears on a Batch.
     XRPL_ASSERT(
         safeCast<TxType>(st.getFieldU16(sfTransactionType)) == ttBATCH,
-        "xrpl::isBatchRawTransactionOkay : not a batch transaction");
+        "xrpl::isBatchRawTransactionOkay : batch transaction");
 
     if (st.isFieldPresent(sfBatchSigners) &&
         st.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners)

src/libxrpl/tx/Transactor.cpp

@@ -244,10 +244,16 @@ Transactor::preflight2(PreflightContext const& ctx)
         return *ret;
     }
 
-    // Skip signature check on batch inner transactions. The inner-batch flag
-    // and parentBatchId checks are in preflight1.
+    // Skip signature check on batch inner transactions. These flag and
+    // parentBatchId checks duplicate preflight1 as defense in depth.
     if (ctx.tx.isFlag(tfInnerBatchTxn))
+    {
+        if (!ctx.rules.enabled(featureBatchV1_1))
+            return temINVALID_FLAG;
+        if (!ctx.parentBatchId.has_value())
+            return temINVALID_INNER_BATCH;
         return tesSUCCESS;
+    }
     // Do not add any checks after this point that are relevant for
     // batch inner transactions. They will be skipped.
 

src/libxrpl/tx/transactors/system/Batch.cpp

@@ -415,8 +415,7 @@ NotTEC
 Batch::preflightSigValidated(PreflightContext const& ctx)
 {
     XRPL_ASSERT(
-        ctx.tx.getTxnType() == ttBATCH,
-        "xrpl::Batch::preflightSigValidated : not a batch transaction");
+        ctx.tx.getTxnType() == ttBATCH, "xrpl::Batch::preflightSigValidated : batch transaction");
     auto const parentBatchId = ctx.tx.getTransactionID();
     auto const outerAccount = ctx.tx.getAccountID(sfAccount);
     auto const& rawTxns = ctx.tx.getFieldArray(sfRawTransactions);
@@ -491,7 +490,21 @@ Batch::preflightSigValidated(PreflightContext const& ctx)
             }
             ++numReqSignersMatched;
         }
+    }
 
+    // Every required signer must be matched. This is cheaper than the signature
+    // check below, so verify it first. Also covers sfBatchSigners being absent
+    // while inner txns require signers (numReqSignersMatched stays 0).
+    if (numReqSignersMatched != requiredSigners.size())
+    {
+        JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: "
+                            << "invalid batch signers.";
+        return temBAD_SIGNER;
+    }
+
+    // Check the batch signer signatures (only present when there are signers).
+    if (ctx.tx.isFieldPresent(sfBatchSigners))
+    {
         // Caches only the cryptographic signature check, not whether each
         // signer is authorized to sign for its account - that ledger-dependent
         // check is Batch::checkBatchSign in preclaim and is never cached.
@@ -508,14 +521,6 @@ Batch::preflightSigValidated(PreflightContext const& ctx)
         }
     }
 
-    // Reject if any required signer was not matched (also covers signers being
-    // required while sfBatchSigners is absent, leaving numReqSignersMatched at 0).
-    if (numReqSignersMatched != requiredSigners.size())
-    {
-        JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: "
-                            << "invalid batch signers.";
-        return temBAD_SIGNER;
-    }
     return tesSUCCESS;
 }
 

src/xrpld/app/misc/detail/TxQ.cpp

@@ -390,9 +390,9 @@ TxQ::canBeHeld(
     std::optional<TxQAccount::TxMap::iterator> const& replacementIter,
     std::scoped_lock<std::mutex> const& lock)
 {
-    // A Batch is never queued: it can advance the account sequence by more
-    // than one, which the TxQ's single-sequence forecast cannot model. It must
-    // apply straight to the open ledger or not at all.
+    // A Batch is never queued: its inner transactions can change the sequence
+    // numbers of multiple accounts, which the TxQ's per-account model cannot
+    // forecast. It must apply straight to the open ledger or not at all.
     if (tx.getTxnType() == ttBATCH)
         return telCAN_NOT_QUEUE;