diff --git a/include/xrpl/core/HashRouter.h b/include/xrpl/core/HashRouter.h index 8521f3fed2..b2057283c1 100644 --- a/include/xrpl/core/HashRouter.h +++ b/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 }; diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 605e0dd396..83794e8e23 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -436,7 +436,7 @@ STTx::checkSingleSign(STObject const& sigObject) const std::expected 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 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 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(st.getFieldU16(sfTransactionType)) == ttBATCH, - "xrpl::isBatchRawTransactionOkay : not a batch transaction"); + "xrpl::isBatchRawTransactionOkay : batch transaction"); if (st.isFieldPresent(sfBatchSigners) && st.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index fbda253c64..8b239da32d 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/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. diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index 1f67b726d1..c11a2c0684 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/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; } diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index c715189eb8..ba22880e47 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -390,9 +390,9 @@ TxQ::canBeHeld( std::optional const& replacementIter, std::scoped_lock 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;