From ecf7f805c90538e177470febb0a3a5b989c60620 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 1 Jul 2026 01:51:41 +0200 Subject: [PATCH 1/9] feat: Introduce lending 1.1 amendment and add `MemoData` field to `VaultDelete` transaction (#6324) --- include/xrpl/protocol/detail/features.macro | 1 + .../xrpl/protocol/detail/transactions.macro | 1 + .../transactions/VaultDelete.h | 37 ++++++++++ .../tx/transactors/vault/VaultDelete.cpp | 8 +++ src/test/app/Vault_test.cpp | 70 +++++++++++++++++++ .../transactions/VaultDeleteTests.cpp | 49 +++++++++++++ 6 files changed, 166 insertions(+) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index a9b237be32..3c808d960b 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -14,6 +14,7 @@ // 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_FIX (Cleanup3_3_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_2_0, Supported::Yes, VoteBehavior::DefaultNo) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 8a3b0ff2ce..b46fca92b6 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -889,6 +889,7 @@ TRANSACTION(ttVAULT_DELETE, 67, VaultDelete, MustDeleteAcct | DestroyMptIssuance | MustModifyVault, ({ {sfVaultID, SoeRequired}, + {sfMemoData, SoeOptional}, })) /** This transaction trades assets for shares with a vault. */ diff --git a/include/xrpl/protocol_autogen/transactions/VaultDelete.h b/include/xrpl/protocol_autogen/transactions/VaultDelete.h index 2b6e248c0a..b4c08ae229 100644 --- a/include/xrpl/protocol_autogen/transactions/VaultDelete.h +++ b/include/xrpl/protocol_autogen/transactions/VaultDelete.h @@ -57,6 +57,32 @@ public: { return this->tx_->at(sfVaultID); } + + /** + * @brief Get sfMemoData (SoeOptional) + * @return The field value, or std::nullopt if not present. + */ + [[nodiscard]] + protocol_autogen::Optional + getMemoData() const + { + if (hasMemoData()) + { + return this->tx_->at(sfMemoData); + } + return std::nullopt; + } + + /** + * @brief Check if sfMemoData is present. + * @return True if the field is present, false otherwise. + */ + [[nodiscard]] + bool + hasMemoData() const + { + return this->tx_->isFieldPresent(sfMemoData); + } }; /** @@ -112,6 +138,17 @@ public: return *this; } + /** + * @brief Set sfMemoData (SoeOptional) + * @return Reference to this builder for method chaining. + */ + VaultDeleteBuilder& + setMemoData(std::decay_t const& value) + { + object_[sfMemoData] = value; + return *this; + } + /** * @brief Build and return the VaultDelete wrapper. * @param publicKey The public key for signing. diff --git a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp index 0550f08b17..fa38ae278b 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp @@ -7,8 +7,10 @@ #include #include #include +#include #include #include +#include #include #include #include // IWYU pragma: keep @@ -28,6 +30,12 @@ VaultDelete::preflight(PreflightContext const& ctx) return temMALFORMED; } + if (ctx.tx.isFieldPresent(sfMemoData) && !ctx.rules.enabled(featureLendingProtocolV1_1)) + return temDISABLED; + + if (!validDataLength(ctx.tx[~sfMemoData], kMaxDataPayloadLength)) + return temMALFORMED; + return tesSUCCESS; } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 9b29442197..d9afd93b23 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -7511,6 +7511,74 @@ class Vault_test : public beast::unit_test::Suite } } + void + testVaultDeleteMemoData() + { + using namespace test::jtx; + + Env env{*this}; + + Account const owner{"owner"}; + env.fund(XRP(1'000'000), owner); + env.close(); + + Vault const vault{env}; + + auto const keylet = keylet::vault(owner.id(), 1); + auto delTx = vault.del({.owner = owner, .id = keylet.key}); + + // Test VaultDelete with featureLendingProtocolV1_1 disabled + // Transaction fails if the data field is provided + { + testcase("VaultDelete memo data featureLendingProtocolV1_1 disabled"); + env.disableFeature(featureLendingProtocolV1_1); + delTx[sfMemoData] = strHex(std::string(kMaxDataPayloadLength, 'A')); + env(delTx, Ter(temDISABLED)); + env.enableFeature(featureLendingProtocolV1_1); + env.close(); + } + + // Transaction fails if the data field is too large + { + testcase("VaultDelete memo data featureLendingProtocolV1_1 enabled data too large"); + delTx[sfMemoData] = strHex(std::string(kMaxDataPayloadLength + 1, 'A')); + env(delTx, Ter(temMALFORMED)); + env.close(); + } + + // Transaction fails if the data field is set, but is empty + { + testcase("VaultDelete memo data featureLendingProtocolV1_1 enabled data empty"); + delTx[sfMemoData] = strHex(std::string(0, 'A')); + env(delTx, Ter(temMALFORMED)); + env.close(); + } + + { + testcase("VaultDelete memo data featureLendingProtocolV1_1 enabled no vault"); + auto const keylet = keylet::vault(owner.id(), env.seq(owner)); + + // Recreate the transaction as the vault keylet changed + auto delTx = vault.del({.owner = owner, .id = keylet.key}); + delTx[sfMemoData] = strHex(std::string(kMaxDataPayloadLength, 'A')); + env(delTx, Ter(tecNO_ENTRY)); + env.close(); + } + + { + testcase("VaultDelete memo data featureLendingProtocolV1_1 enabled data valid"); + PrettyAsset const xrpAsset = xrpIssue(); + auto const [tx, keylet] = vault.create({.owner = owner, .asset = xrpAsset}); + env(tx, Ter(tesSUCCESS)); + env.close(); + // Recreate the transaction as the vault keylet changed + auto delTx = vault.del({.owner = owner, .id = keylet.key}); + delTx[sfMemoData] = strHex(std::string(kMaxDataPayloadLength, 'A')); + env(delTx, Ter(tesSUCCESS)); + env.close(); + } + } + void testVaultDepositFreeze() { @@ -8082,6 +8150,7 @@ class Vault_test : public beast::unit_test::Suite runTests(); env.disableFeature(fixCleanup3_3_0); + runTests(); env.enableFeature(fixCleanup3_3_0); } @@ -8115,6 +8184,7 @@ public: testVaultClawbackAssets(); testVaultEscrowedMPT(); testAssetsMaximum(); + testVaultDeleteMemoData(); testBug6LimitBypassWithShares(); testRemoveEmptyHoldingLockedAmount(); testRemoveEmptyHoldingConfidentialBalances(); diff --git a/src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp b/src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp index 70747af1ed..8f0eb2e4cd 100644 --- a/src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp +++ b/src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp @@ -30,6 +30,7 @@ TEST(TransactionsVaultDeleteTests, BuilderSettersRoundTrip) // Transaction-specific field values auto const vaultIDValue = canonical_UINT256(); + auto const memoDataValue = canonical_VL(); VaultDeleteBuilder builder{ accountValue, @@ -39,6 +40,7 @@ TEST(TransactionsVaultDeleteTests, BuilderSettersRoundTrip) }; // Set optional fields + builder.setMemoData(memoDataValue); auto tx = builder.build(publicKey, secretKey); @@ -62,6 +64,14 @@ TEST(TransactionsVaultDeleteTests, BuilderSettersRoundTrip) } // Verify optional fields + { + auto const& expected = memoDataValue; + auto const actualOpt = tx.getMemoData(); + ASSERT_TRUE(actualOpt.has_value()) << "Optional field sfMemoData should be present"; + expectEqualField(expected, *actualOpt, "sfMemoData"); + EXPECT_TRUE(tx.hasMemoData()); + } + } // 2 & 4) Start from an STTx, construct a builder from it, build a new wrapper, @@ -79,6 +89,7 @@ TEST(TransactionsVaultDeleteTests, BuilderFromStTxRoundTrip) // Transaction-specific field values auto const vaultIDValue = canonical_UINT256(); + auto const memoDataValue = canonical_VL(); // Build an initial transaction VaultDeleteBuilder initialBuilder{ @@ -88,6 +99,7 @@ TEST(TransactionsVaultDeleteTests, BuilderFromStTxRoundTrip) feeValue }; + initialBuilder.setMemoData(memoDataValue); auto initialTx = initialBuilder.build(publicKey, secretKey); @@ -112,6 +124,13 @@ TEST(TransactionsVaultDeleteTests, BuilderFromStTxRoundTrip) } // Verify optional fields + { + auto const& expected = memoDataValue; + auto const actualOpt = rebuiltTx.getMemoData(); + ASSERT_TRUE(actualOpt.has_value()) << "Optional field sfMemoData should be present"; + expectEqualField(expected, *actualOpt, "sfMemoData"); + } + } // 3) Verify wrapper throws when constructed from wrong transaction type. @@ -142,5 +161,35 @@ TEST(TransactionsVaultDeleteTests, BuilderThrowsOnWrongTxType) EXPECT_THROW(VaultDeleteBuilder{wrongTx.getSTTx()}, std::runtime_error); } +// 5) Build with only required fields and verify optional fields return nullopt. +TEST(TransactionsVaultDeleteTests, OptionalFieldsReturnNullopt) +{ + // Generate a deterministic keypair for signing + auto const [publicKey, secretKey] = + generateKeyPair(KeyType::Secp256k1, generateSeed("testVaultDeleteNullopt")); + + // Common transaction fields + auto const accountValue = calcAccountID(publicKey); + std::uint32_t const sequenceValue = 3; + auto const feeValue = canonical_AMOUNT(); + + // Transaction-specific required field values + auto const vaultIDValue = canonical_UINT256(); + + VaultDeleteBuilder builder{ + accountValue, + vaultIDValue, + sequenceValue, + feeValue + }; + + // Do NOT set optional fields + + auto tx = builder.build(publicKey, secretKey); + + // Verify optional fields are not present + EXPECT_FALSE(tx.hasMemoData()); + EXPECT_FALSE(tx.getMemoData().has_value()); +} } From 86d8b244d6e9b61952de736c010c789e12c6498e Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Wed, 1 Jul 2026 08:47:14 -0400 Subject: [PATCH 2/9] 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 From ea13be81b7037ce0a03c706e5b916b0beb581496 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:21:23 +0200 Subject: [PATCH 3/9] feat: Add an invariant to ensure object deletion also deletes its pseudo-account (#7445) Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> --- include/xrpl/tx/invariants/InvariantCheck.h | 26 +++- src/libxrpl/tx/invariants/InvariantCheck.cpp | 95 +++++++++++-- src/test/app/Invariants_test.cpp | 142 +++++++++++++++++-- 3 files changed, 242 insertions(+), 21 deletions(-) diff --git a/include/xrpl/tx/invariants/InvariantCheck.h b/include/xrpl/tx/invariants/InvariantCheck.h index 8931d189fd..ba91cba064 100644 --- a/include/xrpl/tx/invariants/InvariantCheck.h +++ b/include/xrpl/tx/invariants/InvariantCheck.h @@ -375,16 +375,35 @@ public: */ class ValidAmounts { - std::vector> afterEntries_; + std::vector afterEntries_; public: void - visitEntry(bool, std::shared_ptr const&, std::shared_ptr const&); + visitEntry(bool, SLE::const_ref, SLE::const_ref); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; }; +/* + * Verify that when an object with an associated pseudo-account is deleted, + * its pseudo-account is also deleted. + * + * The reverse (pseudo-account deleted → object deleted) is enforced by + * AccountRootsDeletedClean via getPseudoAccountFields(). + */ +class ObjectHasPseudoAccount +{ +public: + void + visitEntry(bool, SLE::const_ref, SLE::const_ref); + + [[nodiscard]] bool + finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; + +private: + std::vector deletedObjSles_; +}; // additional invariant checks can be declared above and then added to this // tuple using InvariantChecks = std::tuple< @@ -416,7 +435,8 @@ using InvariantChecks = std::tuple< ValidConfidentialMPToken, ValidMPTPayment, ValidAmounts, - ValidMPTTransfer>; + ValidMPTTransfer, + ObjectHasPseudoAccount>; /** * @brief get a tuple of all invariant checks diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 308342da74..32df44a96b 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include namespace xrpl { @@ -63,6 +64,23 @@ hasPrivilege(STTx const& tx, Privilege priv) #undef TRANSACTION #pragma pop_macro("TRANSACTION") +// Returns the human-readable name of a ledger entry's type, falling back to +// the numeric type if the format is somehow unknown. +static std::string +ledgerEntryTypeName(SLE const& sle) +{ + auto const item = LedgerFormats::getInstance().findByType(sle.getType()); + + if (item == nullptr) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::ledgerEntryTypeName : ledger entry has no known ledger format"); + return std::to_string(sle.getType()); + // LCOV_EXCL_STOP + } + return item->getName(); +} + void TransactionFeeCheck::visitEntry(bool, SLE::const_ref, SLE::const_ref) { @@ -457,16 +475,8 @@ AccountRootsDeletedClean::finalize( if (auto const sle = view.read(keylet)) { // Finding the object is bad - auto const typeName = [&sle]() { - auto item = LedgerFormats::getInstance().findByType(sle->getType()); - - if (item != nullptr) - return item->getName(); - return std::to_string(sle->getType()); - }(); - - JLOG(j.fatal()) << "Invariant failed: account deletion left behind a " << typeName - << " object"; + JLOG(j.fatal()) << "Invariant failed: account deletion left behind a " + << ledgerEntryTypeName(*sle) << " object"; // The comment above starting with "assert(enforce)" explains this // assert. XRPL_ASSERT( @@ -1070,4 +1080,69 @@ ValidAmounts::finalize( return true; } +void +ObjectHasPseudoAccount::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +{ + if (!isDelete) + return; + + // Before should never be null when isDelete = true + if (!before) + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ObjectHasPseudoAccount::visitEntry : deleted ledger entry missing before state"); + return; + // LCOV_EXCL_STOP + } + + switch (before->getType()) + { + case ltAMM: + case ltVAULT: + case ltLOAN_BROKER: + deletedObjSles_.push_back(before); + break; + default: + return; + } +} + +[[nodiscard]] bool +ObjectHasPseudoAccount::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) const +{ + if (!view.rules().enabled(fixCleanup3_3_0)) + return true; + + if (deletedObjSles_.empty()) + return true; + + bool failed = false; + for (auto const& sle : deletedObjSles_) + { + if (!sle->isFieldPresent(sfAccount)) + { + JLOG(j.fatal()) << "Invariant failed: deleted " << ledgerEntryTypeName(*sle) + << " is missing pseudo-account field"; + failed = true; + continue; + } + + // The pseudo-account must NOT exist on the ledger after the object is deleted. + if (view.exists(keylet::account(sle->getAccountID(sfAccount)))) + { + JLOG(j.fatal()) << "Invariant failed: deleted " << ledgerEntryTypeName(*sle) + << " without deleting its pseudo-account"; + failed = true; + } + } + + return !failed; +} + } // namespace xrpl diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 9fde52ecb2..079ff9df88 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2829,7 +2829,8 @@ class Invariants_test : public beast::unit_test::Suite }); doInvariantCheck( - {"vault updated by a wrong transaction type"}, + {"vault updated by a wrong transaction type", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -2840,7 +2841,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttPAYMENT, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; auto [tx, _] = vault.create({.owner = a1, .asset = xrpIssue()}); @@ -2877,6 +2878,7 @@ class Invariants_test : public beast::unit_test::Suite auto const vaultPage = ac.view().dirInsert( keylet::ownerDir(a1.id()), sleVault->key(), describeOwnerDir(a1.id())); sleVault->setFieldU64(sfOwnerNode, *vaultPage); + sleVault->setAccountID(sfAccount, a1.id()); ac.view().insert(sleVault); return true; }, @@ -2885,7 +2887,8 @@ class Invariants_test : public beast::unit_test::Suite {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); doInvariantCheck( - {"vault deleted by a wrong transaction type"}, + {"vault deleted by a wrong transaction type", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -2896,7 +2899,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_SET, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; auto [tx, _] = vault.create({.owner = a1, .asset = xrpIssue()}); @@ -2905,7 +2908,8 @@ class Invariants_test : public beast::unit_test::Suite }); doInvariantCheck( - {"vault operation updated more than single vault"}, + {"vault operation updated more than single vault", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); @@ -2925,7 +2929,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_DELETE, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; { @@ -2949,6 +2953,7 @@ class Invariants_test : public beast::unit_test::Suite auto const vaultPage = ac.view().dirInsert( keylet::ownerDir(a.id()), sleVault->key(), describeOwnerDir(a.id())); sleVault->setFieldU64(sfOwnerNode, *vaultPage); + sleVault->setAccountID(sfAccount, a.id()); ac.view().insert(sleVault); }; insertVault(a1); @@ -2960,7 +2965,8 @@ class Invariants_test : public beast::unit_test::Suite {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); doInvariantCheck( - {"deleted vault must also delete shares"}, + {"deleted vault must also delete shares", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -2971,7 +2977,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_DELETE, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; auto [tx, _] = vault.create({.owner = a1, .asset = xrpIssue()}); @@ -5176,6 +5182,125 @@ class Invariants_test : public beast::unit_test::Suite } } + void + testObjectHasPseudoAccount() + { + testcase << "object has pseudo-account"; + using namespace jtx; + + auto const amendments = defaultAmendments() | fixCleanup3_3_0; + + // Vault: object deleted without its pseudo-account + { + Keylet vaultKeylet = keylet::amendments(); + doInvariantCheck( + Env{*this, amendments}, + {{"deleted Vault without deleting its pseudo-account"}}, + [&vaultKeylet](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(vaultKeylet); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttVAULT_DELETE, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&vaultKeylet](Account const& a1, Account const&, Env& env) { + Vault const vault{env}; + auto [tx, keylet] = vault.create({.owner = a1, .asset = xrpIssue()}); + env(tx); + vaultKeylet = keylet; + return true; + }); + } + + // AMM: object deleted without its pseudo-account + { + uint256 ammID{}; + Account const gw{"gw"}; + doInvariantCheck( + Env{*this, amendments}, + {{"deleted AMM without deleting its pseudo-account"}}, + [&ammID](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(keylet::amm(ammID)); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttAMM_DELETE, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&ammID, &gw](Account const&, Account const&, Env& env) { + env.fund(XRP(1'000), gw); + AMM const amm(env, gw, XRP(100), gw["USD"](100)); + ammID = amm.ammID(); + return true; + }); + } + + // LoanBroker: object deleted without its pseudo-account + { + Keylet loanBrokerKeylet = keylet::amendments(); + doInvariantCheck( + Env{*this, amendments}, + {{"deleted LoanBroker without deleting its pseudo-account"}}, + [&loanBrokerKeylet](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(loanBrokerKeylet); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttLOAN_BROKER_DELETE, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&loanBrokerKeylet, this](Account const& a1, Account const&, Env& env) { + PrettyAsset const xrpAsset{xrpIssue(), 1'000'000}; + loanBrokerKeylet = this->createLoanBroker(a1, env, xrpAsset); + return BEAST_EXPECT(env.le(loanBrokerKeylet)); + }); + } + + // Deleted object missing sfAccount field (defensive check). + // Manually construct the view to place a vault SLE without + // sfAccount into the base ledger, then erase it. + { + Env env{*this, amendments}; + Account const a1{"A1"}; + Account const a2{"A2"}; + env.fund(XRP(1000), a1, a2); + env.close(); + + OpenView ov{*env.current()}; + + auto const vaultKeylet = keylet::vault(a1.id(), ov.seq()); + auto sleVault = std::make_shared(vaultKeylet); + sleVault->makeFieldAbsent(sfAccount); + ov.rawInsert(sleVault); + + STTx const tx{ttVAULT_DELETE, [](STObject&) {}}; + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + ApplyContext ac{ + env.app(), ov, tx, tesSUCCESS, env.current()->fees().base, TapNone, jlog}; + CurrentTransactionRulesGuard const rulesGuard(ov.rules()); + + auto sle = ac.view().peek(vaultKeylet); + if (!BEAST_EXPECT(sle)) + return; + ac.view().erase(sle); + + auto transactor = makeTransactor(ac); + if (!BEAST_EXPECT(transactor)) + return; + TER const result = transactor->checkInvariants(tesSUCCESS, XRPAmount{}); + BEAST_EXPECT(result == tecINVARIANT_FAILED); + BEAST_EXPECT(sink.messages().str().contains("is missing pseudo-account field")); + } + } + void testConfidentialMPTTransfer() { @@ -5458,6 +5583,7 @@ public: testInvariantOverwrite(defaultAmendments() - fixCleanup3_1_3); testVaultComputeCoarsestScale(); testAMM(); + testObjectHasPseudoAccount(); } }; From 6aed3bb71d2820f081ce2a22666e456420a814b2 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Wed, 1 Jul 2026 14:28:14 +0100 Subject: [PATCH 4/9] chore: Update pre-commit hooks && actions (#7686) --- .github/workflows/pre-commit.yml | 2 +- .github/workflows/publish-docs.yml | 2 +- .github/workflows/reusable-build-test-config.yml | 2 +- .github/workflows/reusable-clang-tidy.yml | 2 +- .github/workflows/upload-conan-deps.yml | 2 +- .pre-commit-config.yaml | 6 +++--- docs/0001-negative-unl/README.md | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 0363534af5..1acd28208e 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -14,7 +14,7 @@ on: jobs: # Call the workflow in the XRPLF/actions repo that runs the pre-commit hooks. run-hooks: - uses: XRPLF/actions/.github/workflows/pre-commit.yml@e06d4138c9ec8dceeb7c818645faa38087ea9e3d + uses: XRPLF/actions/.github/workflows/pre-commit.yml@1bde119a1ab71305ba5d3716e7a82cea1c7bdede with: runs_on: ubuntu-latest container: '{ "image": "ghcr.io/xrplf/ci/tools-rippled-pre-commit:sha-41ec7c1" }' diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index bfa8d2e79c..1ac8d61655 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -47,7 +47,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@9355d190fd7d4de80fadfd161e6edddc9702cd9f + uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c with: enable_ccache: false diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 0cb0219d72..e7a88a0e66 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -113,7 +113,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@9355d190fd7d4de80fadfd161e6edddc9702cd9f + uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c with: enable_ccache: ${{ inputs.ccache_enabled }} diff --git a/.github/workflows/reusable-clang-tidy.yml b/.github/workflows/reusable-clang-tidy.yml index b04847e137..5528f3452e 100644 --- a/.github/workflows/reusable-clang-tidy.yml +++ b/.github/workflows/reusable-clang-tidy.yml @@ -43,7 +43,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@9355d190fd7d4de80fadfd161e6edddc9702cd9f + uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c with: enable_ccache: false diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index 88b364c2b1..22ad36d98f 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -68,7 +68,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@9355d190fd7d4de80fadfd161e6edddc9702cd9f + uses: XRPLF/actions/prepare-runner@64ec3cf3b152b4444638f470bbd6df7a7a30c81c with: enable_ccache: false diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4cbf4c1dd0..7bac4d1140 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -51,12 +51,12 @@ repos: exclude: ^include/xrpl/protocol_autogen/(transactions|ledger_entries)/ - repo: https://github.com/BlankSpruce/gersemi-pre-commit - rev: faadd6a9d852369ca94f4d15b2404c967ba8cb01 # frozen: 0.27.6 + rev: e98930bdc210d3387007f9252d8c1694ea7e410f # frozen: 0.27.7 hooks: - id: gersemi - repo: https://github.com/rbubley/mirrors-prettier - rev: 515f543f5718ebfd6ce22e16708bb32c68ff96e1 # frozen: v3.8.3 + rev: 39e2973981e6d2f9b6c543b0086a2d2393abdc89 # frozen: v3.9.4 hooks: - id: prettier args: [--end-of-line=auto] @@ -86,7 +86,7 @@ repos: files: \.md$ - repo: https://github.com/streetsidesoftware/cspell-cli - rev: 4643f154907327ee0a2c7038f0296e0dd77d9776 # frozen: v10.0.0 + rev: ea11f9efc0bec520073405bc30552da887ba71bc # frozen: v10.0.1 hooks: - id: cspell # Spell check changed files exclude: | diff --git a/docs/0001-negative-unl/README.md b/docs/0001-negative-unl/README.md index dd5f9af2ae..0bc65cd860 100644 --- a/docs/0001-negative-unl/README.md +++ b/docs/0001-negative-unl/README.md @@ -288,7 +288,7 @@ components with non-trivial changes are colored green. validated. ![Sequence diagram](./negativeUNL_highLevel_sequence.png?raw=true "Negative UNL - Changes") +Changes") ## Roads Not Taken From 6d0b758a12eeb24b3c980d4673be3673040fdc63 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Wed, 1 Jul 2026 14:28:41 +0100 Subject: [PATCH 5/9] build: Don't reuse binaries between different C++ versions (#7681) --- conan/profiles/default | 12 +++++++----- conan/profiles/sanitizers | 10 +++++----- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/conan/profiles/default b/conan/profiles/default index e0a88ebca1..ae6e23c3c3 100644 --- a/conan/profiles/default +++ b/conan/profiles/default @@ -10,16 +10,18 @@ os={{ os }} arch={{ arch }} build_type=Debug -compiler={{compiler}} +compiler={{ compiler }} compiler.version={{ compiler_version }} compiler.cppstd=23 {% if os == "Windows" %} compiler.runtime=static {% else %} -compiler.libcxx={{detect_api.detect_libcxx(compiler, version, compiler_exe)}} +compiler.libcxx={{ detect_api.detect_libcxx(compiler, version, compiler_exe) }} {% endif %} [conf] -{% if compiler == "gcc" and compiler_version < 13 %} -tools.build:cxxflags+=['-Wno-restrict'] -{% endif %} +{# By default, Conan tries to reuse binaries built with different cppstd versions. #} +{# We want to avoid that to improve reproduceability, so we add the cppstd version to the package ID. #} +{# More info: https://docs.conan.io/2/reference/extensions/binary_compatibility.html #} +user.package:cppstd_version=23 +tools.info.package_id:confs+=["user.package:cppstd_version"] diff --git a/conan/profiles/sanitizers b/conan/profiles/sanitizers index 083807ea9e..09e6aef02b 100644 --- a/conan/profiles/sanitizers +++ b/conan/profiles/sanitizers @@ -87,15 +87,15 @@ include(default) {% endif %} [conf] -tools.build:defines+={{defines}} -tools.build:cxxflags+={{sanitizer_compiler_flags}} -tools.build:sharedlinkflags+={{sanitizer_linker_flags}} -tools.build:exelinkflags+={{sanitizer_linker_flags}} +tools.build:defines+={{ defines }} +tools.build:cxxflags+={{ sanitizer_compiler_flags }} +tools.build:sharedlinkflags+={{ sanitizer_linker_flags }} +tools.build:exelinkflags+={{ sanitizer_linker_flags }} tools.info.package_id:confs+=["tools.build:cxxflags", "tools.build:exelinkflags", "tools.build:sharedlinkflags", "tools.build:defines"] # &: means "apply only to the consumer/root package" -&:tools.cmake.cmaketoolchain:extra_variables={"SANITIZERS": "{{sanitizers}}", "SANITIZERS_COMPILER_FLAGS": "{{sanitizer_compiler_flags | join(' ')}}", "SANITIZERS_LINKER_FLAGS": "{{sanitizer_linker_flags | join(' ')}}"} +&:tools.cmake.cmaketoolchain:extra_variables={"SANITIZERS": "{{ sanitizers }}", "SANITIZERS_COMPILER_FLAGS": "{{ sanitizer_compiler_flags | join(' ') }}", "SANITIZERS_LINKER_FLAGS": "{{ sanitizer_linker_flags | join(' ') }}"} [options] {% if enable_asan %} From ba739c94ce184933e728295527131242317acb9b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 1 Jul 2026 09:29:06 -0400 Subject: [PATCH 6/9] ci: [DEPENDABOT] bump actions/setup-python from 6.2.0 to 6.3.0 (#7657) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/reusable-package.yml | 2 +- .github/workflows/reusable-strategy-matrix.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/reusable-package.yml b/.github/workflows/reusable-package.yml index 249e807592..6feecbfb75 100644 --- a/.github/workflows/reusable-package.yml +++ b/.github/workflows/reusable-package.yml @@ -30,7 +30,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Set up Python - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 with: python-version: "3.13" diff --git a/.github/workflows/reusable-strategy-matrix.yml b/.github/workflows/reusable-strategy-matrix.yml index c1a1c1a78b..690aa3d423 100644 --- a/.github/workflows/reusable-strategy-matrix.yml +++ b/.github/workflows/reusable-strategy-matrix.yml @@ -26,7 +26,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Set up Python - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 with: python-version: "3.13" From d1ff948244cb8b3027a74851466e8816f7940a90 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Wed, 1 Jul 2026 14:30:03 +0100 Subject: [PATCH 7/9] test: Add tests for TMProofPathResponse and TMReplayDeltaResponse invalid hash/key sizes (#7593) --- src/test/app/LedgerReplay_test.cpp | 62 ++++++++++++++++++++++++++++++ src/test/basics/base_uint_test.cpp | 18 +++++++++ 2 files changed, 80 insertions(+) diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index ed99817f34..da98c03380 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -982,6 +982,46 @@ struct LedgerReplayer_test : public beast::unit_test::Suite BEAST_EXPECT(!reply->has_error()); BEAST_EXPECT(server.msgHandler.processProofPathResponse(reply)); + { + // bad reply: invalid hash/key sizes + { + // reply with undersized ledgerhash (31 bytes) + auto bad = std::make_shared(*reply); + bad->set_ledgerhash(std::string(31, '\x01')); + BEAST_EXPECT(!server.msgHandler.processProofPathResponse(bad)); + } + { + // reply with oversized ledgerhash (33 bytes) + auto bad = std::make_shared(*reply); + bad->set_ledgerhash(std::string(33, '\x01')); + BEAST_EXPECT(!server.msgHandler.processProofPathResponse(bad)); + } + { + // reply with empty ledgerhash + auto bad = std::make_shared(*reply); + bad->set_ledgerhash(std::string()); + BEAST_EXPECT(!server.msgHandler.processProofPathResponse(bad)); + } + { + // reply with undersized key (31 bytes) + auto bad = std::make_shared(*reply); + bad->set_key(std::string(31, '\x01')); + BEAST_EXPECT(!server.msgHandler.processProofPathResponse(bad)); + } + { + // reply with oversized key (33 bytes) + auto bad = std::make_shared(*reply); + bad->set_key(std::string(33, '\x01')); + BEAST_EXPECT(!server.msgHandler.processProofPathResponse(bad)); + } + { + // reply with empty key + auto bad = std::make_shared(*reply); + bad->set_key(std::string()); + BEAST_EXPECT(!server.msgHandler.processProofPathResponse(bad)); + } + } + { // bad reply // bad header @@ -1031,6 +1071,28 @@ struct LedgerReplayer_test : public beast::unit_test::Suite BEAST_EXPECT(!reply->has_error()); BEAST_EXPECT(server.msgHandler.processReplayDeltaResponse(reply)); + { + // bad reply: invalid hash sizes + { + // reply with undersized ledgerhash (31 bytes) + auto bad = std::make_shared(*reply); + bad->set_ledgerhash(std::string(31, '\x01')); + BEAST_EXPECT(!server.msgHandler.processReplayDeltaResponse(bad)); + } + { + // reply with oversized ledgerhash (33 bytes) + auto bad = std::make_shared(*reply); + bad->set_ledgerhash(std::string(33, '\x01')); + BEAST_EXPECT(!server.msgHandler.processReplayDeltaResponse(bad)); + } + { + // reply with empty ledgerhash + auto bad = std::make_shared(*reply); + bad->set_ledgerhash(std::string()); + BEAST_EXPECT(!server.msgHandler.processReplayDeltaResponse(bad)); + } + } + { // bad reply // bad header diff --git a/src/test/basics/base_uint_test.cpp b/src/test/basics/base_uint_test.cpp index 5816b4eb59..c6572a0028 100644 --- a/src/test/basics/base_uint_test.cpp +++ b/src/test/basics/base_uint_test.cpp @@ -117,11 +117,29 @@ struct base_uint_test : beast::unit_test::Suite } } + void + testFromRawSizeMismatch() + { + testcase("base_uint: fromRaw size mismatch"); + + // Container larger than the base_uint (16 bytes vs 12 bytes for test96). + // Only the first 12 bytes are copied; the extra bytes are ignored. + { + Blob const tooBig{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; + test96 const result = test96::fromRaw(tooBig); + BEAST_EXPECT(to_string(result) == "0102030405060708090A0B0C"); + } + } + void run() override { testcase("base_uint: general purpose tests"); +#ifdef NDEBUG + testFromRawSizeMismatch(); +#endif + static_assert(!std::is_constructible_v>); static_assert(!std::is_assignable_v>); From 0d149ba5b621d7ae6210c8ed005c3109a4782413 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:04:38 +0200 Subject: [PATCH 8/9] fix: Disable AMM creation with Vault shares (#7666) --- include/xrpl/ledger/helpers/MPTokenHelpers.h | 9 + include/xrpl/ledger/helpers/TokenHelpers.h | 32 +- src/libxrpl/ledger/helpers/TokenHelpers.cpp | 61 +- src/libxrpl/tx/invariants/MPTInvariant.cpp | 25 +- src/libxrpl/tx/transactors/dex/AMMCreate.cpp | 17 + src/test/app/AMMMPT_test.cpp | 176 +----- src/test/app/Invariants_test.cpp | 217 ++----- src/test/app/Vault_test.cpp | 619 ++++++++++--------- 8 files changed, 495 insertions(+), 661 deletions(-) diff --git a/include/xrpl/ledger/helpers/MPTokenHelpers.h b/include/xrpl/ledger/helpers/MPTokenHelpers.h index c709badab8..b7f87337cf 100644 --- a/include/xrpl/ledger/helpers/MPTokenHelpers.h +++ b/include/xrpl/ledger/helpers/MPTokenHelpers.h @@ -23,6 +23,15 @@ namespace xrpl { [[nodiscard]] bool isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue); +/** Returns true if @p account's MPToken for @p mptIssue carries the + * individual-lock flag (lsfMPTLocked). + * + * @warning This checks only the raw per-holder lock bit. It does **not** + * perform the transitive vault pseudo-account check: if @p mptIssue is a + * vault share whose underlying asset is frozen, this function returns false. + * Call @ref isFrozen instead when determining whether an account may send or + * receive tokens — it combines isIndividualFrozen, isGlobalFrozen, and + * isVaultPseudoAccountFrozen into a single complete check. */ [[nodiscard]] bool isIndividualFrozen(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue); diff --git a/include/xrpl/ledger/helpers/TokenHelpers.h b/include/xrpl/ledger/helpers/TokenHelpers.h index 0c9871cd76..c8a67f776a 100644 --- a/include/xrpl/ledger/helpers/TokenHelpers.h +++ b/include/xrpl/ledger/helpers/TokenHelpers.h @@ -144,19 +144,18 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& ass * * Otherwise checks, in order: * 1. If the asset is globally frozen the remaining checks are redundant. - * 2. For MPT shares: The pseudo-account's vault share must not be transitively frozen via its - * underlying asset. - * 3. The pseudo-account's trustline / MPToken must not be frozen for sending. - * 4. Skipped when submitter == dst (self-withdrawal); a regular freeze should not prevent - * recovering one's own funds. - * 5. The destination must not be deep-frozen (cannot receive under any circumstance). + * 2. The pseudo-account's trustline / MPToken must not be individually frozen for sending. + * 3. The submitter's trustline / MPToken must not be individually frozen. Skipped when + * submitter == dst (self-withdrawal) so a regular freeze does not prevent recovering one's own + * funds. (Enforced as defensive code; no current caller exercises a frozen submitter ≠ dst.) + * 4. The destination must not be deep-frozen. * - * For IOUs a regular individual freeze on the withdrawer does NOT block self-withdrawal; only deep - * freeze does. For MPTs "locked" is equivalent to deep-frozen, so locked MPT holders are always + * For IOUs a regular individual freeze on the submitter does NOT block self-withdrawal; only deep + * freeze does. For MPTs "locked" is equivalent to deep-frozen, so locked MPT holders are always * blocked. * * @param view Ledger view to read freeze state from. - * @param srcAcct Pseudo-account the funds are withdrawn from (sender). + * @param pseudoAcct Pseudo-account the funds are withdrawn from (sender). * @param submitterAcct Account that submitted the withdrawal transaction. * @param dstAcct Account receiving the withdrawn funds. * @param asset Asset being withdrawn. @@ -166,7 +165,7 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& ass [[nodiscard]] TER checkWithdrawFreeze( ReadView const& view, - AccountID const& srcAcct, + AccountID const& pseudoAcct, AccountID const& submitterAcct, AccountID const& dstAcct, Asset const& asset); @@ -175,20 +174,17 @@ checkWithdrawFreeze( * Checks freeze compliance for depositing an asset into a pseudo-account (e.g. Vault, AMM, * LoanBroker). * - * * Checks, in order: * 1. If the asset is globally frozen the remaining checks are redundant. - * 2. For MPT shares: the pseudo-account's vault share must not be transitively frozen via its - * underlying asset (returns tecLOCKED). - * 3. The depositor must not be individually frozen. Skipped when srcAcct is the asset issuer, - * since the issuer can always send its own asset. - * 4. The pseudo-account must not be individually frozen for the asset. Unlike regular accounts, + * 2. The depositor must not be individually frozen for the asset. Skipped when srcAcct is the + * asset issuer, since the issuer can always send its own asset. + * 3. The pseudo-account must not be individually frozen for the asset. Unlike regular accounts, * pseudo-accounts cannot receive deposits under a regular freeze because the deposited funds * could not later be withdrawn. * * @param view Ledger view to read freeze state from. * @param srcAcct Depositor sending the funds. - * @param dstAcct Pseudo-account receiving the deposit. + * @param pseudoAcct Pseudo-account receiving the deposit. * @param asset Asset being deposited. * @return tesSUCCESS if the deposit is permitted, otherwise a freeze result * (tecFROZEN for IOUs, tecLOCKED for MPTs). @@ -197,7 +193,7 @@ checkWithdrawFreeze( checkDepositFreeze( ReadView const& view, AccountID const& srcAcct, - AccountID const& dstAcct, + AccountID const& pseudoAcct, Asset const& asset); //------------------------------------------------------------------------------ diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index 7988e24a56..0ab97a4b60 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -160,19 +160,24 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& ass [[nodiscard]] TER checkWithdrawFreeze( ReadView const& view, - AccountID const& srcAcct, + AccountID const& pseudoAcct, AccountID const& submitterAcct, AccountID const& dstAcct, Asset const& asset) { XRPL_ASSERT( - isPseudoAccount(view, srcAcct), "xrpl::checkWithdrawFreeze : source is a pseudo-account"); + isPseudoAccount(view, pseudoAcct), + "xrpl::checkWithdrawFreeze : source is a pseudo-account"); XRPL_ASSERT( !isPseudoAccount(view, submitterAcct), "xrpl::checkWithdrawFreeze : submitter is not a pseudo-account"); XRPL_ASSERT( !isPseudoAccount(view, dstAcct), "xrpl::checkWithdrawFreeze : destination is not a pseudo-account"); + // The asset being withdrawn must not be issued by a pseudo-account + XRPL_ASSERT( + !isPseudoAccount(view, asset.getIssuer()), + "xrpl::checkWithdrawFreeze : asset issuer cannot be a pseudo-account"); // Funds can always be sent to the issuer if (dstAcct == asset.getIssuer()) @@ -182,16 +187,9 @@ checkWithdrawFreeze( if (auto const ret = checkGlobalFrozen(view, asset); !isTesSuccess(ret)) return ret; - // Special case for shares - check if the shares (and the transitive asset) is not frozen - if (asset.holds() && - isVaultPseudoAccountFrozen(view, srcAcct, asset.get(), 0)) - { - return tecLOCKED; - } - // The transfer is from Submitter to Destination via Source (pseudo-account) // Both Source and Submitter must not be frozen to allow sending funds - if (auto const ret = checkIndividualFrozen(view, srcAcct, asset); !isTesSuccess(ret)) + if (auto const ret = checkIndividualFrozen(view, pseudoAcct, asset); !isTesSuccess(ret)) return ret; // Check submitter's individual freeze only when Submitter != Destination (a regular freeze @@ -203,33 +201,42 @@ checkWithdrawFreeze( } // The destination account must not be deep frozen to receive the funds - return checkDeepFrozen(view, dstAcct, asset); + if (auto const ret = checkDeepFrozen(view, dstAcct, asset); !isTesSuccess(ret)) + return ret; + + if (asset.holds() && + isVaultPseudoAccountFrozen(view, pseudoAcct, asset.get(), 0)) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::checkWithdrawFreeze : pseudo-account backed object holds shares"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } + + return tesSUCCESS; } [[nodiscard]] TER checkDepositFreeze( ReadView const& view, AccountID const& srcAcct, - AccountID const& dstAcct, + AccountID const& pseudoAcct, Asset const& asset) { XRPL_ASSERT( - isPseudoAccount(view, dstAcct), + isPseudoAccount(view, pseudoAcct), "xrpl::checkDepositFreeze : destination is a pseudo-account"); XRPL_ASSERT( !isPseudoAccount(view, srcAcct), "xrpl::checkDepositFreeze : source is not a pseudo-account"); + // The asset being deposited must not be issued by a pseudo-account + XRPL_ASSERT( + !isPseudoAccount(view, asset.getIssuer()), + "xrpl::checkDepositFreeze : asset issuer cannot be a pseudo-account"); if (auto const ret = checkGlobalFrozen(view, asset); !isTesSuccess(ret)) return ret; - // Special case for shares - check if the shares and the transitive asset is not frozen - if (asset.holds() && - isVaultPseudoAccountFrozen(view, dstAcct, asset.get(), 0)) - { - return tecLOCKED; - } - if (srcAcct != asset.getIssuer()) { if (auto const ret = checkIndividualFrozen(view, srcAcct, asset); !isTesSuccess(ret)) @@ -238,7 +245,19 @@ checkDepositFreeze( // Unlike regular accounts, pseudo-accounts cannot receive deposits under a regular freeze // because those funds cannot be later withdrawn - return checkIndividualFrozen(view, dstAcct, asset); + if (auto const ret = checkIndividualFrozen(view, pseudoAcct, asset); !isTesSuccess(ret)) + return ret; + + if (asset.holds() && + isVaultPseudoAccountFrozen(view, pseudoAcct, asset.get(), 0)) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::checkDepositFreeze : pseudo-account backed object holds shares"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } + + return tesSUCCESS; } //------------------------------------------------------------------------------ diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 26bee4effb..d8f7fcc27d 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -837,8 +836,6 @@ ValidMPTTransfer::finalize( ReadView const& view, beast::Journal const& j) { - auto const fix330Enabled = view.rules().enabled(fixCleanup3_3_0); - if (hasPrivilege(tx, OverrideFreeze)) return true; @@ -901,27 +898,9 @@ ValidMPTTransfer::finalize( // Check once: if any involved account is frozen, the whole issuance transfer is // considered frozen. Only need to check for frozen if there is a transfer of funds. - // - // Post-fix330: full isFrozen() applies — vault-share transitive freeze is part of - // the freeze semantics for all changed holders. - // - // Pre-fix330: legacy AMM withdraw only checked individual freeze on the - // destination, not the transitive vault freeze. All other paths (and the AMM - // account itself as sender) did apply the full check. - MPTIssue const issue{mptID}; - auto const legacyAccountFrozen = [&] { - if (isGlobalFrozen(view, issue) || isIndividualFrozen(view, account, issue)) - return true; - bool const isReceiver = - !value.amtBefore.has_value() || *value.amtAfter > *value.amtBefore; - if (txnType == ttAMM_WITHDRAW && isReceiver) - return false; - return isVaultPseudoAccountFrozen(view, account, issue, 0); - }; - bool const accountFrozen = - fix330Enabled ? isFrozen(view, account, issue) : legacyAccountFrozen(); if (!invalidTransfer && - (accountFrozen || !isAuthorized(view, mptID, account, reqAuth))) + (isFrozen(view, account, MPTIssue{mptID}) || + !isAuthorized(view, mptID, account, reqAuth))) { invalidTransfer = true; } diff --git a/src/libxrpl/tx/transactors/dex/AMMCreate.cpp b/src/libxrpl/tx/transactors/dex/AMMCreate.cpp index ef271ba362..66d059c54d 100644 --- a/src/libxrpl/tx/transactors/dex/AMMCreate.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMCreate.cpp @@ -193,6 +193,23 @@ AMMCreate::preclaim(PreclaimContext const& ctx) pseudoAccountAddress(ctx.view, keylet::amm(amount.asset(), amount2.asset()).key); accountId == beast::kZero) return terADDRESS_COLLISION; + + auto const isMPTIssuerPseudo = [&](Asset const& asset) { + if (asset.native()) + return false; + + if (asset.holds()) + return false; + + return isPseudoAccount(ctx.view, asset.getIssuer()); + }; + + if (isMPTIssuerPseudo(amount.asset()) || isMPTIssuerPseudo(amount2.asset())) + { + JLOG(ctx.j.debug()) << "AMM Instance: can't create with vault shares " << amount << " " + << amount2; + return tecWRONG_ASSET; + } } if (auto const ter = canMPTTradeAndTransfer(ctx.view, amount.asset(), accountID, accountID); diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index 9ff6c65c17..c68270591a 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -6892,168 +6892,38 @@ private: void testAMMWithVaultShares() { - testcase("AMM with vault shares — underlying freeze blocks share withdrawal"); + testcase("AMM with vault shares not allowed"); using namespace jtx; + // AMMTestBase::testableAmendments() strips featureSingleAssetVault, // but vault shares require it. Use the global jtx set directly. - FeatureBitset const all{jtx::testableAmendments()}; + Env env{*this, envconfig(), jtx::testableAmendments(), nullptr, beast::Severity::Disabled}; - // When alice's underlying asset is individually frozen: - // - // Deposit (post-fixCleanup3_3_0): checkDepositFreeze checks the AMM - // pseudo-account's underlying, not alice's — deposit is allowed. - // Pre-fix: featureAMMClawback calls isFrozen(alice, share) which - // descends via isVaultPseudoAccountFrozen(alice,...) and finds the - // frozen underlying — deposit is blocked. - // - // Withdrawal (post-fixCleanup3_3_0): checkWithdrawFreeze ends with - // checkDeepFrozen(alice, share) which calls isFrozen(alice, share) - // and finds the frozen underlying — withdrawal is blocked. - // Pre-fix: the old path only checks the AMM account's MPToken lock, - // which is unset — withdrawal succeeds. + env.fund(XRP(100'000), gw_, alice_); + env(fset(gw_, asfDefaultRipple)); + env.close(); - auto runIOU = [&](FeatureBitset const& features) { - bool const fix330 = features[fixCleanup3_3_0]; - Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + PrettyAsset const iou = gw_["IOU"]; + env.trust(iou(1'000'000), alice_); + env(pay(gw_, alice_, iou(10'000))); + env.close(); - env.fund(XRP(100'000), gw_, alice_); - env(fset(gw_, asfDefaultRipple)); - env.close(); + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = alice_, .asset = iou}); + env(createTx); + env.close(); - PrettyAsset const iou = gw_["IOU"]; - env.trust(iou(1'000'000), alice_); - env(pay(gw_, alice_, iou(10'000))); - env.close(); + env(vault.deposit({.depositor = alice_, .id = vaultKeylet.key, .amount = iou(200)})); + env.close(); - Vault const vault{env}; - auto [createTx, vaultKeylet] = vault.create({.owner = alice_, .asset = iou}); - env(createTx); - env.close(); + auto const vaultSle = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultSle)) + return; - // 200 IOU → 200,000,000 vault shares (IOU vault scale = 6) - env(vault.deposit({.depositor = alice_, .id = vaultKeylet.key, .amount = iou(200)})); - env.close(); - - auto const shareMPTID = env.le(vaultKeylet)->at(sfShareMPTID); - // Use half the shares for the AMM; alice keeps the other half. - STAmount const shareAmt{MPTIssue{shareMPTID}, 100'000'000}; - // Pool: XRP(100) = 1e8 drops, shares = 1e8 → LP ≈ 1e8 - AMM amm{env, alice_, XRP(100), shareAmt}; - env.close(); - - // Freeze alice's IOU trustline (individual freeze on underlying). - env(trust(gw_, iou(0), alice_, tfSetFreeze)); - env.close(); - - // post-fix330: checkDepositFreeze checks AMM pseudo's underlying - // (not alice's) → deposit is allowed - // pre-fix330: featureAMMClawback path calls isFrozen(alice, share) - // which descends to alice's frozen IOU → tecLOCKED - amm.deposit( - {.account = alice_, - .asset1In = XRP(1), - .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecLOCKED))}); - - // post-fix330: checkWithdrawFreeze → checkDeepFrozen(alice, share) - // descends to alice's frozen IOU → tecLOCKED - // pre-fix330: the AMM pseudo-account is not authorized for the - // share's underlying (requireAuth recurses share→IOU - // and the AMM holds no IOU trustline), so - // accountHolds(ZeroIfUnauthorized) reports the pool's - // share balance as 0 and the withdrawal math fails → - // tecAMM_FAILED. Vault shares deposited into an AMM are - // only withdrawable once fixCleanup3_3_0 exempts the - // pseudo-account from the recursive auth check. - amm.withdraw( - {.account = alice_, - .tokens = 1'000, - .err = Ter(fix330 ? TER(tecLOCKED) : TER(tecAMM_FAILED))}); - - env(trust(gw_, iou(0), alice_, tfClearFreeze)); - env.close(); - - // Lifting the freeze lets the deposit through in both cases. The - // withdrawal only succeeds post-fix330; pre-fix330 the share balance - // remains inaccessible to the unauthorized pseudo-account, so the - // shares stay stuck → tecAMM_FAILED. - amm.deposit({.account = alice_, .asset1In = XRP(1)}); - amm.withdraw( - {.account = alice_, - .tokens = 1'000, - .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecAMM_FAILED))}); - }; - - runIOU(all); - runIOU(all - fixCleanup3_3_0); - - auto runMPT = [&](FeatureBitset const& features) { - bool const fix330 = features[fixCleanup3_3_0]; - // Expected freeze failures fire invariant checks that log at Error; - // silence them so the test output stays clean. - Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; - - env.fund(XRP(100'000), gw_, alice_); - env.close(); - - MPTTester mptt{env, gw_, kMptInitNoFund}; - mptt.create({.flags = kMptDexFlags | tfMPTCanLock}); - PrettyAsset const mpt = mptt.issuanceID(); - mptt.authorize({.account = alice_}); - env(pay(gw_, alice_, mpt(30'000))); - env.close(); - - Vault const vault{env}; - auto [createTx, vaultKeylet] = vault.create({.owner = alice_, .asset = mpt}); - env(createTx); - env.close(); - - // 20000 MPT → 20000 vault shares (MPT vault scale = 0) - env(vault.deposit({.depositor = alice_, .id = vaultKeylet.key, .amount = mpt(20'000)})); - env.close(); - - auto const shareMPTID = env.le(vaultKeylet)->at(sfShareMPTID); - // Use half the shares for the AMM; alice keeps the other half. - // Pool: XRP(100) = 1e8 drops, shares = 10000 → LP ≈ 1e6 - STAmount const shareAmt{MPTIssue{shareMPTID}, 10'000}; - AMM amm{env, alice_, XRP(100), shareAmt}; - env.close(); - - // Lock alice's underlying MPT (individual lock). - mptt.set({.holder = alice_, .flags = tfMPTLock}); - - // Same pre/post-fix330 semantics as the IOU case above. - amm.deposit( - {.account = alice_, - .asset1In = XRP(1), - .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecLOCKED))}); - - // {.tokens = 1'000} → frac = 1000/1e6 = 0.001 - // XRP out = 1e8 * 0.001 = 1e5 drops, shares out = 10000 * 0.001 = 10 - // post-fix330: checkWithdrawFreeze sees alice's locked underlying - // MPT via the share → tecLOCKED. - // pre-fix330: the AMM pseudo-account is unauthorized for the - // share's underlying MPT (it holds no underlying - // MPToken), so accountHolds(ZeroIfUnauthorized) zeros - // the pool's share balance and the math fails → - // tecAMM_FAILED. - amm.withdraw( - {.account = alice_, - .tokens = 1'000, - .err = Ter(fix330 ? TER(tecLOCKED) : TER(tecAMM_FAILED))}); - - mptt.set({.holder = alice_, .flags = tfMPTUnlock}); - - // Unlocking lets the deposit through; the withdrawal only succeeds - // post-fix330 (pre-fix330 the shares remain stuck → tecAMM_FAILED). - amm.deposit({.account = alice_, .asset1In = XRP(1)}); - amm.withdraw( - {.account = alice_, - .tokens = 1'000, - .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecAMM_FAILED))}); - }; - - runMPT(all); - runMPT(all - fixCleanup3_3_0); + auto const shareMPTID = vaultSle->at(sfShareMPTID); + STAmount const shareAmt{MPTIssue{shareMPTID}, 100'000'000}; + AMM const amm{env, alice_, XRP(100), shareAmt, Ter(tecWRONG_ASSET)}; + env.close(); } void diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 079ff9df88..79fdbb54ed 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -65,6 +64,7 @@ #include #include #include +#include #include #include @@ -4606,15 +4606,17 @@ class Invariants_test : public beast::unit_test::Suite } } - // Vault-share transfer: ValidMPTTransfer gates isVaultPseudoAccountFrozen - // on fixCleanup3_3_0. Pre-amendment, vault-share transfers are allowed - // even when the underlying asset is individually frozen for the sender; - // post-amendment they are blocked. + // Vault-share freeze invariant: isVaultPseudoAccountFrozen descends + // through sfReferenceHolding to test the vault's underlying asset for + // each changed holder. { Account const gw{"gw"}; MPTID shareID{}; - auto const preclose = [&](Account const& a1, Account const& a2, Env& env) -> bool { + // Vault setup: a1 and a2 both deposit IOU and hold vault shares. + auto const setupVault = [&](Account const& a1, + Account const& a2, + Env& env) -> std::tuple { env.fund(XRP(1'000), gw); env.trust(gw["IOU"](10'000), a1); env.trust(gw["IOU"](10'000), a2); @@ -4623,25 +4625,17 @@ class Invariants_test : public beast::unit_test::Suite env(pay(gw, a2, gw["IOU"](500))); env.close(); - PrettyAsset const iou = gw["IOU"]; Vault const vault{env}; - auto [createTx, vaultKeylet] = vault.create({.owner = a1, .asset = iou}); + auto [createTx, vaultKeylet] = vault.create({.owner = a1, .asset = gw["IOU"]}); env(createTx); env.close(); - // Both a1 and a2 deposit IOU, each receiving vault shares. - env(vault.deposit({.depositor = a1, .id = vaultKeylet.key, .amount = iou(100)})); - env(vault.deposit({.depositor = a2, .id = vaultKeylet.key, .amount = iou(100)})); + env(vault.deposit( + {.depositor = a1, .id = vaultKeylet.key, .amount = gw["IOU"](100)})); + env(vault.deposit( + {.depositor = a2, .id = vaultKeylet.key, .amount = gw["IOU"](100)})); env.close(); - shareID = env.le(vaultKeylet)->at(sfShareMPTID); - - // Freeze a2's IOU trustline from the issuer side. - // a2 is the receiver in the simulated AMM withdraw; the - // distinction under test is that pre-fix330 the invariant - // does not apply the transitive vault freeze to receivers. - env(trust(gw, gw["IOU"](0), a2, tfSetFreeze)); - env.close(); - return true; + return {env.le(vaultKeylet)->at(sfShareMPTID), env.le(vaultKeylet)->at(sfAccount)}; }; // Simulate a vault-share transfer: a1 sends 10 shares to a2. @@ -4658,156 +4652,45 @@ class Invariants_test : public beast::unit_test::Suite return true; }; - // post-fixCleanup3_3_0: full isFrozen() applies to all holders; - // isVaultPseudoAccountFrozen finds a2's underlying IOU frozen → - // invalidTransfer → invariant fires. - doInvariantCheck( - Env{*this, defaultAmendments()}, - {{"invalid MPToken transfer between holders"}}, - precheck, - XRPAmount{}, - STTx{ttAMM_WITHDRAW, [](STObject&) {}}, - {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, - preclose); + // Case: vault pseudo-account's IOU trustline is frozen. + { + auto const preclose = [&](Account const& a1, Account const& a2, Env& env) -> bool { + auto [sid, vid] = setupVault(a1, a2, env); + shareID = sid; + env(trust(gw, gw["IOU"](0), Account{"vaultPseudo", vid}, tfSetFreeze)); + env.close(); + return true; + }; - // pre-fixCleanup3_3_0: legacy AMM withdraw only checked - // checkIndividualFrozen on the destination, not the transitive - // vault freeze; a2 as receiver is exempt → invariant passes. - doInvariantCheck( - Env{*this, defaultAmendments() - fixCleanup3_3_0}, - {}, - precheck, - XRPAmount{}, - STTx{ttAMM_WITHDRAW, [](STObject&) {}}, - {tesSUCCESS, tesSUCCESS}, - preclose); - } + doInvariantCheck( + Env{*this, defaultAmendments()}, + {{"invalid MPToken transfer between holders"}}, + precheck, + XRPAmount{}, + STTx{ttPAYMENT, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + preclose); + } - // Side-specific vault-share AMM_WITHDRAW invariant tests. - // Both cases use a real vault (IOU underlying) and a real AMM whose - // pool includes vault shares. precheck simulates an AMM_WITHDRAW by - // transferring 10 vault shares from the AMM pseudo-account to a2. - { - MPTID shareID{}; - AccountID ammAcctID{}; - AccountID vaultPseudoID{}; - Account const gw{"gw"}; + // Case: receiver's (a2's) IOU trustline is frozen. + { + auto const preclose = [&](Account const& a1, Account const& a2, Env& env) -> bool { + auto [sid, vid] = setupVault(a1, a2, env); + shareID = sid; + env(trust(gw, gw["IOU"](0), a2, tfSetFreeze)); + env.close(); + return true; + }; - // Simulate AMM_WITHDRAW: AMM pseudo-account sends 10 vault shares - // to a2. The AMM pseudo is the sender (decreasing balance); - // a2 is the receiver (increasing balance). - auto const precheck2 = - [&](Account const& /*a1*/, Account const& a2, ApplyContext& ac) -> bool { - auto sleAMM = ac.view().peek(keylet::mptoken(shareID, ammAcctID)); - auto sle2 = ac.view().peek(keylet::mptoken(shareID, a2.id())); - if (!sleAMM || !sle2) - return false; - (*sleAMM)[sfMPTAmount] -= 10; - (*sle2)[sfMPTAmount] += 10; - ac.view().update(sleAMM); - ac.view().update(sle2); - return true; - }; - - // Shared vault + AMM setup: a1 deposits 500 IOU into a vault and - // creates an AMM with XRP + 100 vault shares, giving the AMM - // pseudo-account a vault-share MPToken balance. - auto const setupVaultAMM = [&](Account const& a1, Account const& a2, Env& env) -> bool { - env.fund(XRP(1'000), gw); - env(fset(gw, asfDefaultRipple)); - env.close(); - - env.trust(gw["IOU"](10'000), a1); - env.trust(gw["IOU"](10'000), a2); - env.close(); - env(pay(gw, a1, gw["IOU"](1'000))); - env(pay(gw, a2, gw["IOU"](500))); - env.close(); - - Vault const vault{env}; - auto [createTx, vaultKeylet] = vault.create({.owner = a1, .asset = gw["IOU"]}); - env(createTx); - env.close(); - - env(vault.deposit( - {.depositor = a1, .id = vaultKeylet.key, .amount = gw["IOU"](500)})); - env(vault.deposit( - {.depositor = a2, .id = vaultKeylet.key, .amount = gw["IOU"](200)})); - env.close(); - - shareID = env.le(vaultKeylet)->at(sfShareMPTID); - vaultPseudoID = env.le(vaultKeylet)->at(sfAccount); - - // a1 creates AMM with XRP + 100 vault shares; the AMM - // pseudo-account receives an MPToken record for shareID. - AMM const amm(env, a1, XRP(100), STAmount{MPTIssue{shareID}, 100}); - ammAcctID = amm.ammAccount(); - return true; - }; - - // Case 1: freeze the vault pseudo-account's IOU trustline. - // isVaultPseudoAccountFrozen(ammAcct) calls isAnyFrozen({vaultPseudo, - // ammAcct}, IOU); since vaultPseudo is frozen it returns true. The - // AMM sender has a decreasing balance (not a receiver) so it is - // never exempt from the check — invariant fires both pre- and - // post-fixCleanup3_3_0. - auto const preclose3 = [&](Account const& a1, Account const& a2, Env& env) -> bool { - if (!setupVaultAMM(a1, a2, env)) - return false; - env(trust(gw, gw["IOU"](0), Account{"vaultPseudo", vaultPseudoID}, tfSetFreeze)); - env.close(); - return true; - }; - - doInvariantCheck( - Env{*this, defaultAmendments()}, - {{"invalid MPToken transfer between holders"}}, - precheck2, - XRPAmount{}, - STTx{ttAMM_WITHDRAW, [](STObject&) {}}, - {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, - preclose3); - - doInvariantCheck( - Env{*this, defaultAmendments() - fixCleanup3_3_0}, - {{"invalid MPToken transfer between holders"}}, - precheck2, - XRPAmount{}, - STTx{ttAMM_WITHDRAW, [](STObject&) {}}, - {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, - preclose3); - - // Case 2: freeze a2's (receiver's) IOU trustline. - // isVaultPseudoAccountFrozen(a2) → isAnyFrozen({vaultPseudo, a2}, - // IOU) → true. The AMM sender's check passes (vaultPseudo and - // ammAcct are not frozen). Pre-fix330: receiver is exempt from - // isVaultPseudoAccountFrozen in ttAMM_WITHDRAW → passes. - // Post-fix330: full isFrozen() applied to a2 → fires. - auto const preclose4 = [&](Account const& a1, Account const& a2, Env& env) -> bool { - if (!setupVaultAMM(a1, a2, env)) - return false; - env(trust(gw, gw["IOU"](0), a2, tfSetFreeze)); - env.close(); - return true; - }; - - doInvariantCheck( - Env{*this, defaultAmendments()}, - {{"invalid MPToken transfer between holders"}}, - precheck2, - XRPAmount{}, - STTx{ttAMM_WITHDRAW, [](STObject&) {}}, - {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, - preclose4); - - doInvariantCheck( - Env{*this, defaultAmendments() - fixCleanup3_3_0}, - {}, - precheck2, - XRPAmount{}, - STTx{ttAMM_WITHDRAW, [](STObject&) {}}, - {tesSUCCESS, tesSUCCESS}, - preclose4); + doInvariantCheck( + Env{*this, defaultAmendments()}, + {{"invalid MPToken transfer between holders"}}, + precheck, + XRPAmount{}, + STTx{ttPAYMENT, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + preclose); + } } } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index d9afd93b23..f6574872f9 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -1634,8 +1634,6 @@ class Vault_test : public beast::unit_test::Suite env(tx, Ter(tecNO_ENTRY)); }); - // Freeze/lock tests are in testVaultDepositFreeze/testVaultWithdrawFreeze - testCase([this]( Env& env, Account const& issuer, @@ -2238,11 +2236,6 @@ class Vault_test : public beast::unit_test::Suite env(offer(alice, XRP(1), shares(1)), Ter{tecNO_PERMISSION}); env.close(); - // The inherited CanTrade restriction also blocks AMM creation. - AMM const ammUnderlyingFail( - env, alice, XRP(1'000), asset(1'000), Ter{tecNO_PERMISSION}); - AMM const ammShares(env, alice, XRP(1'000), shares(100), Ter{tecNO_PERMISSION}); - // Deposit still works before enabling CanTrade. env(vault.deposit({.depositor = alice, .id = keylet.key, .amount = asset(100)})); env.close(); @@ -7580,185 +7573,212 @@ class Vault_test : public beast::unit_test::Suite } void - testVaultDepositFreeze() + testVaultDepositFreezeIOU() { using namespace test::jtx; + testcase("VaultDeposit IOU freeze checks"); Account const issuer{"issuer"}; Account const owner{"owner"}; + Env env{*this}; + Vault vault{env}; - // === IOU === - { - testcase("VaultDeposit IOU freeze checks"); - Env env{*this}; - Vault vault{env}; + env.fund(XRP(100'000), issuer, owner); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1'000'000), owner); + env(pay(issuer, owner, asset(100'000))); + env.close(); - env.fund(XRP(100'000), issuer, owner); - env(fset(issuer, asfAllowTrustLineClawback)); - env.close(); - PrettyAsset const asset = issuer["IOU"]; - env.trust(asset(1'000'000), owner); - env(pay(issuer, owner, asset(100'000))); - env.close(); + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - env(tx); - env.close(); - auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); + // Initial deposit so the vault pseudo-account has a trustline + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(100)})); + env.close(); - // Initial deposit so the vault pseudo-account has a trustline - env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(100)})); - env.close(); + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); - auto runTests = [&]() { - auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); - - // Global freeze + // Global freeze + { + testcase("VaultDeposit IOU global freeze"); env(fset(issuer, asfGlobalFreeze)); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), Ter(tecFROZEN)); env(fclear(issuer, asfGlobalFreeze)); + } - // Depositor regular freeze + // Depositor freeze + { + testcase("VaultDeposit IOU depositor freeze"); env(trust(issuer, asset(0), owner, tfSetFreeze)); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), Ter(tecFROZEN)); env(trust(issuer, asset(0), owner, tfClearFreeze)); + } - // Depositor deep freeze + // Depositor deep freeze + { + testcase("VaultDeposit IOU depositor deep freeze"); env(trust(issuer, asset(0), owner, tfSetFreeze | tfSetDeepFreeze)); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), Ter(tecFROZEN)); env(trust(issuer, asset(0), owner, tfClearFreeze | tfClearDeepFreeze)); + } - // Vault-account regular freeze - // Post-fix: checkDepositFreeze catches it → tecFROZEN - // Pre-fix: not checked directly, but the transitive share - // check triggers → tecLOCKED - { - auto trustSet = [&]() { - json::Value jv; - jv[jss::Account] = issuer.human(); - { - auto& ja = jv[jss::LimitAmount] = - asset(0).value().getJson(JsonOptions::Values::None); - ja[jss::issuer] = toBase58(vaultAcct.id()); - } - jv[jss::TransactionType] = jss::TrustSet; - return jv; - }(); + // Vault-account freeze + // Post-fix: checkDepositFreeze catches it → tecFROZEN + // Pre-fix: not checked directly, but the transitive share + // check triggers → tecLOCKED + { + testcase("VaultDeposit IOU pseudo-account freeze"); + auto trustSet = [&]() { + json::Value jv; + jv[jss::Account] = issuer.human(); + { + auto& ja = jv[jss::LimitAmount] = + asset(0).value().getJson(JsonOptions::Values::None); + ja[jss::issuer] = toBase58(vaultAcct.id()); + } + jv[jss::TransactionType] = jss::TrustSet; + return jv; + }(); - trustSet[jss::Flags] = tfSetFreeze; - env(trustSet); - env.close(); + trustSet[jss::Flags] = tfSetFreeze; + env(trustSet); + env.close(); - TER const expected = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); - env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), - Ter(expected)); + TER const expected = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(expected)); - trustSet[jss::Flags] = tfClearFreeze; - env(trustSet); - env.close(); - } + trustSet[jss::Flags] = tfClearFreeze; + env(trustSet); + env.close(); + } - // Vault-account deep freeze - { - auto trustSet = [&]() { - json::Value jv; - jv[jss::Account] = issuer.human(); - { - auto& ja = jv[jss::LimitAmount] = - asset(0).value().getJson(JsonOptions::Values::None); - ja[jss::issuer] = toBase58(vaultAcct.id()); - } - jv[jss::TransactionType] = jss::TrustSet; - return jv; - }(); + // Vault-account deep freeze + { + testcase("VaultDeposit IOU pseudo-account deep freeze"); + auto trustSet = [&]() { + json::Value jv; + jv[jss::Account] = issuer.human(); + { + auto& ja = jv[jss::LimitAmount] = + asset(0).value().getJson(JsonOptions::Values::None); + ja[jss::issuer] = toBase58(vaultAcct.id()); + } + jv[jss::TransactionType] = jss::TrustSet; + return jv; + }(); - trustSet[jss::Flags] = tfSetFreeze | tfSetDeepFreeze; - env(trustSet); - env.close(); + trustSet[jss::Flags] = tfSetFreeze | tfSetDeepFreeze; + env(trustSet); + env.close(); - env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), - Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED))); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED))); - trustSet[jss::Flags] = tfClearFreeze | tfClearDeepFreeze; - env(trustSet); - env.close(); - } + trustSet[jss::Flags] = tfClearFreeze | tfClearDeepFreeze; + env(trustSet); + env.close(); + } - // Clawback works while frozen + // Clawback works while frozen + { + testcase("VaultDeposit IOU freeze clawback unaffected"); env(fset(issuer, asfGlobalFreeze)); env(vault.clawback( {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(1)})); env(fclear(issuer, asfGlobalFreeze)); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); env.close(); - }; + } + }; - runTests(); - env.disableFeature(fixCleanup3_3_0); - runTests(); - env.enableFeature(fixCleanup3_3_0); - } + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } - // === MPT === - { - testcase("VaultDeposit MPT lock checks"); - Env env{*this}; - Vault vault{env}; + void + testVaultDepositFreezeMPT() + { + using namespace test::jtx; + testcase("VaultDeposit MPT lock checks"); - env.fund(XRP(100'000), issuer, owner); - env.close(); + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Env env{*this}; + Vault vault{env}; - MPTTester mptt{env, issuer, kMptInitNoFund}; - mptt.create( - {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth}); - PrettyAsset const mpt{mptt.issuanceID()}; + env.fund(XRP(100'000), issuer, owner); + env.close(); - mptt.authorize({.account = owner}); - mptt.authorize({.account = issuer, .holder = owner}); - env.close(); - env(pay(issuer, owner, mpt(100'000))); - env.close(); + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth}); + PrettyAsset const mpt{mptt.issuanceID()}; - auto [tx, keylet] = vault.create({.owner = owner, .asset = mpt}); - env(tx); - env.close(); - auto const vaultAcctID = env.le(keylet)->at(sfAccount); - Account const vaultAcct("vault", vaultAcctID); + mptt.authorize({.account = owner}); + mptt.authorize({.account = issuer, .holder = owner}); + env.close(); + env(pay(issuer, owner, mpt(100'000))); + env.close(); - env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(100)})); - env.close(); + auto [tx, keylet] = vault.create({.owner = owner, .asset = mpt}); + env(tx); + env.close(); + auto const vaultAcctID = env.le(keylet)->at(sfAccount); + Account const vaultAcct("vault", vaultAcctID); - // For MPT isDeepFrozen == isFrozen, so all locks block in - // both pre- and post-fix. - auto runTests = [&]() { - // Global lock + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(100)})); + env.close(); + + // For MPT isDeepFrozen == isFrozen, so all locks block in + // both pre- and post-fix. + auto runTests = [&]() { + // Global lock + { + testcase("VaultDeposit MPT global lock"); mptt.set({.flags = tfMPTLock}); env.close(); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), Ter(tecLOCKED)); mptt.set({.flags = tfMPTUnlock}); env.close(); + } - // Depositor individual lock + // Depositor individual lock + { + testcase("VaultDeposit MPT depositor lock"); mptt.set({.holder = owner, .flags = tfMPTLock}); env.close(); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), Ter(tecLOCKED)); mptt.set({.holder = owner, .flags = tfMPTUnlock}); env.close(); + } - // Vault pseudo-account individual lock + // Vault pseudo-account individual lock + { + testcase("VaultDeposit MPT pseudo-account lock"); mptt.set({.holder = vaultAcct, .flags = tfMPTLock}); env.close(); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), Ter(tecLOCKED)); mptt.set({.holder = vaultAcct, .flags = tfMPTUnlock}); env.close(); + } - // Clawback works while locked + // Clawback works while locked + { + testcase("VaultDeposit MPT lock clawback unaffected"); mptt.set({.flags = tfMPTLock}); env.close(); env(vault.clawback( @@ -7767,16 +7787,16 @@ class Vault_test : public beast::unit_test::Suite env.close(); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); env.close(); - }; + } + }; - runTests(); - env.disableFeature(fixCleanup3_3_0); - runTests(); - env.enableFeature(fixCleanup3_3_0); - } + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); } - // Focused demonstration: a depositor under a regular individual IOU freeze + // Focused demonstration: a depositor under an individual IOU freeze // can still withdraw to themselves (self-withdrawal), but is blocked from // withdrawing to a third party. // @@ -7818,7 +7838,7 @@ class Vault_test : public beast::unit_test::Suite auto runTests = [&]() { auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); - // Set a regular individual freeze on the owner's IOU trustline. + // Set an individual freeze on the owner's IOU trustline. env(trust(issuer, asset(0), owner, tfSetFreeze)); env.close(); @@ -7850,110 +7870,111 @@ class Vault_test : public beast::unit_test::Suite } void - testVaultWithdrawFreeze() + testVaultWithdrawFreezeIOU() { using namespace test::jtx; + testcase("VaultWithdraw IOU freeze checks"); Account const issuer{"issuer"}; Account const owner{"owner"}; + Env env{*this}; + Vault const vault{env}; - // === IOU === - { - testcase("VaultWithdraw IOU freeze checks"); - Env env{*this}; - Vault vault{env}; + env.fund(XRP(100'000), issuer, owner); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1'000'000), owner); + env(pay(issuer, owner, asset(100'000))); + env.close(); - env.fund(XRP(100'000), issuer, owner); - env(fset(issuer, asfAllowTrustLineClawback)); - env.close(); - PrettyAsset const asset = issuer["IOU"]; - env.trust(asset(1'000'000), owner); - env(pay(issuer, owner, asset(100'000))); - env.close(); + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - env(tx); - env.close(); - auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(100)})); + env.close(); - env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(100)})); - env.close(); + Account const charlie{"charlie"}; + env.fund(XRP(10'000), charlie); + env.trust(asset(1'000'000), charlie); + env.close(); - Account const charlie{"charlie"}; - env.fund(XRP(10'000), charlie); - env.trust(asset(1'000'000), charlie); - env.close(); - - auto runTests = [&]() { - auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); - // Post-fix: submitter freeze blocks withdraw to 3rd party - // Pre-fix: submitter's IOU freeze not checked, but - // checkFrozen(depositor, share) may trigger tecLOCKED - TER const submitterTo3rd = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); - - // Global freeze → self-withdraw + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + // Global freeze → self-withdraw + { + testcase("VaultWithdraw IOU global freeze"); env(fset(issuer, asfGlobalFreeze)); env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), Ter(tecFROZEN)); // Global freeze → withdraw to 3rd party - { - auto withdrawToCharlie = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); - withdrawToCharlie[sfDestination] = charlie.human(); - env(withdrawToCharlie, Ter(tecFROZEN)); - } + + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(tecFROZEN)); + env(fclear(issuer, asfGlobalFreeze)); + } - // Vault-account regular freeze - { - auto trustSet = [&]() { - json::Value jv; - jv[jss::Account] = issuer.human(); - { - auto& ja = jv[jss::LimitAmount] = - asset(0).value().getJson(JsonOptions::Values::None); - ja[jss::issuer] = toBase58(vaultAcct.id()); - } - jv[jss::TransactionType] = jss::TrustSet; - return jv; - }(); - - trustSet[jss::Flags] = tfSetFreeze; - env(trustSet); - env.close(); - - TER const vaultAcctFreeze = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); - - // Self-withdraw - env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), - Ter(vaultAcctFreeze)); - // Withdraw to 3rd party + // Vault-account freeze + { + testcase("VaultWithdraw IOU pseudo-account freeze"); + auto trustSet = [&]() { + json::Value jv; + jv[jss::Account] = issuer.human(); { - auto withdrawToCharlie = vault.withdraw( - {.depositor = owner, .id = keylet.key, .amount = asset(1)}); - withdrawToCharlie[sfDestination] = charlie.human(); - env(withdrawToCharlie, Ter(vaultAcctFreeze)); + auto& ja = jv[jss::LimitAmount] = + asset(0).value().getJson(JsonOptions::Values::None); + ja[jss::issuer] = toBase58(vaultAcct.id()); } + jv[jss::TransactionType] = jss::TrustSet; + return jv; + }(); - trustSet[jss::Flags] = tfClearFreeze; - env(trustSet); - env.close(); - } + trustSet[jss::Flags] = tfSetFreeze; + env(trustSet); + env.close(); - // Depositor regular freeze → self-withdraw + TER const terExpected = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); + + // Self-withdraw + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(terExpected)); + // Withdraw to 3rd party + + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(terExpected)); + + trustSet[jss::Flags] = tfClearFreeze; + env(trustSet); + env.close(); + } + + // Depositor freeze, self-withdraw + { + testcase("VaultWithdraw IOU self-withdraw freeze check"); env(trust(issuer, asset(0), owner, tfSetFreeze)); + // Post-fix: self-withdraw allowed (submitter==dst skip) // Pre-fix: isFrozen(depositor, iou) catches it env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecFROZEN))); - // Depositor regular freeze → withdraw to 3rd party - { - auto withdrawTo3rd = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); - withdrawTo3rd[sfDestination] = charlie.human(); - env(withdrawTo3rd, Ter(submitterTo3rd)); - } + // Depositor freeze withdraw to 3rd party + auto withdrawTo3rd = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawTo3rd[sfDestination] = charlie.human(); + + // Post-fix: submitter freeze blocks withdraw to 3rd party + // Pre-fix: submitter's IOU freeze not checked, but checkFrozen(depositor, + // share) triggers tecLOCKED + env(withdrawTo3rd, Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED))); + env(trust(issuer, asset(0), owner, tfClearFreeze)); // Replenish what was withdrawn if (fix330Enabled) @@ -7961,104 +7982,131 @@ class Vault_test : public beast::unit_test::Suite env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); } env.close(); + } - // Depositor deep freeze → self-withdraw blocked + // Depositor deep freeze → self-withdraw blocked + { + testcase("VaultWithdraw IOU depositor deep freeze"); env(trust(issuer, asset(0), owner, tfSetFreeze | tfSetDeepFreeze)); - // TODO: branches are identical - confirm the intended pre/post-fix330 - // expectations and replace with the correct values (one branch may be wrong). - env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), - // NOLINTNEXTLINE(bugprone-branch-clone) - Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecFROZEN))); - env(trust(issuer, asset(0), owner, tfClearFreeze | tfClearDeepFreeze)); - // Destination regular freeze → withdraw to 3rd party + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(tecFROZEN)); + + env(trust(issuer, asset(0), owner, tfClearFreeze | tfClearDeepFreeze)); + } + + // Destination freeze → withdraw to 3rd party + { + testcase("VaultWithdraw IOU freeze withdraw to 3rd party"); + env(trust(issuer, asset(0), charlie, tfSetFreeze)); + // Self-withdraw unaffected by charlie's freeze env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)})); - { - auto withdrawToCharlie = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); - withdrawToCharlie[sfDestination] = charlie.human(); - // Post-fix: regular freeze on dst allowed - // Pre-fix: checkFrozen(dst, iou) catches it - env(withdrawToCharlie, Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecFROZEN))); - } + + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + + // Post-fix: freeze on dst allowed + // Pre-fix: checkFrozen(dst, iou) catches it + env(withdrawToCharlie, Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecFROZEN))); + env(trust(issuer, asset(0), charlie, tfClearFreeze)); + // Replenish: 1 for self-withdraw + 1 if charlie withdraw succeeded env(vault.deposit( {.depositor = owner, .id = keylet.key, .amount = asset(fix330Enabled ? 2 : 1)})); env.close(); + } + + // Destination deep freeze → withdraw to 3rd party blocked + { + testcase("VaultWithdraw IOU deep freeze withdraw to 3rd party"); - // Destination deep freeze → withdraw to 3rd party blocked env(trust(issuer, asset(0), charlie, tfSetFreeze | tfSetDeepFreeze)); - { - auto withdrawToCharlie = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); - withdrawToCharlie[sfDestination] = charlie.human(); - env(withdrawToCharlie, Ter(tecFROZEN)); - } + + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(tecFROZEN)); + // Destination deep freeze → self-withdraw unaffected env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + env(trust(issuer, asset(0), charlie, tfClearFreeze | tfClearDeepFreeze)); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); env.close(); + } - // Clawback works while frozen + // Clawback works while frozen + { + testcase("VaultWithdraw IOU freeze clawback unaffected"); env(fset(issuer, asfGlobalFreeze)); + env(vault.clawback( {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(1)})); + env(fclear(issuer, asfGlobalFreeze)); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); env.close(); - }; + } + }; - runTests(); - env.disableFeature(fixCleanup3_3_0); - runTests(); - env.enableFeature(fixCleanup3_3_0); - } + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } - // === MPT === - { - testcase("VaultWithdraw MPT lock checks"); - Env env{*this}; - Vault vault{env}; + void + testVaultWithdrawFreezeMPT() + { + using namespace test::jtx; + testcase("VaultWithdraw MPT lock checks"); - env.fund(XRP(100'000), issuer, owner); - env.close(); + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Env env{*this}; + Vault vault{env}; - MPTTester mptt{env, issuer, kMptInitNoFund}; - mptt.create( - {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth}); - PrettyAsset const mpt{mptt.issuanceID()}; + env.fund(XRP(100'000), issuer, owner); + env.close(); - mptt.authorize({.account = owner}); - mptt.authorize({.account = issuer, .holder = owner}); - env.close(); - env(pay(issuer, owner, mpt(100'000))); - env.close(); + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth}); + PrettyAsset const mpt{mptt.issuanceID()}; - auto [tx, keylet] = vault.create({.owner = owner, .asset = mpt}); - env(tx); - env.close(); - Account const vaultAcct("vault", env.le(keylet)->at(sfAccount)); + mptt.authorize({.account = owner}); + mptt.authorize({.account = issuer, .holder = owner}); + env.close(); + env(pay(issuer, owner, mpt(100'000))); + env.close(); - env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(100)})); - env.close(); + auto [tx, keylet] = vault.create({.owner = owner, .asset = mpt}); + env(tx); + env.close(); + Account const vaultAcct("vault", env.le(keylet)->at(sfAccount)); - Account const charlie{"charlie"}; - env.fund(XRP(10'000), charlie); - env.close(); - mptt.authorize({.account = charlie}); - mptt.authorize({.account = issuer, .holder = charlie}); - env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(100)})); + env.close(); - auto runTests = [&]() { - auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + Account const charlie{"charlie"}; + env.fund(XRP(10'000), charlie); + env.close(); + mptt.authorize({.account = charlie}); + mptt.authorize({.account = issuer, .holder = charlie}); + env.close(); - // Global lock + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + + // Global lock + { + testcase("VaultWithdraw MPT global lock"); mptt.set({.flags = tfMPTLock}); env.close(); env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), @@ -8081,17 +8129,23 @@ class Vault_test : public beast::unit_test::Suite env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); } env.close(); + } - // Vault pseudo-account individual lock + // Vault pseudo-account individual lock + { + testcase("VaultWithdraw MPT pseudo-account lock"); mptt.set({.holder = vaultAcct, .flags = tfMPTLock}); env.close(); env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), Ter(tecLOCKED)); mptt.set({.holder = vaultAcct, .flags = tfMPTUnlock}); env.close(); + } - // Depositor individual lock → self-withdraw blocked - // (isDeepFrozen == isFrozen for MPT) + // Depositor individual lock → self-withdraw blocked + // (isDeepFrozen == isFrozen for MPT) + { + testcase("VaultWithdraw MPT depositor lock"); mptt.set({.holder = owner, .flags = tfMPTLock}); env.close(); env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), @@ -8120,8 +8174,11 @@ class Vault_test : public beast::unit_test::Suite env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); } env.close(); + } - // 3rd party destination lock → withdraw to 3rd party blocked + // 3rd party destination lock → withdraw to 3rd party blocked + { + testcase("VaultWithdraw MPT 3rd party destination lock"); mptt.set({.holder = charlie, .flags = tfMPTLock}); env.close(); { @@ -8136,8 +8193,11 @@ class Vault_test : public beast::unit_test::Suite env.close(); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); env.close(); + } - // Clawback works while locked + // Clawback works while locked + { + testcase("VaultWithdraw MPT lock clawback unaffected"); mptt.set({.flags = tfMPTLock}); env.close(); env(vault.clawback( @@ -8146,14 +8206,13 @@ class Vault_test : public beast::unit_test::Suite env.close(); env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); env.close(); - }; + } + }; - runTests(); - env.disableFeature(fixCleanup3_3_0); - - runTests(); - env.enableFeature(fixCleanup3_3_0); - } + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); } public: @@ -8198,8 +8257,10 @@ public: testWithdrawSoleShareholderPartialFixedSharesUsesFullPrice(); testWithdrawSoleShareholderLoanRepaymentExit(); - testVaultDepositFreeze(); - testVaultWithdrawFreeze(); + testVaultDepositFreezeIOU(); + testVaultDepositFreezeMPT(); + testVaultWithdrawFreezeIOU(); + testVaultWithdrawFreezeMPT(); testVaultSelfWithdrawWhileFrozen(); testReferenceHolding(); From c53aafa6bfeb8591532c770cfcc59abf5f156a2f Mon Sep 17 00:00:00 2001 From: Timothy Banks Date: Wed, 1 Jul 2026 11:36:28 -0400 Subject: [PATCH 9/9] refactor: Retire InnerObjTemplate fix (#7368) --- include/xrpl/protocol/detail/features.macro | 2 +- src/libxrpl/ledger/helpers/AMMHelpers.cpp | 6 ++--- src/libxrpl/protocol/STObject.cpp | 6 ++--- src/libxrpl/tx/transactors/dex/AMMBid.cpp | 17 ++++---------- src/libxrpl/tx/transactors/dex/AMMVote.cpp | 4 +--- src/test/app/AMM_test.cpp | 24 +------------------- src/test/jtx/impl/AMM.cpp | 10 ++------ src/xrpld/rpc/handlers/orderbook/AMMInfo.cpp | 4 +--- 8 files changed, 15 insertions(+), 58 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index a1911761bb..7e64a49b0c 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -58,7 +58,6 @@ XRPL_FIX (XChainRewardRounding, Supported::Yes, VoteBehavior::DefaultNo XRPL_FIX (EmptyDID, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PriceOracle, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (AMMOverflowOffer, Supported::Yes, VoteBehavior::DefaultYes) -XRPL_FIX (InnerObjTemplate, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (FillOrKill, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(DID, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (DisallowIncomingV1, Supported::Yes, VoteBehavior::DefaultNo) @@ -101,6 +100,7 @@ XRPL_RETIRE_FIX(1623) XRPL_RETIRE_FIX(1781) XRPL_RETIRE_FIX(AmendmentMajorityCalc) XRPL_RETIRE_FIX(CheckThreading) +XRPL_RETIRE_FIX(InnerObjTemplate) XRPL_RETIRE_FIX(MasterKeyAsRegularKey) XRPL_RETIRE_FIX(NonFungibleTokensV1_2) XRPL_RETIRE_FIX(NFTokenRemint) diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index 7cd27a1f34..df6d335085 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -602,9 +602,7 @@ std::uint16_t getTradingFee(ReadView const& view, SLE const& ammSle, AccountID const& account) { using namespace std::chrono; - XRPL_ASSERT( - !view.rules().enabled(fixInnerObjTemplate) || ammSle.isFieldPresent(sfAuctionSlot), - "xrpl::getTradingFee : auction present"); + XRPL_ASSERT(ammSle.isFieldPresent(sfAuctionSlot), "xrpl::getTradingFee : auction present"); if (ammSle.isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = safeDowncast(ammSle.peekAtField(sfAuctionSlot)); @@ -822,7 +820,7 @@ initializeFeeAuctionVote( // AMM creator gets the auction slot for free. // AuctionSlot is created on AMMCreate and updated on AMMDeposit // when AMM is in an empty state - if (rules.enabled(fixInnerObjTemplate) && !ammSle->isFieldPresent(sfAuctionSlot)) + if (!ammSle->isFieldPresent(sfAuctionSlot)) { STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot); ammSle->set(std::move(auctionSlot)); diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index 445da16828..c493ba65af 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -78,12 +78,10 @@ STObject::makeInnerObject(SField const& name) // The if is complicated because inner object templates were added in // two phases: // 1. If there are no available Rules, then always apply the template. - // 2. fixInnerObjTemplate added templates to two AMM inner objects. - // 3. fixInnerObjTemplate2 added templates to all remaining inner objects. + // 2. fixInnerObjTemplate2 added templates to all remaining inner objects. std::optional const& rules = getCurrentTransactionRules(); bool const isAMMObj = name == sfAuctionSlot || name == sfVoteEntry; - if (!rules || (rules->enabled(fixInnerObjTemplate) && isAMMObj) || - (rules->enabled(fixInnerObjTemplate2) && !isAMMObj)) + if (!rules || isAMMObj || rules->enabled(fixInnerObjTemplate2)) { if (SOTemplate const* elements = InnerObjectFormats::getInstance().findSOTemplateBySField(name)) diff --git a/src/libxrpl/tx/transactors/dex/AMMBid.cpp b/src/libxrpl/tx/transactors/dex/AMMBid.cpp index 83432fa0d0..3454559e82 100644 --- a/src/libxrpl/tx/transactors/dex/AMMBid.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMBid.cpp @@ -184,18 +184,11 @@ applyBid(ApplyContext& ctx, Sandbox& sb, AccountID const& account, beast::Journa return {tecINTERNAL, false}; STAmount const lptAMMBalance = (*ammSle)[sfLPTokenBalance]; auto const lpTokens = ammLPHolds(sb, *ammSle, account, ctx.journal); - auto const& rules = ctx.view().rules(); - if (!rules.enabled(fixInnerObjTemplate)) - { - if (!ammSle->isFieldPresent(sfAuctionSlot)) - ammSle->makeFieldPresent(sfAuctionSlot); - } - else - { - XRPL_ASSERT(ammSle->isFieldPresent(sfAuctionSlot), "xrpl::applyBid : has auction slot"); - if (!ammSle->isFieldPresent(sfAuctionSlot)) - return {tecINTERNAL, false}; - } + + XRPL_ASSERT(ammSle->isFieldPresent(sfAuctionSlot), "xrpl::applyBid : has auction slot"); + if (!ammSle->isFieldPresent(sfAuctionSlot)) + return {tecINTERNAL, false}; + auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auto const current = duration_cast(ctx.view().header().parentCloseTime.time_since_epoch()).count(); diff --git a/src/libxrpl/tx/transactors/dex/AMMVote.cpp b/src/libxrpl/tx/transactors/dex/AMMVote.cpp index 23604d46cc..0f2b721ac4 100644 --- a/src/libxrpl/tx/transactors/dex/AMMVote.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMVote.cpp @@ -195,9 +195,7 @@ applyVote(ApplyContext& ctx, Sandbox& sb, AccountID const& accountID, beast::Jou } } - XRPL_ASSERT( - !ctx.view().rules().enabled(fixInnerObjTemplate) || ammSle->isFieldPresent(sfAuctionSlot), - "xrpl::applyVote : has auction slot"); + XRPL_ASSERT(ammSle->isFieldPresent(sfAuctionSlot), "xrpl::applyVote : has auction slot"); // Update the vote entries and the trading/discounted fee. ammSle->setFieldArray(sfVoteSlots, updatedVoteSlots); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 3672c4a68a..d4b6ba5429 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -5747,38 +5747,16 @@ private: }; // ledger is closed after each transaction, vote/withdraw don't fail - // regardless whether the amendment is enabled or not test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, true); - test(all - fixInnerObjTemplate, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, true); // ledger is not closed after each transaction - // vote/withdraw don't fail if the amendment is enabled test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, false); - // vote/withdraw fail if the amendment is not enabled - // second vote/withdraw still fail: second vote fails because - // the initial trading fee is 0, consequently second withdraw fails - // because the second vote fails - test( - all - fixInnerObjTemplate, - tefEXCEPTION, - tefEXCEPTION, - tefEXCEPTION, - tefEXCEPTION, - 0, - false); // if non-zero trading/discounted fee then vote/withdraw - // don't fail whether the ledger is closed or not and - // the amendment is enabled or not + // don't fail whether the ledger is closed or not test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, true); - test(all - fixInnerObjTemplate, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, true); test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, false); - test(all - fixInnerObjTemplate, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, false); // non-zero trading fee but discounted fee is 0, vote doesn't fail // but withdraw fails test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 9, false); - // second vote sets the trading fee to non-zero, consequently - // second withdraw doesn't fail even if the amendment is not - // enabled and the ledger is not closed - test(all - fixInnerObjTemplate, tesSUCCESS, tefEXCEPTION, tesSUCCESS, tesSUCCESS, 9, false); } void diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 2184f7e1b0..5c815d4226 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -738,8 +738,7 @@ AMM::bid(BidArg const& arg) { if (auto const amm = env_.current()->read(keylet::amm(asset1_.asset(), asset2_.asset()))) { - if (env_.current()->rules().enabled(fixInnerObjTemplate) && - !amm->isFieldPresent(sfAuctionSlot)) + if (!amm->isFieldPresent(sfAuctionSlot)) Throw("AMM::Bid"); if (amm->isFieldPresent(sfAuctionSlot)) { @@ -867,8 +866,7 @@ AMM::expectAuctionSlot(auto&& cb) const { if (auto const amm = env_.current()->read(keylet::amm(asset1_.asset(), asset2_.asset()))) { - if (env_.current()->rules().enabled(fixInnerObjTemplate) && - !amm->isFieldPresent(sfAuctionSlot)) + if (!amm->isFieldPresent(sfAuctionSlot)) Throw("AMM::expectAuctionSlot"); if (amm->isFieldPresent(sfAuctionSlot)) { @@ -876,10 +874,6 @@ AMM::expectAuctionSlot(auto&& cb) const safeDowncast(amm->peekAtField(sfAuctionSlot)); if (auctionSlot.isFieldPresent(sfAccount)) { - // This could fail in pre-fixInnerObjTemplate tests - // if the submitted transactions recreate one of - // the failure scenarios. Access as optional - // to avoid the failure. auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0); auto const slotInterval = ammAuctionTimeSlot( env_.app().getTimeKeeper().now().time_since_epoch().count(), auctionSlot); diff --git a/src/xrpld/rpc/handlers/orderbook/AMMInfo.cpp b/src/xrpld/rpc/handlers/orderbook/AMMInfo.cpp index 043fdd2d7b..7f54f81423 100644 --- a/src/xrpld/rpc/handlers/orderbook/AMMInfo.cpp +++ b/src/xrpld/rpc/handlers/orderbook/AMMInfo.cpp @@ -214,9 +214,7 @@ doAMMInfo(RPC::JsonContext& context) } if (voteSlots.size() > 0) ammResult[jss::vote_slots] = std::move(voteSlots); - XRPL_ASSERT( - !ledger->rules().enabled(fixInnerObjTemplate) || amm->isFieldPresent(sfAuctionSlot), - "xrpl::doAMMInfo : auction slot is set"); + XRPL_ASSERT(amm->isFieldPresent(sfAuctionSlot), "xrpl::doAMMInfo : auction slot is present"); if (amm->isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = safeDowncast(amm->peekAtField(sfAuctionSlot));