From 86d8b244d6e9b61952de736c010c789e12c6498e Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Wed, 1 Jul 2026 08:47:14 -0400 Subject: [PATCH] feat: Add Batch (XLS-56) V1_1 (#6446) Co-authored-by: Mayukha Vadari --- include/xrpl/core/HashRouter.h | 6 +- include/xrpl/ledger/OpenView.h | 2 +- include/xrpl/protocol/Batch.h | 10 +- include/xrpl/protocol/Protocol.h | 3 + include/xrpl/protocol/STObject.h | 5 + include/xrpl/protocol/STTx.h | 53 +- include/xrpl/protocol/TER.h | 2 + include/xrpl/protocol/detail/features.macro | 8 +- .../xrpl/protocol/detail/transactions.macro | 2 +- .../protocol_autogen/transactions/Batch.h | 2 +- include/xrpl/tx/ApplyContext.h | 6 +- include/xrpl/tx/Transactor.h | 49 +- include/xrpl/tx/transactors/system/Batch.h | 8 +- src/libxrpl/protocol/STObject.cpp | 14 + src/libxrpl/protocol/STTx.cpp | 133 +- src/libxrpl/protocol/TER.cpp | 2 + src/libxrpl/tx/Transactor.cpp | 105 +- src/libxrpl/tx/apply.cpp | 34 +- .../tx/transactors/lending/LoanSet.cpp | 2 +- .../tx/transactors/payment/Payment.cpp | 32 +- src/libxrpl/tx/transactors/system/Batch.cpp | 153 +- src/test/app/Batch_test.cpp | 1227 ++++++++++++++--- .../app/ConfidentialTransferExtended_test.cpp | 35 +- src/test/app/LedgerReplay_test.cpp | 45 +- src/test/app/TxQ_test.cpp | 3 +- src/test/jtx/TestHelpers.h | 4 +- src/test/jtx/batch.h | 5 +- src/test/jtx/impl/batch.cpp | 16 +- src/test/rpc/Feature_test.cpp | 2 +- src/test/rpc/Simulate_test.cpp | 16 +- src/xrpld/app/ledger/detail/BuildLedger.cpp | 9 + src/xrpld/app/misc/NetworkOPs.cpp | 7 +- src/xrpld/app/misc/detail/TxQ.cpp | 7 + src/xrpld/overlay/detail/PeerImp.cpp | 4 +- .../rpc/handlers/transaction/Simulate.cpp | 8 + 35 files changed, 1536 insertions(+), 483 deletions(-) diff --git a/include/xrpl/core/HashRouter.h b/include/xrpl/core/HashRouter.h index d36b8aee6e..c8b34d8e93 100644 --- a/include/xrpl/core/HashRouter.h +++ b/include/xrpl/core/HashRouter.h @@ -19,12 +19,14 @@ 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 }; diff --git a/include/xrpl/ledger/OpenView.h b/include/xrpl/ledger/OpenView.h index 4ba2a7759b..d145473516 100644 --- a/include/xrpl/ledger/OpenView.h +++ b/include/xrpl/ledger/OpenView.h @@ -28,7 +28,7 @@ inline constexpr struct OpenLedgerT /** Batch view construction tag. Views constructed with this tag are part of a stack of views - used during batch transaction applied. + used during batch transaction application. */ inline constexpr struct BatchViewT { diff --git a/include/xrpl/protocol/Batch.h b/include/xrpl/protocol/Batch.h index 2f2412b3ff..4e021442d6 100644 --- a/include/xrpl/protocol/Batch.h +++ b/include/xrpl/protocol/Batch.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -7,9 +8,16 @@ namespace xrpl { inline void -serializeBatch(Serializer& msg, std::uint32_t const& flags, std::vector const& txids) +serializeBatch( + Serializer& msg, + AccountID const& outerAccount, + std::uint32_t outerSeqValue, + std::uint32_t const& flags, + std::vector const& txids) { msg.add32(HashPrefix::Batch); + msg.addBitString(outerAccount); + msg.add32(outerSeqValue); msg.add32(flags); msg.add32(std::uint32_t(txids.size())); for (auto const& txid : txids) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 7eac92e83c..ba6347dfa7 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -311,6 +311,9 @@ constexpr std::size_t kPermissionMaxSize = 10; /** The maximum number of transactions that can be in a batch. */ constexpr std::size_t kMaxBatchTxCount = 8; +/** The maximum number of batch signers. */ +constexpr std::size_t kMaxBatchSigners = kMaxBatchTxCount * 3; + /** Length of a secp256k1 scalar in bytes. */ constexpr std::size_t kEcScalarLength = kMPT_SCALAR_SIZE; diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index e65cc79c78..044e48ce62 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -217,6 +217,11 @@ public: [[nodiscard]] AccountID getAccountID(SField const& field) const; + /** The account responsible for the fee and authorization: the delegate when + sfDelegate is present, otherwise the account. */ + [[nodiscard]] AccountID + getFeePayer() const; + [[nodiscard]] Blob getFieldVL(SField const& field) const; [[nodiscard]] STAmount const& diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index 659fede31d..e78fce27a1 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -12,6 +12,7 @@ #include #include +#include namespace xrpl { @@ -51,51 +52,48 @@ public: STTx(TxType type, std::function assembler); // STObject functions. - SerializedTypeID + [[nodiscard]] SerializedTypeID getSType() const override; - std::string + [[nodiscard]] std::string getFullText() const override; // Outer transaction functions / signature functions. static Blob getSignature(STObject const& sigObject); - Blob + [[nodiscard]] Blob getSignature() const { return getSignature(*this); } - uint256 + [[nodiscard]] uint256 getSigningHash() const; - TxType + [[nodiscard]] TxType getTxnType() const; - Blob + [[nodiscard]] Blob getSigningPubKey() const; - SeqProxy + [[nodiscard]] SeqProxy getSeqProxy() const; /** Returns the first non-zero value of (Sequence, TicketSequence). */ - std::uint32_t + [[nodiscard]] std::uint32_t getSeqValue() const; - AccountID - getFeePayer() const; - - boost::container::flat_set + [[nodiscard]] boost::container::flat_set getMentionedAccounts() const; - uint256 + [[nodiscard]] uint256 getTransactionID() const; - json::Value + [[nodiscard]] json::Value getJson(JsonOptions options) const override; - json::Value + [[nodiscard]] json::Value getJson(JsonOptions options, bool binary) const; void @@ -108,27 +106,27 @@ public: @param rules The current ledger rules. @return `true` if valid signature. If invalid, the error message string. */ - std::expected + [[nodiscard]] std::expected checkSign(Rules const& rules) const; - std::expected + [[nodiscard]] std::expected checkBatchSign(Rules const& rules) const; // SQL Functions with metadata. static std::string const& getMetaSQLInsertReplaceHeader(); - std::string + [[nodiscard]] std::string getMetaSQL(std::uint32_t inLedger, std::string const& escapedMetaData) const; - std::string + [[nodiscard]] std::string getMetaSQL( Serializer rawTxn, std::uint32_t inLedger, TxnSql status, std::string const& escapedMetaData) const; - std::vector const& + [[nodiscard]] std::vector const& getBatchTransactionIDs() const; private: @@ -138,28 +136,31 @@ private: Will be *this more often than not. @return `true` if valid signature. If invalid, the error message string. */ - std::expected + [[nodiscard]] std::expected checkSign(Rules const& rules, STObject const& sigObject) const; - std::expected + [[nodiscard]] std::expected checkSingleSign(STObject const& sigObject) const; - std::expected + [[nodiscard]] std::expected checkMultiSign(Rules const& rules, STObject const& sigObject) const; - std::expected + [[nodiscard]] std::expected checkBatchSingleSign(STObject const& batchSigner) const; - std::expected + [[nodiscard]] std::expected checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const; + void + buildBatchTxnIds(); + STBase* copy(std::size_t n, void* buf) const override; STBase* move(std::size_t n, void* buf) override; friend class detail::STVar; - mutable std::vector batchTxnIds_; + std::optional> batchTxnIds_; }; bool diff --git a/include/xrpl/protocol/TER.h b/include/xrpl/protocol/TER.h index 84c344ea76..4ee084e714 100644 --- a/include/xrpl/protocol/TER.h +++ b/include/xrpl/protocol/TER.h @@ -175,6 +175,8 @@ enum TEFcodes : TERUnderlyingType { tefNO_TICKET, tefNFTOKEN_IS_NOT_TRANSFERABLE, tefINVALID_LEDGER_FIX_TYPE, + tefNO_DST_PARTIAL, + tefBAD_PATH_COUNT, }; //------------------------------------------------------------------------------ diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 3c808d960b..a1911761bb 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -14,13 +14,14 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. -XRPL_FEATURE(LendingProtocolV1_1, Supported::No, VoteBehavior::DefaultNo) -XRPL_FEATURE(ConfidentialTransfer, Supported::No, VoteBehavior::DefaultNo) + +XRPL_FEATURE(BatchV1_1, Supported::No, VoteBehavior::DefaultNo) +XRPL_FEATURE(LendingProtocolV1_1, Supported::No, VoteBehavior::DefaultNo) +XRPL_FEATURE(ConfidentialTransfer, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_3_0, Supported::Yes, 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) -XRPL_FIX (BatchInnerSigs, Supported::No, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionDelegationV1_1, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (DirectoryLimit, Supported::Yes, VoteBehavior::DefaultNo) @@ -34,7 +35,6 @@ XRPL_FEATURE(TokenEscrow, Supported::Yes, VoteBehavior::DefaultN XRPL_FIX (EnforceNFTokenTrustlineV2, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (AMMv1_3, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionedDEX, Supported::Yes, VoteBehavior::DefaultNo) -XRPL_FEATURE(Batch, Supported::No, VoteBehavior::DefaultNo) XRPL_FEATURE(SingleAssetVault, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (PayChanCancelAfter, Supported::Yes, VoteBehavior::DefaultNo) // Check flags in Credential transactions diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index b46fca92b6..b0c4f66bae 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -940,7 +940,7 @@ TRANSACTION(ttVAULT_CLAWBACK, 70, VaultClawback, #endif TRANSACTION(ttBATCH, 71, Batch, Delegation::NotDelegable, - featureBatch, + featureBatchV1_1, NoPriv, ({ {sfRawTransactions, SoeRequired}, diff --git a/include/xrpl/protocol_autogen/transactions/Batch.h b/include/xrpl/protocol_autogen/transactions/Batch.h index 55b99d0eef..00f553ada7 100644 --- a/include/xrpl/protocol_autogen/transactions/Batch.h +++ b/include/xrpl/protocol_autogen/transactions/Batch.h @@ -20,7 +20,7 @@ class BatchBuilder; * * Type: ttBATCH (71) * Delegable: Delegation::NotDelegable - * Amendment: featureBatch + * Amendment: featureBatchV1_1 * Privileges: NoPriv * * Immutable wrapper around STTx providing type-safe field access. diff --git a/include/xrpl/tx/ApplyContext.h b/include/xrpl/tx/ApplyContext.h index 8540037601..c98b3f82c5 100644 --- a/include/xrpl/tx/ApplyContext.h +++ b/include/xrpl/tx/ApplyContext.h @@ -24,6 +24,10 @@ public: ApplyFlags flags, beast::Journal journal = beast::Journal{beast::Journal::getNullSink()}); + // Convenience constructor used only by tests that build an ApplyContext + // directly (e.g. invariant checks). Production always uses the parentBatchId + // constructor above; this one fixes parentBatchId to std::nullopt and so is + // never valid for a batch inner (hence the TapBatch assert). explicit ApplyContext( ServiceRegistry& registry, OpenView& base, @@ -124,7 +128,7 @@ private: ApplyFlags flags_; std::optional view_; - // The ID of the batch transaction we are executing under, if seated. + // The ID of the batch transaction we are executing under, if set. std::optional parentBatchId_; }; diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 02fedfe970..2b50cfa6e7 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -180,9 +180,6 @@ public: static NotTEC checkSign(PreclaimContext const& ctx); - static NotTEC - checkBatchSign(PreclaimContext const& ctx); - // Returns the fee in fee units, not scaled for load. static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); @@ -373,7 +370,12 @@ protected: std::optional const& parentBatchId, AccountID const& idAccount, STObject const& sigObject, - beast::Journal const j); + beast::Journal const j, + // A batch may carry an inner from an account that an earlier inner + // creates, so the signer account need not exist yet; when it does not, + // only its own master key may authorize it. Normal transactions require + // the account to already exist. + bool permitUncreatedAccount = false); // Base class always returns true static bool @@ -413,25 +415,8 @@ protected: std::optional value, unit::ValueUnit min = unit::ValueUnit{}); -private: - static NotTEC - checkPermission( - ReadView const& view, - STTx const& tx, - std::unordered_set& heldGranularPermissions); - - std::pair - reset(XRPAmount fee); - - TER - consumeSeqProxy(SLE::pointer const& sleAccount); - - TER - payFee(); - - std::tuple - processPersistentChanges(TER result, XRPAmount fee); - + // Signature-authorization helpers. protected so the Batch transactor can + // reuse them when validating each BatchSigner in Batch::checkBatchSign. static NotTEC checkSingleSign( ReadView const& view, @@ -448,6 +433,24 @@ private: STObject const& sigObject, beast::Journal const j); +private: + static NotTEC + checkPermission( + ReadView const& view, + STTx const& tx, + std::unordered_set& heldGranularPermissions); + + std::pair + reset(XRPAmount fee); + + TER + consumeSeqProxy(SLE::pointer const& sleAccount); + TER + payFee(); + + std::tuple + processPersistentChanges(TER result, XRPAmount fee); + void trapTransaction(uint256) const; /** Performs early sanity checks on the account and fee fields. diff --git a/include/xrpl/tx/transactors/system/Batch.h b/include/xrpl/tx/transactors/system/Batch.h index 43f0103319..927a5b27cd 100644 --- a/include/xrpl/tx/transactors/system/Batch.h +++ b/include/xrpl/tx/transactors/system/Batch.h @@ -1,7 +1,5 @@ #pragma once -#include -#include #include namespace xrpl { @@ -61,6 +59,12 @@ public: ttLOAN_MANAGE, ttLOAN_PAY, }); + +private: + // Skips signature verification for inner txns, so keep it private: it must + // only be reached through Batch::checkSign. + static NotTEC + checkBatchSign(PreclaimContext const& ctx); }; } // namespace xrpl diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index e16cbc871f..445da16828 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -635,6 +635,20 @@ STObject::getAccountID(SField const& field) const return getFieldByValue(field); } +AccountID +STObject::getFeePayer() const +{ + // If sfDelegate is present, the delegate account is the payer + // note: if a delegate is specified, its authorization to act on behalf of the account is + // enforced in `Transactor::invokeCheckPermission` + // cryptographic signature validity is checked separately (e.g., in `Transactor::checkSign`) + if (isFieldPresent(sfDelegate)) + return getAccountID(sfDelegate); + + // Default payer + return getAccountID(sfAccount); +} + Blob STObject::getFieldVL(SField const& field) const { diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index be3b1a082f..cd2da12316 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -1,7 +1,6 @@ #include #include -#include #include #include #include @@ -72,6 +71,7 @@ STTx::STTx(STObject&& object) : STObject(std::move(object)) txType_ = safeCast(getFieldU16(sfTransactionType)); applyTemplate(getTxFormat(txType_)->getSOTemplate()); // may throw tid_ = getHash(HashPrefix::TransactionId); + buildBatchTxnIds(); } STTx::STTx(SerialIter& sit) : STObject(sfTransaction) @@ -88,6 +88,7 @@ STTx::STTx(SerialIter& sit) : STObject(sfTransaction) applyTemplate(getTxFormat(txType_)->getSOTemplate()); // May throw tid_ = getHash(HashPrefix::TransactionId); + buildBatchTxnIds(); } STTx::STTx(TxType type, std::function assembler) : STObject(sfTransaction) @@ -105,6 +106,7 @@ STTx::STTx(TxType type, std::function assembler) : STObject(sfT logicError("Transaction type was mutated during assembly"); tid_ = getHash(HashPrefix::TransactionId); + buildBatchTxnIds(); } STBase* @@ -212,20 +214,6 @@ STTx::getSeqValue() const return getSeqProxy().value(); } -AccountID -STTx::getFeePayer() const -{ - // If sfDelegate is present, the delegate account is the payer - // note: if a delegate is specified, its authorization to act on behalf of the account is - // enforced in `Transactor::invokeCheckPermission` - // cryptographic signature validity is checked separately (e.g., in `Transactor::checkSign`) - if (isFieldPresent(sfDelegate)) - return getAccountID(sfDelegate); - - // Default payer - return getAccountID(sfAccount); -} - void STTx::sign( PublicKey const& publicKey, @@ -279,6 +267,17 @@ STTx::checkSign(Rules const& rules) const if (auto const ret = checkSign(rules, counterSig); !ret) return std::unexpected("Counterparty: " + ret.error()); } + + // Verify the batch signer signatures here too, so they are cached with the + // rest of signature checking (checkValidity / SF_SIGGOOD) and stay out of + // the transaction engine. Gated on a batch (batchTxnIds_ seated) that + // actually carries signers; a batch whose inners are all from the outer + // account has no sfBatchSigners and needs no signer crypto. + if (batchTxnIds_ && isFieldPresent(sfBatchSigners)) + { + if (auto const ret = checkBatchSign(rules); !ret) + return ret; + } return {}; } @@ -287,12 +286,15 @@ STTx::checkBatchSign(Rules const& rules) const { try { - XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchSign : not a batch transaction"); if (getTxnType() != ttBATCH) { - JLOG(debugLog().fatal()) << "not a batch transaction"; + // LCOV_EXCL_START + UNREACHABLE("STTx::checkBatchSign : not a batch transaction"); return std::unexpected("Not a batch transaction."); + // LCOV_EXCL_STOP } + if (!isFieldPresent(sfBatchSigners)) + return std::unexpected("Missing BatchSigners field."); // LCOV_EXCL_LINE STArray const& signers{getFieldArray(sfBatchSigners)}; for (auto const& signer : signers) { @@ -307,9 +309,10 @@ STTx::checkBatchSign(Rules const& rules) const } catch (std::exception const& e) { - JLOG(debugLog().error()) << "Batch signature check failed: " << e.what(); + // LCOV_EXCL_START + return std::unexpected(std::string("Internal batch signature check failure: ") + e.what()); + // LCOV_EXCL_STOP } - return std::unexpected("Internal batch signature check failure."); } json::Value @@ -429,8 +432,11 @@ STTx::checkSingleSign(STObject const& sigObject) const std::expected STTx::checkBatchSingleSign(STObject const& batchSigner) const { + XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchSingleSign : batch transaction"); Serializer msg; - serializeBatch(msg, getFlags(), getBatchTransactionIDs()); + serializeBatch( + msg, getAccountID(sfAccount), getSeqValue(), getFlags(), getBatchTransactionIDs()); + finishMultiSigningData(batchSigner.getAccountID(sfAccount), msg); return singleSignHelper(batchSigner, msg.slice()); } @@ -504,7 +510,7 @@ multiSignHelper( { return std::unexpected( std::string("Invalid signature on account ") + toBase58(accountID) + - errorWhat.value_or("") + "."); + (errorWhat ? ": " + *errorWhat : "") + "."); } } // All signatures verified. @@ -514,14 +520,18 @@ multiSignHelper( std::expected STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const { + 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. + auto const batchSignerAccount = batchSigner.getAccountID(sfAccount); Serializer dataStart; - serializeBatch(dataStart, getFlags(), getBatchTransactionIDs()); + serializeBatch( + dataStart, getAccountID(sfAccount), getSeqValue(), getFlags(), getBatchTransactionIDs()); + dataStart.addBitString(batchSignerAccount); return multiSignHelper( batchSigner, - std::nullopt, + batchSignerAccount, [&dataStart](AccountID const& accountID) -> Serializer { Serializer s = dataStart; finishMultiSigningData(accountID, s); @@ -555,42 +565,38 @@ STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const rules); } -/** - * @brief Retrieves a batch of transaction IDs from the STTx. - * - * This function returns a vector of transaction IDs by extracting them from - * the field array `sfRawTransactions` within the STTx. If the batch - * transaction IDs have already been computed and cached in `batchTxnIds_`, - * it returns the cached vector. Otherwise, it computes the transaction IDs, - * caches them, and then returns the vector. - * - * @return A vector of `uint256` containing the batch transaction IDs. - * - * @note The function asserts that the `sfRawTransactions` field array is not - * empty and that the size of the computed batch transaction IDs matches the - * size of the `sfRawTransactions` field array. - */ +void +STTx::buildBatchTxnIds() +{ + // Precondition: the template must have been applied first, so the fields + // (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 applied"); + if (getTxnType() != ttBATCH || !isFieldPresent(sfRawTransactions)) + return; + + auto const& raw = getFieldArray(sfRawTransactions); + + // Seated for any batch with raw transactions. The count is validated in + // preflight and at the relay boundary, so build every id here; this keeps + // the invariant batchTxnIds_->size() == rawTransactions.size(). + auto& ids = batchTxnIds_.emplace(); + ids.reserve(raw.size()); + for (STObject const& rb : raw) + ids.push_back(rb.getHash(HashPrefix::TransactionId)); +} + 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( - !getFieldArray(sfRawTransactions).empty(), - "STTx::getBatchTransactionIDs : empty raw transactions"); - - // The list of inner ids is built once, then reused on subsequent calls. - // After the list is built, it must always have the same size as the array - // `sfRawTransactions`. The assert below verifies that. - if (batchTxnIds_.empty()) - { - for (STObject const& rb : getFieldArray(sfRawTransactions)) - batchTxnIds_.push_back(rb.getHash(HashPrefix::TransactionId)); - } - + batchTxnIds_.has_value(), "STTx::getBatchTransactionIDs : batch transaction IDs built"); XRPL_ASSERT( - batchTxnIds_.size() == getFieldArray(sfRawTransactions).size(), + batchTxnIds_->size() == getFieldArray(sfRawTransactions).size(), "STTx::getBatchTransactionIDs : batch transaction IDs size mismatch"); - return batchTxnIds_; + return *batchTxnIds_; } //------------------------------------------------------------------------------ @@ -727,13 +733,22 @@ invalidMPTAmountInTx(STObject const& tx) } static bool -isRawTransactionOkay(STObject const& st, std::string& reason) +isBatchRawTransactionOkay(STObject const& st, std::string& reason) { if (!st.isFieldPresent(sfRawTransactions)) return true; + // sfRawTransactions only appears on a Batch. passesLocalChecks runs on + // unverified user and peer input, so reject (rather than assert) a non-batch + // transaction that carries it. + if (st.getFieldU16(sfTransactionType) != ttBATCH) + { + reason = "Only Batch transactions may contain raw transactions."; + return false; + } + if (st.isFieldPresent(sfBatchSigners) && - st.getFieldArray(sfBatchSigners).size() > kMaxBatchTxCount) + st.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) { reason = "Batch Signers array exceeds max entries."; return false; @@ -757,6 +772,12 @@ isRawTransactionOkay(STObject const& st, std::string& reason) } raw.applyTemplate(getTxFormat(tt)->getSOTemplate()); + + // passesLocalChecks recurses back into isBatchRawTransactionOkay, + // but an inner can never be a batch (rejected above), so the + // recursion terminates at depth 1. + if (!passesLocalChecks(raw, reason)) + return false; } catch (std::exception const& e) { @@ -791,7 +812,7 @@ passesLocalChecks(STObject const& st, std::string& reason) return false; } - if (!isRawTransactionOkay(st, reason)) + if (!isBatchRawTransactionOkay(st, reason)) return false; return true; diff --git a/src/libxrpl/protocol/TER.cpp b/src/libxrpl/protocol/TER.cpp index e5c1d17b1a..a6f8192a2f 100644 --- a/src/libxrpl/protocol/TER.cpp +++ b/src/libxrpl/protocol/TER.cpp @@ -130,6 +130,8 @@ transResults() MAKE_ERROR(tefNO_TICKET, "Ticket is not in ledger."), MAKE_ERROR(tefNFTOKEN_IS_NOT_TRANSFERABLE, "The specified NFToken is not transferable."), MAKE_ERROR(tefINVALID_LEDGER_FIX_TYPE, "The LedgerFixType field has an invalid value."), + MAKE_ERROR(tefNO_DST_PARTIAL, "Partial payment to create account not allowed."), + MAKE_ERROR(tefBAD_PATH_COUNT, "Malformed: Too many paths."), MAKE_ERROR(telLOCAL_ERROR, "Local failure."), MAKE_ERROR(telBAD_DOMAIN, "Domain too long."), diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 9ab8779f03..0f6988543e 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -1,6 +1,5 @@ #include -#include #include #include #include @@ -221,13 +220,15 @@ Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask) if (ctx.tx.getSeqProxy().isTicket() && ctx.tx.isFieldPresent(sfAccountTxnID)) return temINVALID; - if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch)) + if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatchV1_1)) return temINVALID_FLAG; - XRPL_ASSERT( - ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() || - !ctx.rules.enabled(featureBatch), - "Inner batch transaction must have a parent batch ID."); + // Reject if the inner batch flag and parentBatchId are inconsistent. + // A standalone tx with tfInnerBatchTxn but no parentBatchId is an + // attack attempt. A tx with parentBatchId but without tfInnerBatchTxn + // is a programming error. + if (ctx.tx.isFlag(tfInnerBatchTxn) != ctx.parentBatchId.has_value()) + return temINVALID_INNER_BATCH; return tesSUCCESS; } @@ -243,15 +244,19 @@ Transactor::preflight2(PreflightContext const& ctx) return *ret; } - // It should be impossible for the InnerBatchTxn flag to be set without - // featureBatch being enabled - XRPL_ASSERT_PARTS( - !ctx.tx.isFlag(tfInnerBatchTxn) || ctx.rules.enabled(featureBatch), - "xrpl::Transactor::preflight2", - "InnerBatch flag only set if feature enabled"); - // Skip signature check on batch inner transactions - if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch)) + // Skip the signature check on batch inner transactions. preflight1 already + // enforces both conditions; re-checking them as defense in depth guarantees + // we never return success (and so skip signature validation) for an inner + // transaction unless the amendment is enabled and it really sits inside a + // batch. + 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. @@ -708,23 +713,26 @@ Transactor::checkSign( std::optional const& parentBatchId, AccountID const& idAccount, STObject const& sigObject, - beast::Journal const j) + beast::Journal const j, + bool permitUncreatedAccount) { { auto const sle = view.read(keylet::account(idAccount)); - if (view.rules().enabled(featureLendingProtocol) && isPseudoAccount(sle)) + if ((view.rules().enabled(featureLendingProtocol) || + view.rules().enabled(featureBatchV1_1) || view.rules().enabled(fixCleanup3_3_0)) && + isPseudoAccount(sle)) { - // Pseudo-accounts can't sign transactions. This check is gated on - // the Lending Protocol amendment because that's the project it was - // added under, and it doesn't justify another amendment + // Pseudo-accounts can't sign transactions. This check is gated on a + // few different amendments so that it takes effect as soon as any of + // them is activated. return tefBAD_AUTH; } } auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey); // Ignore signature check on batch inner transactions - if (parentBatchId && view.rules().enabled(featureBatch)) + if (parentBatchId && view.rules().enabled(featureBatchV1_1)) { // Defensive Check: These values are also checked in Batch::preflight if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() || @@ -762,7 +770,16 @@ Transactor::checkSign( auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner))); auto const sleAccount = view.read(keylet::account(idAccount)); if (!sleAccount) - return terNO_ACCOUNT; + { + // An account that does not exist yet can only be authorized by its own + // master key, and only where an un-created signer is permitted (a batch + // whose earlier inner creates the account). Otherwise it cannot sign. + if (!permitUncreatedAccount) + return terNO_ACCOUNT; + if (idAccount != idSigner) + return tefBAD_AUTH; + return tesSUCCESS; + } return checkSingleSign(view, idSigner, idAccount, sleAccount, j); } @@ -775,50 +792,6 @@ Transactor::checkSign(PreclaimContext const& ctx) return checkSign(ctx.view, ctx.flags, ctx.parentBatchId, idAccount, ctx.tx, ctx.j); } -NotTEC -Transactor::checkBatchSign(PreclaimContext const& ctx) -{ - NotTEC ret = tesSUCCESS; - STArray const& signers{ctx.tx.getFieldArray(sfBatchSigners)}; - for (auto const& signer : signers) - { - auto const idAccount = signer.getAccountID(sfAccount); - - Blob const& pkSigner = signer.getFieldVL(sfSigningPubKey); - if (pkSigner.empty()) - { - if (ret = checkMultiSign(ctx.view, ctx.flags, idAccount, signer, ctx.j); - !isTesSuccess(ret)) - return ret; - } - else - { - // LCOV_EXCL_START - if (!publicKeyType(makeSlice(pkSigner))) - return tefBAD_AUTH; - // LCOV_EXCL_STOP - - auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner))); - auto const sleAccount = ctx.view.read(keylet::account(idAccount)); - - // A batch can include transactions from an un-created account ONLY - // when the account master key is the signer - if (!sleAccount) - { - if (idAccount != idSigner) - return tefBAD_AUTH; - - return tesSUCCESS; - } - - if (ret = checkSingleSign(ctx.view, idSigner, idAccount, sleAccount, ctx.j); - !isTesSuccess(ret)) - return ret; - } - } - return ret; -} - NotTEC Transactor::checkSingleSign( ReadView const& view, @@ -1114,7 +1087,7 @@ Transactor::reset(XRPAmount fee) // balance should have already been checked in checkFee / preFlight. XRPL_ASSERT( - balance != beast::kZero && (!view().open() || balance >= fee), + (fee == beast::kZero || balance != beast::kZero) && (!view().open() || balance >= fee), "xrpl::Transactor::reset : valid balance"); // We retry/reject the transaction if the account balance is zero or diff --git a/src/libxrpl/tx/apply.cpp b/src/libxrpl/tx/apply.cpp index b70cb0d345..d85c4cfc40 100644 --- a/src/libxrpl/tx/apply.cpp +++ b/src/libxrpl/tx/apply.cpp @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include @@ -39,30 +38,14 @@ checkValidity(HashRouter& router, STTx const& tx, Rules const& rules) auto const id = tx.getTransactionID(); auto const flags = router.getFlags(id); - // Ignore signature check on batch inner transactions - if (tx.isFlag(tfInnerBatchTxn) && rules.enabled(featureBatch)) + // Batch inner transactions are never independently valid: they are applied + // within their batch, not through checkValidity. Reaching here means one was + // relayed or submitted on its own, so mark it bad regardless of the + // amendment (like PeerImp and NetworkOPs). + if (tx.isFlag(tfInnerBatchTxn)) { - // Defensive Check: These values are also checked in Batch::preflight - if (tx.isFieldPresent(sfTxnSignature) || !tx.getSigningPubKey().empty() || - tx.isFieldPresent(sfSigners)) - return {Validity::SigBad, "Malformed: Invalid inner batch transaction."}; - - // This block should probably have never been included in the - // original `Batch` implementation. An inner transaction never - // has a valid signature. - bool const neverValid = rules.enabled(fixBatchInnerSigs); - if (!neverValid) - { - std::string reason; - if (!passesLocalChecks(tx, reason)) - { - router.setFlags(id, kSfLocalbad); - return {Validity::SigGoodOnly, reason}; - } - - router.setFlags(id, kSfSiggood); - return {Validity::Valid, ""}; - } + router.setFlags(id, kSfSigbad); + return {Validity::SigBad, "Batch inner transactions are never considered validly signed."}; } if (any(flags & kSfSigbad)) @@ -183,6 +166,9 @@ applyBatchTransactions( // If the transaction should be applied push its changes to the // whole-batch view. + // NOTE: each inner tx is individually capped at kOversizeMetaDataCap; + // there is no aggregate cap here. Bounded by kMaxBatchTxCount * cap, + // which standalone txns can already produce in one ledger. if (ret.applied && (isTesSuccess(ret.ter) || isTecClaim(ret.ter))) perTxBatchView.apply(batchView); diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index 903707320b..1ac387d1b1 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -57,7 +57,7 @@ LoanSet::preflight(PreflightContext const& ctx) auto const& tx = ctx.tx; // Special case for Batch inner transactions - if (tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch) && + if (tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatchV1_1) && !tx.isFieldPresent(sfCounterparty)) { auto const parentBatchId = ctx.parentBatchId.value_or(uint256{0}); diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index 7f1e4d8079..6e3883572a 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -363,16 +363,21 @@ Payment::preclaim(PreclaimContext const& ctx) // transaction would succeed. return tecNO_DST; } - if (ctx.view.open() && partialPaymentAllowed) + // A partial payment may not fund a new account. + if (partialPaymentAllowed) { - // You cannot fund an account with a partial payment. - // Make retry work smaller, by rejecting this. - JLOG(ctx.j.trace()) << "Delay transaction: Partial payment not " - "allowed to create account."; - - // Another transaction could create the account and then this - // transaction would succeed. - return telNO_DST_PARTIAL; + // Open view: the soft tel (unchanged). + if (ctx.view.open()) + { + // Make retry work smaller, by rejecting this. + JLOG(ctx.j.trace()) << "Delay transaction: Partial payment not " + "allowed to create account."; + return telNO_DST_PARTIAL; + } + // Inner batch txns are claimed on a closed view, where a tel is + // invalid, so use the tef. + if (ctx.parentBatchId && ctx.view.rules().enabled(featureBatchV1_1)) + return tefNO_DST_PARTIAL; } if (dstAmount < STAmount(ctx.view.fees().reserve)) { @@ -400,7 +405,7 @@ Payment::preclaim(PreclaimContext const& ctx) } // Payment with at least one intermediate step and uses transitive balances. - if ((hasPaths || sendMax || !dstAmount.native()) && ctx.view.open()) + if (hasPaths || sendMax || !dstAmount.native()) { STPathSet const& paths = ctx.tx.getFieldPathSet(sfPaths); @@ -408,7 +413,12 @@ Payment::preclaim(PreclaimContext const& ctx) return path.size() > kMaxPathLength; })) { - return telBAD_PATH_COUNT; + // Open view: the soft tel (unchanged). Inner batch txns are claimed + // on a closed view, where a tel is invalid, so use the tef. + if (ctx.view.open()) + return telBAD_PATH_COUNT; + if (ctx.parentBatchId && ctx.view.rules().enabled(featureBatchV1_1)) + return tefBAD_PATH_COUNT; } } diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index 64a62ac273..16d27f98d9 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include #include #include @@ -21,11 +23,13 @@ #include #include +#include #include #include #include #include #include +#include namespace xrpl { @@ -107,13 +111,13 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) } // Calculate the Signers/BatchSigners Fees - std::int32_t signerCount = 0; + std::uint32_t signerCount = 0; if (tx.isFieldPresent(sfBatchSigners)) { auto const& signers = tx.getFieldArray(sfBatchSigners); // LCOV_EXCL_START - if (signers.size() > kMaxBatchTxCount) + if (signers.size() > kMaxBatchSigners) { JLOG(debugLog().error()) << "BatchTrace: Batch Signers array exceeds max entries."; return XRPAmount{kInitialXrp}; @@ -128,7 +132,16 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) } else if (signer.isFieldPresent(sfSigners)) { - signerCount += signer.getFieldArray(sfSigners).size(); + auto const& nestedSigners = signer.getFieldArray(sfSigners); + // LCOV_EXCL_START + if (nestedSigners.size() > STTx::kMaxMultiSigners) + { + JLOG(debugLog().error()) + << "BatchTrace: Nested Signers array exceeds max entries."; + return kInitialXrp; + } + // LCOV_EXCL_STOP + signerCount += nestedSigners.size(); } } } @@ -149,7 +162,8 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in signerFees calculation."; return XRPAmount{kInitialXrp}; } - if (txnFees + signerFees > maxAmount - batchBase) + XRPAmount const innerFees = txnFees + signerFees; + if (innerFees > maxAmount - batchBase) { JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in total fee calculation."; return XRPAmount{kInitialXrp}; @@ -157,7 +171,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) // LCOV_EXCL_STOP // 10 drops per batch signature + sum of inner tx fees + batchBase - return signerFees + txnFees + batchBase; + return innerFees + batchBase; } std::uint32_t @@ -227,6 +241,14 @@ Batch::preflight(PreflightContext const& ctx) return temARRAY_TOO_LARGE; } + if (ctx.tx.isFieldPresent(sfBatchSigners) && + ctx.tx.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) + { + JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]:" + << "signers array exceeds " << kMaxBatchSigners << " entries."; + return temARRAY_TOO_LARGE; + } + // Validation Inner Batch Txns std::unordered_set uniqueHashes; std::unordered_map> accountSeqTicket; @@ -384,46 +406,48 @@ Batch::preflight(PreflightContext const& ctx) NotTEC Batch::preflightSigValidated(PreflightContext const& ctx) { + XRPL_ASSERT( + 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); - // Build the signers list - std::unordered_set requiredSigners; + // Accounts that must sign the batch: each inner authorizer and counterparty + // (excluding the outer account), sorted and de-duplicated to match against + // the ascending, unique batch signers. + std::vector requiredSigners; + requiredSigners.reserve(kMaxBatchSigners); for (STObject const& rb : rawTxns) { - auto const innerAccount = rb.getAccountID(sfAccount); + // A delegated inner is signed by the delegate, not the account holder, + // so the delegate is the required signer when present. + AccountID const authorizer = rb.getFeePayer(); - // If the inner account is the same as the outer account, do not add the - // inner account to the required signers set. - if (innerAccount != outerAccount) - requiredSigners.insert(innerAccount); + // The outer account signs the batch itself, so it is never added to the + // required signers. + if (authorizer != outerAccount) + requiredSigners.push_back(authorizer); // Some transactions have a Counterparty, who must also sign the // transaction if they are not the outer account - if (auto const counterparty = rb.at(~sfCounterparty); + if (auto const counterparty = rb[~sfCounterparty]; counterparty && counterparty != outerAccount) - requiredSigners.insert(*counterparty); + requiredSigners.push_back(*counterparty); } + std::ranges::sort(requiredSigners); + auto const dupes = std::ranges::unique(requiredSigners); + requiredSigners.erase(dupes.begin(), dupes.end()); + + std::size_t numReqSignersMatched = 0; // Validation Batch Signers - std::unordered_set batchSigners; if (ctx.tx.isFieldPresent(sfBatchSigners)) { STArray const& signers = ctx.tx.getFieldArray(sfBatchSigners); - // Check that the batch signers array is not too large. - if (signers.size() > kMaxBatchTxCount) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "signers array exceeds 8 entries."; - return temARRAY_TOO_LARGE; - } - - // Add batch signers to the set to ensure all signer accounts are - // unique. Meanwhile, remove signer accounts from the set of inner - // transaction accounts (`requiredSigners`). By the end of the loop, - // `requiredSigners` should be empty, indicating that all inner - // accounts are matched with signers. + // Batch signers must be strictly ascending and match the required + // signers exactly; since both are sorted, each must be the next + // required signer. + AccountID lastBatchSigner{beast::kZero}; for (auto const& signer : signers) { AccountID const signerAccount = signer.getAccountID(sfAccount); @@ -434,40 +458,66 @@ Batch::preflightSigValidated(PreflightContext const& ctx) return temBAD_SIGNER; } - if (!batchSigners.insert(signerAccount).second) + if (lastBatchSigner == signerAccount) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " << "duplicate signer found: " << signerAccount; - return temREDUNDANT; - } - - // Check that the batch signer is in the required signers set. - // Remove it if it does, as it can be crossed off the list. - if (requiredSigners.erase(signerAccount) == 0) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "no account signature for inner txn."; return temBAD_SIGNER; } - } - // Check the batch signers signatures. - auto const sigResult = ctx.tx.checkBatchSign(ctx.rules); + if (lastBatchSigner > signerAccount) + { + JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " + << "unsorted signers array: " << signerAccount; + return temBAD_SIGNER; + } + lastBatchSigner = signerAccount; - if (!sigResult) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "invalid batch txn signature: " << sigResult.error(); - return temBAD_SIGNATURE; + if (numReqSignersMatched >= requiredSigners.size() || + requiredSigners[numReqSignersMatched] != signerAccount) + { + JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " + << "missing signer or extra signer provided: " << signerAccount; + return temBAD_SIGNER; + } + ++numReqSignersMatched; } } - if (!requiredSigners.empty()) + // Every required signer must be matched. 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; } + + return tesSUCCESS; +} + +NotTEC +Batch::checkBatchSign(PreclaimContext const& ctx) +{ + STArray const& signers{ctx.tx.getFieldArray(sfBatchSigners)}; + for (auto const& signer : signers) + { + // Reuse the standard signer-authorization rules so the batch and + // non-batch paths cannot drift. permitUncreatedAccount allows an inner + // from an account that an earlier inner creates, which must be + // authorized by its own master key. + auto const idAccount = signer.getAccountID(sfAccount); + if (auto const ret = Transactor::checkSign( + ctx.view, + ctx.flags, + ctx.parentBatchId, + idAccount, + signer, + ctx.j, + /*permitUncreatedAccount=*/true); + !isTesSuccess(ret)) + return ret; + } return tesSUCCESS; } @@ -479,7 +529,7 @@ Batch::preflightSigValidated(PreflightContext const& ctx) * corresponding error code. * * Next, it verifies the batch-specific signature requirements by calling - * Transactor::checkBatchSign. If this check fails, it also returns the + * Batch::checkBatchSign. If this check fails, it also returns the * corresponding error code. * * If both checks succeed, the function returns tesSUCCESS. @@ -494,8 +544,11 @@ Batch::checkSign(PreclaimContext const& ctx) if (auto ret = Transactor::checkSign(ctx); !isTesSuccess(ret)) return ret; - if (auto ret = Transactor::checkBatchSign(ctx); !isTesSuccess(ret)) - return ret; + if (ctx.tx.isFieldPresent(sfBatchSigners)) + { + if (auto ret = checkBatchSign(ctx); !isTesSuccess(ret)) + return ret; + } return tesSUCCESS; } diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 6aaae62155..f9c156fe06 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -47,12 +48,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -61,15 +64,17 @@ #include #include #include +#include #include +#include #include #include +#include #include #include #include #include -#include #include #include #include @@ -200,16 +205,13 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - bool const withInnerSigFix = features[fixBatchInnerSigs]; - for (bool const withBatch : {true, false}) { - testcase << "enabled: Batch " << (withBatch ? "enabled" : "disabled") - << ", Inner Sig Fix: " << (withInnerSigFix ? "enabled" : "disabled"); + testcase << "enabled: Batch " << (withBatch ? "enabled" : "disabled"); - auto const amend = withBatch ? features : features - featureBatch; + auto const amend = withBatch ? features : features - featureBatchV1_1; - test::jtx::Env env{*this, amend}; + Env env{*this, amend}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -230,12 +232,10 @@ class Batch_test : public beast::unit_test::Suite } // tfInnerBatchTxn - // If the feature is disabled, the transaction fails with - // temINVALID_FLAG. If the feature is enabled, the transaction fails - // early in checkValidity() + // A standalone transaction carrying this flag is never valid, so it + // is rejected early in checkValidity() regardless of the amendment. { - auto const txResult = withBatch ? Ter(telENV_RPC_FAILED) : Ter(temINVALID_FLAG); - env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), txResult); + env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), Ter(telENV_RPC_FAILED)); env.close(); } @@ -254,7 +254,7 @@ class Batch_test : public beast::unit_test::Suite //---------------------------------------------------------------------- // preflight - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -431,6 +431,41 @@ class Batch_test : public beast::unit_test::Suite env.close(); } + // temINVALID_INNER_BATCH: tfInnerBatchTxn set but no parentBatchId. + { + auto jtx = env.jt(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn)); + PreflightContext const pfCtx( + env.app(), *jtx.stx, env.current()->rules(), TapNone, env.journal); + auto const pf = Transactor::invokePreflight(pfCtx); + BEAST_EXPECT(pf == temINVALID_INNER_BATCH); + } + + // temINVALID_FLAG: tfInnerBatchTxn set but featureBatchV1_1 disabled. + { + Env disabledEnv{*this, features - featureBatchV1_1}; + disabledEnv.fund(XRP(10000), alice, bob); + disabledEnv.close(); + auto jtx = disabledEnv.jt(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn)); + PreflightContext const pfCtx( + disabledEnv.app(), + *jtx.stx, + disabledEnv.current()->rules(), + TapNone, + disabledEnv.journal); + auto const pf = Transactor::invokePreflight(pfCtx); + BEAST_EXPECT(pf == temINVALID_FLAG); + } + + // temINVALID_INNER_BATCH: parentBatchId set but tfInnerBatchTxn not + // set. + { + auto jtx = env.jt(pay(alice, bob, XRP(1))); + PreflightContext const pfCtx( + env.app(), *jtx.stx, uint256{1}, env.current()->rules(), TapBatch, env.journal); + auto const pf = Transactor::invokePreflight(pfCtx); + BEAST_EXPECT(pf == temINVALID_INNER_BATCH); + } + // temBAD_FEE: Batch: inner txn must have a fee of 0. { auto const seq = env.seq(alice); @@ -537,16 +572,16 @@ class Batch_test : public beast::unit_test::Suite env.close(); } - // DEFENSIVE: temARRAY_TOO_LARGE: Batch: signers array exceeds 8 - // entries. + // DEFENSIVE: temARRAY_TOO_LARGE: Batch: signers array exceeds + // kMaxBatchSigners entries. // ACTUAL: telENV_RPC_FAILED: isRawTransactionOkay() { auto const seq = env.seq(alice); - auto const batchFee = batch::calcBatchFee(env, 9, 2); + auto const batchFee = batch::calcBatchFee(env, kMaxBatchSigners + 1, 2); env(batch::outer(alice, seq, batchFee, tfAllOrNothing), batch::Inner(pay(alice, bob, XRP(10)), seq + 1), batch::Inner(pay(alice, bob, XRP(5)), seq + 2), - batch::Sig(bob, carol, alice, bob, carol, alice, bob, carol, alice, alice), + batch::Sig(std::vector(kMaxBatchSigners + 1, bob)), Ter(telENV_RPC_FAILED)); env.close(); } @@ -563,7 +598,7 @@ class Batch_test : public beast::unit_test::Suite env.close(); } - // temREDUNDANT: Batch: duplicate signer found + // temBAD_SIGNER: Batch: duplicate signer (caught by ascending order check) { auto const seq = env.seq(alice); auto const batchFee = batch::calcBatchFee(env, 2, 2); @@ -571,7 +606,7 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(alice, bob, XRP(10)), seq + 1), batch::Inner(pay(bob, alice, XRP(5)), env.seq(bob)), batch::Sig(bob, bob), - Ter(temREDUNDANT)); + Ter(temBAD_SIGNER)); env.close(); } @@ -611,7 +646,13 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(bob, alice, XRP(5)), bobSeq)); Serializer msg; - serializeBatch(msg, tfAllOrNothing, jt.stx->getBatchTransactionIDs()); + serializeBatch( + msg, + jt.stx->getAccountID(sfAccount), + jt.stx->getSeqValue(), + tfAllOrNothing, + jt.stx->getBatchTransactionIDs()); + finishMultiSigningData(bob.id(), msg); auto const sig = xrpl::sign(bob.pk(), bob.sk(), msg.slice()); jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfAccount.jsonName] = bob.human(); @@ -620,7 +661,7 @@ class Batch_test : public beast::unit_test::Suite jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] = strHex(Slice{sig.data(), sig.size()}); - env(jt.jv, Ter(temBAD_SIGNATURE)); + env(jt.jv, Ter(telENV_RPC_FAILED)); env.close(); } @@ -649,7 +690,7 @@ class Batch_test : public beast::unit_test::Suite //---------------------------------------------------------------------- // preclaim - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -890,7 +931,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -971,6 +1012,46 @@ class Batch_test : public beast::unit_test::Suite env(jt.jv, batch::Sig(bob), Ter(telENV_RPC_FAILED)); env.close(); } + + // Inner OfferCreate with MPT TakerPays. Valid under featureMPTokensV2. + { + MPTIssue const issue(makeMptID(1, alice)); + STAmount const mptAmt{issue, UINT64_C(100)}; + + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const seq = env.seq(alice); + + json::Value tx1; + tx1[jss::TransactionType] = jss::OfferCreate; + tx1[jss::Account] = alice.human(); + tx1[jss::TakerPays] = mptAmt.getJson(JsonOptions::Values::None); + tx1[jss::TakerGets] = XRP(10).value().getJson(JsonOptions::Values::None); + + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(tx1, seq + 1), + batch::Inner(pay(alice, bob, XRP(1)), seq + 2), + Ter(tesSUCCESS)); + env.close(); + } + + // Invalid: inner txn with invalid memo (non-URL-safe MemoType) + { + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const seq = env.seq(alice); + + auto tx1 = pay(alice, bob, XRP(10)); + auto& ma = tx1["Memos"]; + auto& mi = ma[ma.size()]; + auto& m = mi["Memo"]; + m["MemoType"] = strHex(std::string("\x01\x02\x03", 3)); + m["MemoData"] = strHex(std::string("test")); + + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(tx1, seq + 1), + batch::Inner(pay(alice, bob, XRP(1)), seq + 2), + Ter(telENV_RPC_FAILED)); + env.close(); + } } void @@ -981,7 +1062,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1239,7 +1320,7 @@ class Batch_test : public beast::unit_test::Suite // Bad Fee Without Signer { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1261,7 +1342,7 @@ class Batch_test : public beast::unit_test::Suite // Bad Fee With MultiSign { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1288,7 +1369,7 @@ class Batch_test : public beast::unit_test::Suite // Bad Fee With MultiSign + BatchSigners { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1317,7 +1398,7 @@ class Batch_test : public beast::unit_test::Suite // Bad Fee With MultiSign + BatchSigners.Signers { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1349,7 +1430,7 @@ class Batch_test : public beast::unit_test::Suite // Bad Fee With BatchSigners { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1373,7 +1454,7 @@ class Batch_test : public beast::unit_test::Suite // Bad Fee Dynamic Fee Calculation { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1412,7 +1493,7 @@ class Batch_test : public beast::unit_test::Suite // telENV_RPC_FAILED: Batch: txns array exceeds 8 entries. { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1437,7 +1518,7 @@ class Batch_test : public beast::unit_test::Suite // temARRAY_TOO_LARGE: Batch: txns array exceeds 8 entries. { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1465,41 +1546,52 @@ class Batch_test : public beast::unit_test::Suite }); } - // telENV_RPC_FAILED: Batch: signers array exceeds 8 entries. + // Regression: the relay-boundary local check (isBatchRawTransactionOkay) + // caps the signers array at kMaxBatchSigners, matching Batch::preflight - + // not kMaxBatchTxCount. A batch with more than kMaxBatchTxCount but at + // most kMaxBatchSigners signers must get past local checks and reach + // signer validation. Before the cap was aligned it was wrongly rejected + // at the boundary (telENV_RPC_FAILED) before preflight ran; now it + // reaches preflightSigValidated and fails there as extra signers + // (temBAD_SIGNER) rather than being dropped pre-engine. { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); env.close(); + // Over kMaxBatchTxCount (8) but within kMaxBatchSigners (24). + std::size_t const signerCount = kMaxBatchTxCount + 1; + auto const aliceSeq = env.seq(alice); - auto const batchFee = batch::calcBatchFee(env, 9, 2); + auto const batchFee = batch::calcBatchFee(env, signerCount, 2); env(batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 1), batch::Inner(pay(alice, bob, XRP(5)), aliceSeq + 2), - batch::Sig(bob, bob, bob, bob, bob, bob, bob, bob, bob, bob), - Ter(telENV_RPC_FAILED)); + batch::Sig(std::vector(signerCount, bob)), + Ter(temBAD_SIGNER)); env.close(); } - // temARRAY_TOO_LARGE: Batch: signers array exceeds 8 entries. + // temARRAY_TOO_LARGE: Batch preflight: signers array exceeds + // kMaxBatchSigners entries. { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); env.close(); - auto const batchFee = batch::calcBatchFee(env, 0, 9); + auto const batchFee = batch::calcBatchFee(env, kMaxBatchSigners + 1, 2); auto const aliceSeq = env.seq(alice); auto jt = env.jtnofill( batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 1), batch::Inner(pay(alice, bob, XRP(5)), aliceSeq + 2), - batch::Sig(bob, bob, bob, bob, bob, bob, bob, bob, bob, bob)); + batch::Sig(std::vector(kMaxBatchSigners + 1, bob))); env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal j) { auto const result = xrpl::apply(env.app(), view, *jt.stx, TapNone, j); @@ -1517,7 +1609,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1677,7 +1769,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1966,7 +2058,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2267,7 +2359,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2538,22 +2630,16 @@ class Batch_test : public beast::unit_test::Suite void doTestInnerSubmitRPC(FeatureBitset features, bool withBatch) { - bool const withInnerSigFix = features[fixBatchInnerSigs]; + std::string const testName = + std::string("inner submit rpc: batch ") + (withBatch ? "enabled" : "disabled") + ": "; - std::string const testName = [&]() { - std::stringstream ss; - ss << "inner submit rpc: batch " << (withBatch ? "enabled" : "disabled") - << ", inner sig fix: " << (withInnerSigFix ? "enabled" : "disabled") << ": "; - return ss.str(); - }(); - - auto const amend = withBatch ? features : features - featureBatch; + auto const amend = withBatch ? features : features - featureBatchV1_1; using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, amend}; - if (!BEAST_EXPECT(amend[featureBatch] == withBatch)) + Env env{*this, amend}; + if (!BEAST_EXPECT(amend[featureBatchV1_1] == withBatch)) return; auto const alice = Account("alice"); @@ -2562,37 +2648,20 @@ class Batch_test : public beast::unit_test::Suite env.fund(XRP(10000), alice, bob); env.close(); - auto submitAndValidate = [&](std::string caseName, - Slice const& slice, - int line, - std::optional expectedEnabled = std::nullopt, - std::optional expectedDisabled = std::nullopt, - bool expectInvalidFlag = false) { - testcase << testName << caseName - << (expectInvalidFlag ? " - Expected to reach tx engine!" : ""); + // Any transaction carrying tfInnerBatchTxn is rejected in checkValidity() + // before it reaches the tx engine, regardless of its signing fields or + // whether the amendment is enabled. + auto submitAndValidate = [&](std::string caseName, Slice const& slice, int line) { + testcase << testName << caseName; auto const jrr = env.rpc("submit", strHex(slice))[jss::result]; - auto const expected = withBatch - ? expectedEnabled.value_or( - "fails local checks: Malformed: Invalid inner batch " - "transaction.") - : expectedDisabled.value_or("fails local checks: Empty SigningPubKey."); - if (expectInvalidFlag) - { - expect( - jrr[jss::status] == "success" && jrr[jss::engine_result] == "temINVALID_FLAG", - pretty(jrr), - __FILE__, - line); - } - else - { - expect( - jrr[jss::status] == "error" && jrr[jss::error] == "invalidTransaction" && - jrr[jss::error_exception] == expected, - pretty(jrr), - __FILE__, - line); - } + expect( + jrr[jss::status] == "error" && jrr[jss::error] == "invalidTransaction" && + jrr[jss::error_exception] == + "fails local checks: Batch inner transactions are never " + "considered validly signed.", + pretty(jrr), + __FILE__, + line); env.close(); }; @@ -2621,12 +2690,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "SigningPubKey set", - s.slice(), - __LINE__, - std::nullopt, - "fails local checks: Invalid signature."); + submitAndValidate("SigningPubKey set", s.slice(), __LINE__); } // Invalid RPC Submission: Signers @@ -2640,12 +2704,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "Signers set", - s.slice(), - __LINE__, - std::nullopt, - "fails local checks: Invalid Signers array size."); + submitAndValidate("Signers set", s.slice(), __LINE__); } { @@ -2656,8 +2715,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", jt.jv); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "Fully signed", s.slice(), __LINE__, std::nullopt, std::nullopt, !withBatch); + submitAndValidate("Fully signed", s.slice(), __LINE__); } // Invalid RPC Submission: tfInnerBatchTxn @@ -2670,13 +2728,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "No signing fields set", - s.slice(), - __LINE__, - "fails local checks: Empty SigningPubKey.", - "fails local checks: Empty SigningPubKey.", - withBatch && !withInnerSigFix); + submitAndValidate("No signing fields set", s.slice(), __LINE__); } // Invalid RPC Submission: tfInnerBatchTxn pseudo-transaction @@ -2687,7 +2739,7 @@ class Batch_test : public beast::unit_test::Suite { STTx const amendTx(ttAMENDMENT, [seq = env.closed()->header().seq + 1](auto& obj) { obj.setAccountID(sfAccount, AccountID()); - obj.setFieldH256(sfAmendment, fixBatchInnerSigs); + obj.setFieldH256(sfAmendment, featureBatchV1_1); obj.setFieldU32(sfLedgerSequence, seq); obj.setFieldU32(sfFlags, tfInnerBatchTxn); }); @@ -2695,13 +2747,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "Pseudo-transaction", - s.slice(), - __LINE__, - withInnerSigFix ? "fails local checks: Empty SigningPubKey." - : "fails local checks: Cannot submit pseudo transactions.", - "fails local checks: Empty SigningPubKey."); + submitAndValidate("Pseudo-transaction", s.slice(), __LINE__); } } @@ -2722,7 +2768,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2771,6 +2817,93 @@ class Batch_test : public beast::unit_test::Suite // Alice pays XRP & Fee; Bob receives XRP BEAST_EXPECT(env.balance(alice) == preAlice - XRP(1000) - batchFee); BEAST_EXPECT(env.balance(bob) == XRP(1000)); + + // An inner partial payment must never fund a new account. With + // featureBatchV1_1 the partial-payment-to-create check runs on the + // batch's closed view and rejects it (tefNO_DST_PARTIAL). A USD->XRP path is + // set up so that, absent the check, the partial payment would deliver + // XRP and create carol; the check must block exactly that. A + // cross-currency shape (IOU SendMax, XRP Amount) also lets the inner + // pass preflight and reach the preclaim check. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const gw = Account("gw"); + auto const carol = Account("carol"); // never funded + auto const usd = gw["USD"]; + env.fund(XRP(10000), alice, gw); + env.close(); + env.memoize(carol); + + env(trust(alice, usd(100000))); + env(pay(gw, alice, usd(10000))); + env(offer(gw, usd(2000), XRP(2000))); // USD -> XRP liquidity + env.close(); + + auto const seq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, 0, 2); + + auto pp = pay(alice, carol, XRP(1000)); + pp[jss::SendMax] = usd(2000).value().getJson(JsonOptions::Values::None); + pp[jss::Flags] = tfPartialPayment; + + env(batch::outer(alice, seq, batchFee, tfIndependent), + batch::Inner(pp, seq + 1), + batch::Inner(noop(alice), seq + 2), + Ter(tesSUCCESS)); + env.close(); + + // carol was not created despite the available path. + BEAST_EXPECT(!env.le(carol)); + } + } + + void + testCheckAllSignatures(FeatureBitset features) + { + testcase("check all signatures"); + + using namespace test::jtx; + using namespace std::literals; + + // Verifies that checkBatchSign validates all signers even when an + // unfunded account (signed with its master key) appears first in the + // sorted signer list. A funded account with an invalid signature must + // still be rejected with tefBAD_AUTH. + + Env env{*this, features}; + + auto const alice = Account("alice"); + // "aaa" sorts before other accounts alphabetically, ensuring the + // unfunded account is checked first in the sorted signer list + auto const unfunded = Account("aaa"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, carol); + env.close(); + + // Verify sort order: unfunded.id() < carol.id() + BEAST_EXPECT(unfunded.id() < carol.id()); + + auto const seq = env.seq(alice); + auto const ledSeq = env.current()->seq(); + auto const batchFee = batch::calcBatchFee(env, 2, 3); + + // The batch includes: + // 1. alice pays unfunded (to create unfunded's account) + // 2. unfunded does a noop (signed by unfunded's master key - valid) + // 3. carol pays alice (signed by alice's key - INVALID since alice is + // not carol's regular key) + // + // checkBatchSign must validate all signers regardless of order. + // This must fail with tefBAD_AUTH. + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, unfunded, XRP(100)), seq + 1), + batch::Inner(noop(unfunded), ledSeq), + batch::Inner(pay(carol, alice, XRP(1000)), env.seq(carol)), + batch::Sig(unfunded, Reg{carol, alice}), + Ter(tefBAD_AUTH)); + env.close(); } void @@ -2781,7 +2914,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2845,7 +2978,7 @@ class Batch_test : public beast::unit_test::Suite // tfIndependent: account delete success { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2897,7 +3030,7 @@ class Batch_test : public beast::unit_test::Suite // tfIndependent: account delete fails { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2957,7 +3090,7 @@ class Batch_test : public beast::unit_test::Suite // tfAllOrNothing: account delete fails { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3009,7 +3142,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; - test::jtx::Env env{*this, features}; + Env env{*this, features}; Account const issuer{"issuer"}; // For simplicity, lender will be the sole actor for the vault & @@ -3186,7 +3319,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3322,7 +3455,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3396,7 +3529,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3470,7 +3603,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3531,7 +3664,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3591,7 +3724,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3666,7 +3799,7 @@ class Batch_test : public beast::unit_test::Suite // overwritten by the payment in the batch transaction. Because the // terPRE_SEQ is outside of the batch this noop transaction will ge // reapplied in the following ledger - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -3729,7 +3862,7 @@ class Batch_test : public beast::unit_test::Suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3783,7 +3916,7 @@ class Batch_test : public beast::unit_test::Suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3831,7 +3964,7 @@ class Batch_test : public beast::unit_test::Suite // Outer Batch terPRE_SEQ { - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -3905,7 +4038,7 @@ class Batch_test : public beast::unit_test::Suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3964,7 +4097,7 @@ class Batch_test : public beast::unit_test::Suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -4037,7 +4170,7 @@ class Batch_test : public beast::unit_test::Suite // batch will run in the close ledger process. The batch will be // allied and then retry this transaction in the current ledger. - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -4100,7 +4233,7 @@ class Batch_test : public beast::unit_test::Suite // Create Object Before Batch Txn { - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -4163,7 +4296,7 @@ class Batch_test : public beast::unit_test::Suite // batch will run in the close ledger process. The batch will be // applied and then retry this transaction in the current ledger. - test::jtx::Env env{*this, features}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -4226,7 +4359,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -4265,7 +4398,7 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, features}; + Env env{*this, features}; XRPAmount const baseFee = env.current()->fees().base; auto const alice = Account("alice"); @@ -4359,9 +4492,12 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - // only outer batch transactions are counter towards the queue size + // A Batch is never queued. Under open-ledger congestion a Batch that + // pays only its base fee cannot apply directly and, unlike an ordinary + // transaction, is rejected outright rather than held in the queue. A + // Batch that pays the escalated open-ledger fee applies directly. { - test::jtx::Env env{ + Env env{ *this, makeSmallQueueConfig({{Keys::kMinimumTxnInLedgerStandalone, "2"}}), features, @@ -4378,7 +4514,7 @@ class Batch_test : public beast::unit_test::Suite env.fund(XRP(10000), noripple(carol)); env.close(env.now() + 5s, 10000ms); - // Fill the ledger + // Fill the open ledger so escalation is active. env(noop(alice)); env(noop(alice)); env(noop(alice)); @@ -4391,33 +4527,28 @@ class Batch_test : public beast::unit_test::Suite auto const bobSeq = env.seq(bob); auto const batchFee = batch::calcBatchFee(env, 1, 2); - // Queue Batch - { - env(batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), - batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 1), - batch::Inner(pay(bob, alice, XRP(5)), bobSeq), - batch::Sig(bob), - Ter(terQUEUED)); - } + // Paying only the base fee, the Batch cannot enter the congested + // open ledger and is rejected rather than queued. + env(batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 1), + batch::Inner(pay(bob, alice, XRP(5)), bobSeq), + batch::Sig(bob), + Ter(telCAN_NOT_QUEUE)); - checkMetrics(*this, env, 2, std::nullopt, 3, 2); + // The Batch was not queued; only carol's transaction is queued. + checkMetrics(*this, env, 1, std::nullopt, 3, 2); - // Replace Queued Batch - { - env(batch::outer(alice, aliceSeq, openLedgerFee(env, batchFee), tfAllOrNothing), - batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 1), - batch::Inner(pay(bob, alice, XRP(5)), bobSeq), - batch::Sig(bob), - Ter(tesSUCCESS)); - env.close(); - } - - checkMetrics(*this, env, 0, 12, 1, 6); + // Paying the escalated open-ledger fee, the Batch applies directly. + env(batch::outer(alice, aliceSeq, openLedgerFee(env, batchFee), tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 1), + batch::Inner(pay(bob, alice, XRP(5)), bobSeq), + batch::Sig(bob), + Ter(tesSUCCESS)); } // inner batch transactions are counter towards the ledger tx count { - test::jtx::Env env{ + Env env{ *this, makeSmallQueueConfig({{Keys::kMinimumTxnInLedgerStandalone, "2"}}), features, @@ -4457,6 +4588,50 @@ class Batch_test : public beast::unit_test::Suite env(noop(carol), Ter(terQUEUED)); checkMetrics(*this, env, 1, std::nullopt, 3, 2); } + + // A Batch is never queued, so it also cannot sit behind the account's + // already-queued transactions: with a sequence gap it cannot apply + // directly and is rejected rather than queued. + { + Env env{ + *this, + makeSmallQueueConfig({{Keys::kMinimumTxnInLedgerStandalone, "2"}}), + features, + nullptr, + beast::Severity::Error}; + + auto alice = Account("alice"); + auto bob = Account("bob"); + + env.fund(XRP(10000), noripple(alice, bob)); + env.close(env.now() + 5s, 10000ms); + + // Fill the open ledger so subsequent transactions queue. + env(noop(alice)); + env(noop(alice)); + env(noop(alice)); + checkMetrics(*this, env, 0, std::nullopt, 3, 2); + + auto const aliceSeq = env.seq(alice); + + // Queue two normal transactions for alice. + env(noop(alice), Seq(aliceSeq + 0), Ter(terQUEUED)); + env(noop(alice), Seq(aliceSeq + 1), Ter(terQUEUED)); + checkMetrics(*this, env, 2, std::nullopt, 3, 2); + + // The Batch's sequence sits behind the queued transactions, so it + // cannot apply directly and is rejected rather than queued. + auto const bobSeq = env.seq(bob); + auto const batchFee = batch::calcBatchFee(env, 1, 2); + env(batch::outer(alice, aliceSeq + 2, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(10)), aliceSeq + 3), + batch::Inner(pay(bob, alice, XRP(5)), bobSeq), + batch::Sig(bob), + Ter(telCAN_NOT_QUEUE)); + + // The two queued transactions are untouched. + checkMetrics(*this, env, 2, std::nullopt, 3, 2); + } } void @@ -4508,6 +4683,376 @@ class Batch_test : public beast::unit_test::Suite } } + void + testBatchDelegateConsent(FeatureBitset features) + { + testcase("batch delegate consent"); + + using namespace test::jtx; + using namespace std::literals; + + // Delegate consent bypass. + // + // Alice delegates Payment to Bob. A Batch carries an inner Payment with + // Delegate=Bob, but Bob never signs the batch (and, because the inner + // Account == the outer Account, no BatchSigners are required at all). + // Batch waives the per-inner signature check, and preflightSigValidated + // derives required signers from sfAccount only -- it ignores sfDelegate + // -- so Bob's delegated authority is exercised without Bob's consent. + // + // SECURE EXPECTATION: the delegate (Bob) must authorize the inner txn, + // so the batch must be rejected (temBAD_SIGNER) when Bob does not sign. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const mallory = Account("mallory"); + env.fund(XRP(10000), alice, bob, mallory); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + auto const preMallory = env.balance(mallory); + + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const seq = env.seq(alice); + + auto inner = batch::Inner(pay(alice, mallory, XRP(1000)), seq + 1); + inner[jss::Delegate] = bob.human(); + + // Bob is NOT among the batch signers; he never consents. + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + inner, + batch::Inner(pay(alice, mallory, XRP(1)), seq + 2), + Ter(temBAD_SIGNER)); + env.close(); + + // No funds should have moved to Mallory. + BEAST_EXPECT(env.balance(mallory) == preMallory); + } + + // Self-grant attribution forgery. + // + // With no pre-existing delegation, Alice places DelegateSet(authorize + // Bob) and a Delegate=Bob action in the SAME batch. The second inner + // reads the delegation created by the first from the batch's running + // view, and Bob never signs -- manufacturing on-chain attribution of + // the action to Bob without his consent. + // + // SECURE EXPECTATION: rejected (temBAD_SIGNER) -- Bob must consent, and + // a grant cannot be created and exercised within the same atomic batch. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const mallory = Account("mallory"); + env.fund(XRP(10000), alice, bob, mallory); + env.close(); + + auto const preMallory = env.balance(mallory); + + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const seq = env.seq(alice); + + auto inner = batch::Inner(pay(alice, mallory, XRP(1000)), seq + 2); + inner[jss::Delegate] = bob.human(); + + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(delegate::set(alice, bob, {"Payment"}), seq + 1), + inner, + Ter(temBAD_SIGNER)); + env.close(); + + BEAST_EXPECT(env.balance(mallory) == preMallory); + } + + // Legitimate counterpart: the same atomic grant-and-use is allowed when + // the delegate co-signs the batch -- consent is present, so it succeeds. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const mallory = Account("mallory"); + env.fund(XRP(10000), alice, bob, mallory); + env.close(); + + auto const preMallory = env.balance(mallory); + + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const seq = env.seq(alice); + + auto inner = batch::Inner(pay(alice, mallory, XRP(1000)), seq + 2); + inner[jss::Delegate] = bob.human(); + + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(delegate::set(alice, bob, {"Payment"}), seq + 1), + inner, + batch::Sig(bob), + Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT(env.balance(mallory) == preMallory + XRP(1000)); + } + + // Multi-account: a delegated inner from a non-outer account also + // requires the delegate's signature (not the account holder's). + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + env(delegate::set(bob, carol, {"Payment"})); + env.close(); + + auto const preAlice = env.balance(alice); + + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const aliceSeq = env.seq(alice); + auto const bobSeq = env.seq(bob); + + auto inner = batch::Inner(pay(bob, alice, XRP(1)), bobSeq); + inner[jss::Delegate] = carol.human(); + + // Carol (the delegate) does not sign. + env(batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), + inner, + batch::Inner(pay(alice, bob, XRP(2)), aliceSeq + 1), + Ter(temBAD_SIGNER)); + env.close(); + + BEAST_EXPECT(env.balance(alice) == preAlice); + } + + // Wrong signer: a batch signature from someone other than the named + // delegate does not satisfy the requirement. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const mallory = Account("mallory"); + env.fund(XRP(10000), alice, bob, carol, mallory); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + auto const preMallory = env.balance(mallory); + + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const seq = env.seq(alice); + + auto inner = batch::Inner(pay(alice, mallory, XRP(1000)), seq + 1); + inner[jss::Delegate] = bob.human(); + + // Carol signs instead of the named delegate Bob. + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + inner, + batch::Inner(pay(alice, mallory, XRP(1)), seq + 2), + batch::Sig(carol), + Ter(temBAD_SIGNER)); + env.close(); + + BEAST_EXPECT(env.balance(mallory) == preMallory); + } + + // Delegate is the outer account: the outer signature already provides + // the delegate's consent, so no BatchSigners are required. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(10000), alice, bob); + env.close(); + + // Bob delegates Payment to Alice, who is also the batch submitter. + env(delegate::set(bob, alice, {"Payment"})); + env.close(); + + auto const preBob = env.balance(bob); + + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const aliceSeq = env.seq(alice); + auto const bobSeq = env.seq(bob); + + auto inner = batch::Inner(pay(bob, alice, XRP(1)), bobSeq); + inner[jss::Delegate] = alice.human(); + + env(batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), + inner, + batch::Inner(pay(alice, bob, XRP(2)), aliceSeq + 1), + Ter(tesSUCCESS)); + env.close(); + + // Net: Bob sends 1 to Alice, receives 2 from Alice. + BEAST_EXPECT(env.balance(bob) == preBob + XRP(1)); + } + + // Principal signs instead of the delegate. Bob delegates Payment to + // Carol and an inner pays from Bob with Delegate=Carol, so Carol -- not + // Bob -- is the required batch signer. Bob's own signature (the account + // holder) does not satisfy the delegate-consent requirement. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + env(delegate::set(bob, carol, {"Payment"})); + env.close(); + + auto const preAlice = env.balance(alice); + + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const aliceSeq = env.seq(alice); + auto const bobSeq = env.seq(bob); + + auto inner = batch::Inner(pay(bob, alice, XRP(1)), bobSeq); + inner[jss::Delegate] = carol.human(); + + // Bob (the principal) signs in place of Carol (the delegate). + env(batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), + inner, + batch::Inner(pay(alice, bob, XRP(2)), aliceSeq + 1), + batch::Sig(bob), + Ter(temBAD_SIGNER)); + env.close(); + + BEAST_EXPECT(env.balance(alice) == preAlice); + } + + // Control for the revocation case below: identical setup, but inner 1 + // is a benign payment instead of a revocation. With Alice's delegation + // intact, the delegated inner 2 succeeds -- proving the failure in the + // revocation case is caused by the revocation, not the setup. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const mallory = Account("mallory"); + env.fund(XRP(10000), alice, bob, mallory); + env.close(); + + env(delegate::set(bob, alice, {"Payment"})); + env.close(); + + auto const preMallory = env.balance(mallory); + + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const seq = env.seq(bob); + + auto inner2 = batch::Inner(pay(bob, mallory, XRP(1000)), seq + 2); + inner2[jss::Delegate] = alice.human(); + + // Inner 1 is Bob's own payment (no revocation); Alice co-signs for + // the delegated inner 2. + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(bob, seq, batchFee, tfIndependent), + batch::Inner(pay(bob, alice, XRP(1)), seq + 1), + inner2, + batch::Sig(alice)); + env.close(); + + std::vector const testCases = { + {.index = 0, + .txType = "Batch", + .result = "tesSUCCESS", + .txHash = batchID, + .batchID = std::nullopt}, + {.index = 1, + .txType = "Payment", + .result = "tesSUCCESS", + .txHash = txIDs[0], + .batchID = batchID}, + {.index = 2, + .txType = "Payment", + .result = "tesSUCCESS", + .txHash = txIDs[1], + .batchID = batchID}, + }; + validateClosedLedger(env, testCases); + + // The delegated payment applied: Mallory received the funds. + BEAST_EXPECT(env.balance(mallory) == preMallory + XRP(1000)); + } + + // Revocation within the same batch. Bob grants Alice Payment permission + // beforehand; an earlier inner revokes it before a later delegated + // inner tries to use it. Alice must still co-sign (delegate consent is + // derived from the static tx), but the revoked permission makes the + // delegated inner fail at apply time -- a grant cannot be used after it + // is removed earlier in the same batch. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const mallory = Account("mallory"); + env.fund(XRP(10000), alice, bob, mallory); + env.close(); + + env(delegate::set(bob, alice, {"Payment"})); + env.close(); + + auto const preMallory = env.balance(mallory); + + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const seq = env.seq(bob); + + // Inner 2: Alice acts as Bob's delegate, so Alice is the required + // batch signer. + auto inner2 = batch::Inner(pay(bob, mallory, XRP(1000)), seq + 2); + inner2[jss::Delegate] = alice.human(); + + // tfIndependent: inner 1 (the revocation) applies; inner 2 then + // fails for lack of permission without reverting inner 1. An empty + // permission list deletes the Delegate object. + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(bob, seq, batchFee, tfIndependent), + batch::Inner(delegate::set(bob, alice, {}), seq + 1), + inner2, + batch::Sig(alice)); + env.close(); + + std::vector const testCases = { + {.index = 0, + .txType = "Batch", + .result = "tesSUCCESS", + .txHash = batchID, + .batchID = std::nullopt}, + {.index = 1, + .txType = "DelegateSet", + .result = "tesSUCCESS", + .txHash = txIDs[0], + .batchID = batchID}, + // inner 2 fails: Alice's permission was revoked in inner 1. + }; + validateClosedLedger(env, testCases); + + // The delegated payment never applied. + BEAST_EXPECT(env.balance(mallory) == preMallory); + BEAST_EXPECT(env.rpc("tx", txIDs[1])[jss::result][jss::error] == "txnNotFound"); + } + } + void testBatchDelegate(FeatureBitset features) { @@ -4518,7 +5063,7 @@ class Batch_test : public beast::unit_test::Suite // delegated non atomic inner { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -4533,17 +5078,20 @@ class Batch_test : public beast::unit_test::Suite auto const preAlice = env.balance(alice); auto const preBob = env.balance(bob); - auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const batchFee = batch::calcBatchFee(env, 1, 2); auto const seq = env.seq(alice); auto tx = batch::Inner(pay(alice, bob, XRP(1)), seq + 1); tx[jss::Delegate] = bob.human(); + // The delegate (Bob) authorizes the delegated inner, so Bob must + // provide the batch signature. auto const [txIDs, batchID] = submitBatch( env, tesSUCCESS, batch::outer(alice, seq, batchFee, tfAllOrNothing), tx, - batch::Inner(pay(alice, bob, XRP(2)), seq + 2)); + batch::Inner(pay(alice, bob, XRP(2)), seq + 2), + batch::Sig(bob)); env.close(); std::vector const testCases = { @@ -4575,7 +5123,7 @@ class Batch_test : public beast::unit_test::Suite // delegated atomic inner { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -4598,13 +5146,15 @@ class Batch_test : public beast::unit_test::Suite auto tx = batch::Inner(pay(bob, alice, XRP(1)), bobSeq); tx[jss::Delegate] = carol.human(); + // Carol is the delegate authorizing the inner txn on Bob's behalf, + // so Carol -- not Bob -- must provide the batch signature. auto const [txIDs, batchID] = submitBatch( env, tesSUCCESS, batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), tx, batch::Inner(pay(alice, bob, XRP(2)), aliceSeq + 1), - batch::Sig(bob)); + batch::Sig(carol)); env.close(); std::vector const testCases = { @@ -4639,7 +5189,7 @@ class Batch_test : public beast::unit_test::Suite // this also makes sure tfInnerBatchTxn won't block delegated AccountSet // with granular permission { - test::jtx::Env env{*this, features}; + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -4654,19 +5204,22 @@ class Batch_test : public beast::unit_test::Suite auto const preAlice = env.balance(alice); auto const preBob = env.balance(bob); - auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const batchFee = batch::calcBatchFee(env, 1, 2); auto const seq = env.seq(alice); auto tx = batch::Inner(noop(alice), seq + 1); std::string const domain = "example.com"; tx[sfDomain.jsonName] = strHex(domain); tx[jss::Delegate] = bob.human(); + // Bob is the delegate authorizing the inner AccountSet, so Bob must + // provide the batch signature. auto const [txIDs, batchID] = submitBatch( env, tesSUCCESS, batch::outer(alice, seq, batchFee, tfAllOrNothing), tx, - batch::Inner(pay(alice, bob, XRP(2)), seq + 2)); + batch::Inner(pay(alice, bob, XRP(2)), seq + 2), + batch::Sig(bob)); env.close(); std::vector const testCases = { @@ -4700,7 +5253,7 @@ class Batch_test : public beast::unit_test::Suite // this also makes sure tfInnerBatchTxn won't block delegated // MPTokenIssuanceSet with granular permission { - test::jtx::Env env{*this, features}; + Env env{*this, features}; Account const alice{"alice"}; Account const bob{"bob"}; env.fund(XRP(100000), alice, bob); @@ -4717,7 +5270,7 @@ class Batch_test : public beast::unit_test::Suite env.close(); auto const seq = env.seq(alice); - auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const batchFee = batch::calcBatchFee(env, 1, 2); json::Value jv1; jv1[sfTransactionType] = jss::MPTokenIssuanceSet; @@ -4735,12 +5288,15 @@ class Batch_test : public beast::unit_test::Suite jv2[sfMPTokenIssuanceID] = to_string(mptID); jv2[sfFlags] = tfMPTUnlock; + // Both inners are delegated to Bob, so Bob must provide the batch + // signature (one signer covers both). auto const [txIDs, batchID] = submitBatch( env, tesSUCCESS, batch::outer(alice, seq, batchFee, tfAllOrNothing), batch::Inner(jv1, seq + 1), - batch::Inner(jv2, seq + 2)); + batch::Inner(jv2, seq + 2), + batch::Sig(bob)); env.close(); std::vector const testCases = { @@ -4767,7 +5323,7 @@ class Batch_test : public beast::unit_test::Suite // this also makes sure tfInnerBatchTxn won't block delegated TrustSet // with granular permission { - test::jtx::Env env{*this, features}; + Env env{*this, features}; Account const gw{"gw"}; Account const alice{"alice"}; Account const bob{"bob"}; @@ -4781,19 +5337,22 @@ class Batch_test : public beast::unit_test::Suite env.close(); auto const seq = env.seq(gw); - auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const batchFee = batch::calcBatchFee(env, 1, 2); auto jv1 = trust(gw, gw["USD"](0), alice, tfSetfAuth); jv1[sfDelegate] = bob.human(); auto jv2 = trust(gw, gw["USD"](0), alice, tfSetFreeze); jv2[sfDelegate] = bob.human(); + // Both inners are delegated to Bob, so Bob must provide the batch + // signature. auto const [txIDs, batchID] = submitBatch( env, tesSUCCESS, batch::outer(gw, seq, batchFee, tfAllOrNothing), batch::Inner(jv1, seq + 1), - batch::Inner(jv2, seq + 2)); + batch::Inner(jv2, seq + 2), + batch::Sig(bob)); env.close(); std::vector const testCases = { @@ -4818,7 +5377,7 @@ class Batch_test : public beast::unit_test::Suite // inner transaction not authorized by the delegating account. { - test::jtx::Env env{*this, features}; + Env env{*this, features}; Account const gw{"gw"}; Account const alice{"alice"}; Account const bob{"bob"}; @@ -4832,20 +5391,23 @@ class Batch_test : public beast::unit_test::Suite env.close(); auto const seq = env.seq(gw); - auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const batchFee = batch::calcBatchFee(env, 1, 2); auto jv1 = trust(gw, gw["USD"](0), alice, tfSetFreeze); jv1[sfDelegate] = bob.human(); auto jv2 = trust(gw, gw["USD"](0), alice, tfClearFreeze); jv2[sfDelegate] = bob.human(); + // Both inners are delegated to Bob, so Bob must provide the batch + // signature; jv2 still fails preclaim for lack of permission. auto const [txIDs, batchID] = submitBatch( env, tesSUCCESS, batch::outer(gw, seq, batchFee, tfIndependent), batch::Inner(jv1, seq + 1), // terNO_DELEGATE_PERMISSION: not authorized to clear freeze - batch::Inner(jv2, seq + 2)); + batch::Inner(jv2, seq + 2), + batch::Sig(bob)); env.close(); std::vector const testCases = { @@ -5004,7 +5566,7 @@ class Batch_test : public beast::unit_test::Suite batch::outer(alice, seq, batchFee, tfAllOrNothing), batch::Inner(pay(alice, bob, XRP(10)), seq + 1), batch::Inner(pay(alice, bob, XRP(5)), seq + 2), - batch::Sig(bob, carol, alice, bob, carol, alice, bob, carol, alice, alice)); + batch::Sig(std::vector(kMaxBatchSigners + 1, bob))); XRPAmount const txBaseFee = getBaseFee(jtx); BEAST_EXPECT(txBaseFee == XRPAmount(kInitialXrp)); } @@ -5022,6 +5584,304 @@ class Batch_test : public beast::unit_test::Suite } } + void + testStandaloneInnerBatchFlag(FeatureBitset features) + { + testcase("standalone tx with tfInnerBatchTxn rejected"); + + using namespace test::jtx; + using namespace std::literals; + + // A standalone Payment with tfInnerBatchTxn must be rejected. + // Without proper guards this would bypass signature verification + // in preflight2. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(10000), alice, bob); + env.close(); + + // Submit a normal Payment with tfInnerBatchTxn flag. + // preflight1 must reject with temINVALID_INNER_BATCH because + // the flag is set but no parentBatchId exists. + env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), Ter(telENV_RPC_FAILED)); + env.close(); + + // Verify via direct apply path (bypassing RPC layer) + env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal j) { + // Construct a Payment STTx with tfInnerBatchTxn, + // empty signing pub key, and no signature — mimicking + // what an attacker would send to skip sig verification. + STTx const stx = STTx(ttPAYMENT, [&](auto& obj) { + obj.setAccountID(sfAccount, alice.id()); + obj.setAccountID(sfDestination, bob.id()); + obj.setFieldAmount(sfAmount, XRP(1)); + obj.setFieldAmount(sfFee, XRP(0)); + obj.setFieldU32(sfSequence, env.seq(alice)); + obj.setFieldU32(sfFlags, tfInnerBatchTxn); + }); + + auto const result = xrpl::apply(env.app(), view, stx, TapNone, j); + // Must NOT be applied — signature was never checked + BEAST_EXPECT(!result.applied); + return false; + }); + } + } + + void + testOuterBinding(FeatureBitset features) + { + testcase("outer binding"); + + using namespace test::jtx; + + // Signatures captured from one outer account cannot be replayed + // under a different outer account. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const eve = Account("eve"); + env.fund(XRP(10000), alice, bob, carol, eve); + env.close(); + + auto const preCarol = env.balance(carol); + auto const bobSeq = env.seq(bob); + auto const carolSeq = env.seq(carol); + + auto const aliceSeq = env.seq(alice); + auto const batchFee1 = batch::calcBatchFee(env, 2, 2); + auto jt1 = env.jt( + batch::outer(alice, aliceSeq, batchFee1, tfOnlyOne), + batch::Inner(pay(bob, alice, XRP(100)), bobSeq), + batch::Inner(pay(carol, alice, XRP(50)), carolSeq), + batch::Sig(bob, carol)); + + auto const capturedSigners = jt1.jv[sfBatchSigners.jsonName]; + + env(jt1, Ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(env.seq(bob) == bobSeq + 1); + BEAST_EXPECT(env.seq(carol) == carolSeq); + + auto const batchFee2 = batch::calcBatchFee(env, 2, 2); + auto jt2 = env.jtnofill( + batch::outer(eve, env.seq(eve), batchFee2, tfOnlyOne), + batch::Inner(pay(bob, alice, XRP(100)), bobSeq), + batch::Inner(pay(carol, alice, XRP(50)), carolSeq)); + + jt2.jv[sfBatchSigners.jsonName] = capturedSigners; + env(jt2.jv, Ter(telENV_RPC_FAILED)); + env.close(); + + BEAST_EXPECT(env.seq(carol) == carolSeq); + BEAST_EXPECT(env.balance(carol) == preCarol); + } + + // Signatures are bound to the outer sequence; replaying them + // at a higher sequence must fail. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + auto const preCarol = env.balance(carol); + auto const bobSeq = env.seq(bob); + auto const carolSeq = env.seq(carol); + + auto const aliceSeq1 = env.seq(alice); + auto const batchFee1 = batch::calcBatchFee(env, 2, 2); + auto jt1 = env.jt( + batch::outer(alice, aliceSeq1, batchFee1, tfOnlyOne), + batch::Inner(pay(bob, alice, XRP(500)), bobSeq), + batch::Inner(pay(carol, alice, XRP(500)), carolSeq), + batch::Sig(bob, carol)); + + auto const capturedSigners = jt1.jv[sfBatchSigners.jsonName]; + + env(jt1, Ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(env.seq(bob) == bobSeq + 1); + BEAST_EXPECT(env.seq(carol) == carolSeq); + + auto const batchFee2 = batch::calcBatchFee(env, 2, 2); + auto jt2 = env.jtnofill( + batch::outer(alice, env.seq(alice), batchFee2, tfOnlyOne), + batch::Inner(pay(bob, alice, XRP(500)), bobSeq), + batch::Inner(pay(carol, alice, XRP(500)), carolSeq)); + + jt2.jv[sfBatchSigners.jsonName] = capturedSigners; + env(jt2.jv, Ter(telENV_RPC_FAILED)); + env.close(); + + BEAST_EXPECT(env.balance(carol) == preCarol); + BEAST_EXPECT(env.seq(carol) == carolSeq); + } + + // Multi-signed batch signer entries are bound to their account; + // reusing inner signatures under a different batch signer must fail. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const dave = Account("dave"); + auto const elsa = Account("elsa"); + env.fund(XRP(10000), alice, bob, carol, dave, elsa); + env.close(); + + env(signers(bob, 2, {{dave, 1}, {elsa, 1}})); + env.close(); + env(signers(carol, 2, {{dave, 1}, {elsa, 1}})); + env.close(); + + auto const seq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, 3, 2); + auto jt1 = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(10)), seq + 1), + batch::Inner(pay(bob, alice, XRP(5)), env.seq(bob)), + batch::Msig(bob, {dave, elsa}), + Ter(tesSUCCESS)); + + auto const bobSignerEntry = jt1.jv[sfBatchSigners.jsonName][0u]; + + env(jt1, Ter(tesSUCCESS)); + env.close(); + + auto const seq2 = env.seq(alice); + auto const batchFee2 = batch::calcBatchFee(env, 3, 2); + auto jt2 = env.jtnofill( + batch::outer(alice, seq2, batchFee2, tfAllOrNothing), + batch::Inner(pay(alice, carol, XRP(10)), seq2 + 1), + batch::Inner(pay(carol, alice, XRP(5)), env.seq(carol))); + + json::Value carolSigner; + carolSigner[sfBatchSigner.jsonName][jss::Account] = carol.human(); + carolSigner[sfBatchSigner.jsonName][jss::SigningPubKey] = ""; + carolSigner[sfBatchSigner.jsonName][sfSigners.jsonName] = + bobSignerEntry[sfBatchSigner.jsonName][sfSigners.jsonName]; + + jt2.jv[sfBatchSigners.jsonName][0u] = carolSigner; + env(jt2.jv, Ter(telENV_RPC_FAILED)); + env.close(); + } + } + + void + testUnsortedBatchSigners(FeatureBitset features) + { + testcase("unsorted batch signers"); + + using namespace test::jtx; + + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + auto const seq = env.seq(alice); + auto const bobSeq = env.seq(bob); + auto const carolSeq = env.seq(carol); + auto const batchFee = batch::calcBatchFee(env, 2, 2); + + auto jt = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(pay(bob, alice, XRP(10)), bobSeq), + batch::Inner(pay(carol, alice, XRP(5)), carolSeq), + batch::Sig(bob, carol)); + + auto const s0 = jt.jv[sfBatchSigners.jsonName][0u]; + auto const s1 = jt.jv[sfBatchSigners.jsonName][1u]; + jt.jv[sfBatchSigners.jsonName][0u] = s1; + jt.jv[sfBatchSigners.jsonName][1u] = s0; + + env(jt.jv, Ter(temBAD_SIGNER)); + env.close(); + } + + void + testBatchSigCache(FeatureBitset features) + { + testcase("batch signature caching"); + + using namespace test::jtx; + + // Mirrors apply.cpp's file-local kSfSiggood (the standard signature-good + // cache); batch signer sigs are now verified and cached alongside the + // outer signature in checkSign/checkValidity. + constexpr HashRouterFlags kSfSiggood = HashRouterFlags::PRIVATE2; + + // Valid batch: alice (outer) + an inner from bob, who co-signs. + auto buildValidBatch = [](Env& env) { + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const seq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, 1, 2); + return env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(1)), seq + 1), + batch::Inner(pay(bob, alice, XRP(2)), env.seq(bob)), + batch::Sig(bob)); + }; + + // WRITE: a valid batch records "good" on its tx id. + { + Env env{*this, features}; + env.fund(XRP(10000), Account("alice"), Account("bob")); + env.close(); + + auto jt = buildValidBatch(env); + auto const txid = jt.stx->getTransactionID(); + + BEAST_EXPECT(!any(env.app().getHashRouter().getFlags(txid) & kSfSiggood)); + env(jt, Ter(tesSUCCESS)); + BEAST_EXPECT(any(env.app().getHashRouter().getFlags(txid) & kSfSiggood)); + env.close(); + } + + // READ: corrupt only a signer's signature (outer sig + signer key + // untouched), so just the crypto would fail. Caught when uncached... + { + Env env{*this, features}; + env.fund(XRP(10000), Account("alice"), Account("bob")); + env.close(); + + auto jt = buildValidBatch(env); + jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] = + "00"; + env(jt.jv, Ter(telENV_RPC_FAILED)); + env.close(); + } + { + // ...but a planted "good" skips the crypto, so it applies. + Env env{*this, features}; + env.fund(XRP(10000), Account("alice"), Account("bob")); + env.close(); + + auto jt = buildValidBatch(env); + jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] = + "00"; + auto const txid = STTx{parse(jt.jv)}.getTransactionID(); + env.app().getHashRouter().setFlags(txid, kSfSiggood); + env(jt.jv, Ter(tesSUCCESS)); + env.close(); + } + } + void testWithFeats(FeatureBitset features) { @@ -5038,6 +5898,7 @@ class Batch_test : public beast::unit_test::Suite testIndependent(features); testInnerSubmitRPC(features); testAccountActivation(features); + testCheckAllSignatures(features); testAccountSet(features); testAccountDelete(features); testLoan(features); @@ -5053,8 +5914,13 @@ class Batch_test : public beast::unit_test::Suite testBatchTxQueue(features); testBatchNetworkOps(features); testBatchDelegate(features); + testBatchDelegateConsent(features); testValidateRPCResponse(features); testBatchCalculateBaseFee(features); + testStandaloneInnerBatchFlag(features); + testOuterBinding(features); + testUnsortedBatchSigners(features); + testBatchSigCache(features); } public: @@ -5064,7 +5930,6 @@ public: using namespace test::jtx; auto const sa = testableAmendments(); - testWithFeats(sa - fixBatchInnerSigs); testWithFeats(sa); } }; diff --git a/src/test/app/ConfidentialTransferExtended_test.cpp b/src/test/app/ConfidentialTransferExtended_test.cpp index 0aee7516c9..953325a6e9 100644 --- a/src/test/app/ConfidentialTransferExtended_test.cpp +++ b/src/test/app/ConfidentialTransferExtended_test.cpp @@ -1945,7 +1945,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env.close(); auto const bobSeq = env.seq(bob); - auto const batchFee = batch::calcConfidentialBatchFee(env, 0, 2); + auto const batchFee = batch::calcConfidentialBatchFee(env, 1, 2); // jv1: proof against spending balance 100 auto jv1 = mpt.sendJV({.account = bob, .dest = carol, .amt = 60}, bobSeq + 1); @@ -1957,6 +1957,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env(batch::outer(bob, bobSeq, batchFee, tfAllOrNothing), batch::Inner(jv1, bobSeq + 1), batch::Inner(jv2, bobSeq + 2), + batch::Sig(dave), Ter(tesSUCCESS)); env.close(); @@ -1981,7 +1982,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env.close(); auto const bobSeq = env.seq(bob); - auto const batchFee = batch::calcConfidentialBatchFee(env, 0, 2); + auto const batchFee = batch::calcConfidentialBatchFee(env, 1, 2); // jv1: proof against spending balance 100. auto jv1 = mpt.sendJV({.account = bob, .dest = carol, .amt = 40}, bobSeq + 1); @@ -1994,6 +1995,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env(batch::outer(bob, bobSeq, batchFee, tfAllOrNothing), batch::Inner(jv1, bobSeq + 1), batch::Inner(jv2, bobSeq + 2), + batch::Sig(dave), Ter(tesSUCCESS)); env.close(); @@ -2029,7 +2031,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase auto const bobSeq = env.seq(bob); auto const carolSeq = env.seq(carol); - auto const batchFee = batch::calcConfidentialBatchFee(env, 1, 2); + auto const batchFee = batch::calcConfidentialBatchFee(env, 2, 2); // jv1: direct send from carol (valid proof). auto const jv1 = mpt.sendJV({.account = carol, .dest = dave, .amt = 30}, carolSeq); @@ -2040,7 +2042,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env(batch::outer(bob, bobSeq, batchFee, tfAllOrNothing), batch::Inner(jv1, carolSeq), batch::Inner(jv2, bobSeq + 1), - batch::Sig(carol), + batch::Sig(carol, dave), Ter(tesSUCCESS)); env.close(); @@ -2066,7 +2068,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase // Bob does not grant dave any permissions. auto const bobSeq = env.seq(bob); auto const carolSeq = env.seq(carol); - auto const batchFee = batch::calcConfidentialBatchFee(env, 1, 2); + auto const batchFee = batch::calcConfidentialBatchFee(env, 2, 2); auto jv1 = mpt.sendJV({.account = bob, .dest = carol, .amt = 50}, bobSeq + 1); jv1[jss::Delegate] = dave.human(); @@ -2075,7 +2077,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env(batch::outer(bob, bobSeq, batchFee, tfIndependent), batch::Inner(jv1, bobSeq + 1), batch::Inner(jv2, carolSeq), - batch::Sig(carol), + batch::Sig(carol, dave), Ter(tesSUCCESS)); env.close(); @@ -2093,8 +2095,9 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase testcase("Test batch delegated send with delegate as outer account"); using namespace test::jtx; - // Dave has delegation permission, but the inner Account is bob. - // Without bob's BatchSigner, the batch is rejected. + // Dave holds bob's ConfidentialMPTSend delegation and is the outer batch + // signer, so dave's outer signature consents to the delegated inner. + // The batch applies. { Env env{*this, features}; Account const alice("alice"); @@ -2119,11 +2122,11 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase env(batch::outer(dave, daveSeq, batchFee, tfAllOrNothing), batch::Inner(jv1, bobSeq), batch::Inner(jv2, daveSeq + 1), - Ter(temBAD_SIGNER)); + Ter(tesSUCCESS)); env.close(); - BEAST_EXPECT(mpt.getDecryptedBalance(bob, MPTTester::holderEncryptedSpending) == 100); - BEAST_EXPECT(mpt.getDecryptedBalance(carol, MPTTester::holderEncryptedInbox) == 0); + BEAST_EXPECT(mpt.getDecryptedBalance(bob, MPTTester::holderEncryptedSpending) == 60); + BEAST_EXPECT(mpt.getDecryptedBalance(carol, MPTTester::holderEncryptedInbox) == 40); } // Dave submits a mixed batch: bob signs inner tx1, and @@ -2163,8 +2166,10 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase BEAST_EXPECT(mpt.getDecryptedBalance(carol, MPTTester::holderEncryptedInbox) == 70); } - // Verify the delegator Bob's BatchSigner does not bypass the missing delegation permission. - // The delegated inner send fails. + // The delegated inner's required signer is the delegate (dave), not bob. + // Bob signs but is not a required signer, so the batch is rejected as an + // extra signer. The delegator's signature cannot stand in for the + // delegate's. { Env env{*this, features}; Account const alice("alice"); @@ -2189,7 +2194,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase batch::Inner(jv1, bobSeq), batch::Inner(jv2, carolSeq), batch::Sig(bob, carol), - Ter(tesSUCCESS)); + Ter(temBAD_SIGNER)); env.close(); // jv1 fails before jv2 is attempted. @@ -2257,7 +2262,7 @@ class ConfidentialTransferExtended_test : public ConfidentialTransferTestBase batch::Inner(jv4, frankSeq), batch::Inner(jv5, frankSeq + 1), batch::Inner(jv6, bobSeq + 2), - batch::Sig(bob, carol, frank), + batch::Sig(erin, frank), Ter(tesSUCCESS)); env.close(); diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index 2422db05de..ed99817f34 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -1,12 +1,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -36,6 +38,8 @@ #include #include #include +#include +#include #include #include #include @@ -68,7 +72,7 @@ namespace xrpl::test { struct LedgerReplay_test : public beast::unit_test::Suite { void - run() override + testReplayLedger() { testcase("Replay ledger"); @@ -91,6 +95,45 @@ struct LedgerReplay_test : public beast::unit_test::Suite BEAST_EXPECT(replayed->header().hash == lastClosed->header().hash); } + + void + testReplayBatchLedger() + { + testcase("Replay ledger with batch transactions"); + + using namespace jtx; + + Env env(*this, testableAmendments()); + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(100000), alice, bob); + env.close(); + + auto const seq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, 0, 2); + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(1)), seq + 1), + batch::Inner(pay(alice, bob, XRP(2)), seq + 2), + Ter(tesSUCCESS)); + env.close(); + + LedgerMaster& ledgerMaster = env.app().getLedgerMaster(); + auto const lastClosed = ledgerMaster.getClosedLedger(); + auto const lastClosedParent = ledgerMaster.getLedgerByHash(lastClosed->header().parentHash); + + auto const replayed = buildLedger( + LedgerReplay(lastClosedParent, lastClosed), TapNone, env.app(), env.journal); + + BEAST_EXPECT(replayed->header().hash == lastClosed->header().hash); + } + + void + run() override + { + testReplayLedger(); + testReplayBatchLedger(); + } }; enum class InboundLedgersBehavior { diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 2d10eb4beb..91eb26da7e 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -61,8 +61,7 @@ namespace xrpl::test { class TxQPosNegFlows_test : public beast::unit_test::Suite { - // Same as corresponding values from TxQ.h - static constexpr FeeLevel64 kBaseFeeLevel{256}; + static constexpr FeeLevel64 kBaseFeeLevel{TxQ::kBaseLevel}; static constexpr FeeLevel64 kMinEscalationFeeLevel = kBaseFeeLevel * 500; static void diff --git a/src/test/jtx/TestHelpers.h b/src/test/jtx/TestHelpers.h index 0e35e6b9ec..34532b17f7 100644 --- a/src/test/jtx/TestHelpers.h +++ b/src/test/jtx/TestHelpers.h @@ -731,8 +731,8 @@ create(jtx::Account const& account, jtx::Account const& dest, STAmount const& se } // namespace check -static constexpr FeeLevel64 kBaseFeeLevel{TxQ::kBaseLevel}; -static constexpr FeeLevel64 kMinEscalationFeeLevel = kBaseFeeLevel * 500; +inline constexpr FeeLevel64 kBaseFeeLevel{TxQ::kBaseLevel}; +inline constexpr FeeLevel64 kMinEscalationFeeLevel = kBaseFeeLevel * 500; inline uint256 getCheckIndex(AccountID const& account, std::uint32_t uSequence) diff --git a/src/test/jtx/batch.h b/src/test/jtx/batch.h index 0e78d409fa..4d564f150a 100644 --- a/src/test/jtx/batch.h +++ b/src/test/jtx/batch.h @@ -58,7 +58,6 @@ class Inner { private: json::Value txn_; - std::uint32_t seq_; std::optional ticket_; public: @@ -66,10 +65,10 @@ public: json::Value txn, std::uint32_t const& sequence, std::optional const& ticket = std::nullopt) - : txn_(std::move(txn)), seq_(sequence), ticket_(ticket) + : txn_(std::move(txn)), ticket_(ticket) { txn_[jss::SigningPubKey] = ""; - txn_[jss::Sequence] = seq_; + txn_[jss::Sequence] = sequence; txn_[jss::Fee] = "0"; txn_[jss::Flags] = txn_[jss::Flags].asUInt() | tfInnerBatchTxn; diff --git a/src/test/jtx/impl/batch.cpp b/src/test/jtx/impl/batch.cpp index 66ca0c7d54..b1061f65a3 100644 --- a/src/test/jtx/impl/batch.cpp +++ b/src/test/jtx/impl/batch.cpp @@ -99,7 +99,13 @@ Sig::operator()(Env& env, JTx& jt) const jo[jss::SigningPubKey] = strHex(e.sig.pk().slice()); Serializer msg; - serializeBatch(msg, stx.getFlags(), stx.getBatchTransactionIDs()); + serializeBatch( + msg, + stx.getAccountID(sfAccount), + stx.getSeqValue(), + stx.getFlags(), + stx.getBatchTransactionIDs()); + finishMultiSigningData(e.acct.id(), msg); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) auto const sig = xrpl::sign(*publicKeyType(e.sig.pk().slice()), e.sig.sk(), msg.slice()); jo[sfTxnSignature.getJsonName()] = strHex(Slice{sig.data(), sig.size()}); @@ -137,7 +143,13 @@ Msig::operator()(Env& env, JTx& jt) const iso[jss::SigningPubKey] = strHex(e.sig.pk().slice()); Serializer msg; - serializeBatch(msg, stx.getFlags(), stx.getBatchTransactionIDs()); + serializeBatch( + msg, + stx.getAccountID(sfAccount), + stx.getSeqValue(), + stx.getFlags(), + stx.getBatchTransactionIDs()); + msg.addBitString(master.id()); finishMultiSigningData(e.acct.id(), msg); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) auto const sig = xrpl::sign(*publicKeyType(e.sig.pk().slice()), e.sig.sk(), msg.slice()); diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 8cda5965cb..a36e51cb6f 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -134,7 +134,7 @@ class Feature_test : public beast::unit_test::Suite // or removed, swap out for any other feature. BEAST_EXPECT( featureToName(fixRemoveNFTokenAutoTrustLine) == "fixRemoveNFTokenAutoTrustLine"); - BEAST_EXPECT(featureToName(featureBatch) == "Batch"); + BEAST_EXPECT(featureToName(featureBatchV1_1) == "BatchV1_1"); BEAST_EXPECT(featureToName(featureDID) == "DID"); BEAST_EXPECT(featureToName(fixIncludeKeyletFields) == "fixIncludeKeyletFields"); BEAST_EXPECT(featureToName(featureTokenEscrow) == "TokenEscrow"); diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index 5ea79c3996..0d00360a58 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -420,6 +420,20 @@ class Simulate_test : public beast::unit_test::Suite BEAST_EXPECT( resp[jss::result][jss::error_message] == "Transaction should not be signed."); } + { + // tfInnerBatchTxn flag on top-level transaction + json::Value params; + json::Value txJson{json::ValueType::Object}; + txJson[jss::TransactionType] = jss::AccountSet; + txJson[jss::Account] = env.master.human(); + txJson[jss::Flags] = tfInnerBatchTxn; + params[jss::tx_json] = txJson; + + auto const resp = env.rpc("json", "simulate", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "tfInnerBatchTxn flag is not allowed on top-level transactions."); + } } void @@ -476,7 +490,7 @@ class Simulate_test : public beast::unit_test::Suite auto jt = env.jtnofill( batch::outer(alice, env.seq(alice), batchFee, tfAllOrNothing), batch::Inner(pay(alice, bob, XRP(10)), seq + 1), - batch::Inner(pay(alice, bob, XRP(10)), seq + 1)); + batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); jt.jv.removeMember(jss::TxnSignature); json::Value params; diff --git a/src/xrpld/app/ledger/detail/BuildLedger.cpp b/src/xrpld/app/ledger/detail/BuildLedger.cpp index 2eb64ecaf9..038a7be4b7 100644 --- a/src/xrpld/app/ledger/detail/BuildLedger.cpp +++ b/src/xrpld/app/ledger/detail/BuildLedger.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -233,7 +234,15 @@ buildLedger( j, [&](OpenView& accum, std::shared_ptr const& built) { for (auto& tx : replayData.orderedTxns()) + { + // Inner batch transactions are applied as part of their outer + // Batch transaction, never on their own. Skip them here so they + // are not re-applied a second time outside of the batch during + // replay. + if (tx.second->isFlag(tfInnerBatchTxn)) + continue; applyTransaction(app, accum, *tx.second, false, applyFlags, j); + } }); } diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 1802441a66..82445f3625 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1253,8 +1253,9 @@ NetworkOPsImp::submitTransaction(std::shared_ptr const& iTrans) return; } - // Enforce Network bar for batch txn - if (iTrans->isFlag(tfInnerBatchTxn) && ledgerMaster_.getValidatedRules().enabled(featureBatch)) + // Reject any transaction with the tfInnerBatchTxn flag at the network + // boundary, regardless of amendment state. + if (iTrans->isFlag(tfInnerBatchTxn)) { JLOG(journal_.error()) << "Submitted transaction invalid: tfInnerBatchTxn flag present."; return; @@ -1320,7 +1321,7 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) // under no circumstances will we ever accept an inner txn within a batch // txn from the network. auto const sttx = *transaction->getSTransaction(); - if (sttx.isFlag(tfInnerBatchTxn) && view->rules().enabled(featureBatch)) + if (sttx.isFlag(tfInnerBatchTxn)) { transaction->setStatus(TransStatus::INVALID); transaction->setResult(temINVALID_FLAG); diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index ec5ac569d6..7e57302d23 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -390,6 +391,12 @@ TxQ::canBeHeld( std::optional const& replacementIter, std::scoped_lock const& lock) { + // 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; + // PreviousTxnID is deprecated and should never be used. // AccountTxnID is not supported by the transaction // queue yet, but should be added in the future. diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index e1d7d23215..5ac61cfd91 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1309,7 +1309,7 @@ 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 + There is no need to check whether the featureBatchV1_1 amendment is enabled. * If the `tfInnerBatchTxn` flag is set, and the amendment is @@ -2873,7 +2873,7 @@ PeerImp::checkTransaction( // charge strongly for relaying batch txns // LCOV_EXCL_START /* - There is no need to check whether the featureBatch amendment is + There is no need to check whether the featureBatchV1_1 amendment is enabled. * If the `tfInnerBatchTxn` flag is set, and the amendment is diff --git a/src/xrpld/rpc/handlers/transaction/Simulate.cpp b/src/xrpld/rpc/handlers/transaction/Simulate.cpp index 7ee28c4886..2a6c2280e3 100644 --- a/src/xrpld/rpc/handlers/transaction/Simulate.cpp +++ b/src/xrpld/rpc/handlers/transaction/Simulate.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -355,6 +356,13 @@ doSimulate(RPC::JsonContext& context) return RPC::makeError(RpcNotImpl); } + // Reject transactions with the tfInnerBatchTxn flag. + if (stTx->isFlag(tfInnerBatchTxn)) + { + return RPC::makeError( + RpcInvalidParams, "tfInnerBatchTxn flag is not allowed on top-level transactions."); + } + std::string reason; auto transaction = std::make_shared(stTx, reason, context.app); // Actually run the transaction through the transaction processor