From 37cb4cdbe3ef5bfd5372effbddec5348102a96f0 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Thu, 3 Sep 2026 16:27:45 +0000 Subject: [PATCH 01/16] fix: Recycle the escrow reserve in EscrowCancel and EscrowFinish (#8142) --- .../tx/transactors/escrow/EscrowCancel.cpp | 9 +- .../tx/transactors/escrow/EscrowFinish.cpp | 17 ++-- src/test/app/EscrowToken_test.cpp | 95 +++++++++++++++++++ src/test/app/Sponsor_test.cpp | 21 ++-- 4 files changed, 119 insertions(+), 23 deletions(-) diff --git a/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp b/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp index 21e6bd2c30..32b56b909f 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp @@ -169,6 +169,12 @@ EscrowCancel::doApply() auto const sle = ctx_.view().peek(keylet::account(account)); STAmount const amount = slep->getFieldAmount(sfAmount); + // The return can re-create a holding the owner deleted while the escrow + // was pending; the removed escrow must not be counted against its reserve. + bool const recycleReserve = ctx_.view().rules().enabled(fixCleanup3_4_0); + if (recycleReserve) + decreaseOwnerCountForObject(ctx_.view(), sle, slep, 1, ctx_.journal); + // Transfer amount back to the owner if (isXRP(amount)) { @@ -212,7 +218,8 @@ EscrowCancel::doApply() } } - decreaseOwnerCountForObject(ctx_.view(), sle, slep, 1, ctx_.journal); + if (!recycleReserve) + decreaseOwnerCountForObject(ctx_.view(), sle, slep, 1, ctx_.journal); // Remove escrow from ledger ctx_.view().erase(slep); diff --git a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp index aa352d5e98..09219b0bf1 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp @@ -343,14 +343,12 @@ EscrowFinish::doApply() } } - // With the Sponsor amendment, release the escrow reserve before delivery. - // Token delivery can auto-create a destination holding, and the same - // sponsor (or the same account, for a self-escrow) may cover both the - // escrow being removed and the holding being created. Without the - // amendment, keep the legacy order: releasing early changes the reserve - // arithmetic for self-escrows and would break consensus if not gated. - bool const sponsorEnabled = ctx_.view().rules().enabled(featureSponsor); - if (sponsorEnabled) + // Delivery can auto-create the destination's holding; the removed escrow + // must not be counted against its reserve. The two share a reserve payer + // for a self-escrow, or when one sponsor covers both. + bool const recycleReserve = + ctx_.view().rules().enabled(featureSponsor) || ctx_.view().rules().enabled(fixCleanup3_4_0); + if (recycleReserve) decreaseOwnerCountForObject(ctx_.view(), account, slep, 1, ctx_.journal); STAmount const amount = slep->getFieldAmount(sfAmount); @@ -402,8 +400,7 @@ EscrowFinish::doApply() ctx_.view().update(sled); - // Adjust source owner count (legacy position, pre-Sponsor) - if (!sponsorEnabled) + if (!recycleReserve) decreaseOwnerCountForObject(ctx_.view(), account, slep, 1, ctx_.journal); // Remove escrow from ledger diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index 72db63bd3f..3a2bc14183 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -951,6 +951,99 @@ struct EscrowToken_test : public beast::unit_test::Suite } } + void + testIOUCancelReserveRecycle(FeatureBitset features) + { + testcase("IOU Cancel Reserve Recycle"); + using namespace jtx; + using namespace std::literals; + + // Escrowing the whole IOU balance lets the owner delete the now-zero + // trust line, so cancelling has to re-create it: one object destroyed, + // one created, and the reserve requirement unchanged. + Env env{*this, features}; + bool const fixEnabled = env.current()->rules().enabled(fixCleanup3_4_0); + + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account("gw"); + auto const usd = gw["USD"]; + + env.fund(XRP(10'000), alice, bob, gw); + env.close(); + + env(fset(gw, asfAllowTrustLineLocking)); + env.close(); + + env.trust(usd(10'000), alice); + env.close(); + + env(pay(gw, alice, usd(10'000))); + env.close(); + BEAST_EXPECT(env.ownerCount(alice) == 1); + + auto const cancelAfter = env.now() + 100s; + auto const seq = env.seq(alice); + env(escrow::create(alice, bob, usd(10'000)), + escrow::kFinishTime(env.now() + 1s), + escrow::kCancelTime(cancelAfter), + Fee(baseFee)); + env.close(); + BEAST_EXPECT(env.ownerCount(alice) == 2); + + auto const trustLineKey = keylet::trustLine(alice.id(), gw.id(), usd.currency); + env(trust(alice, usd(0))); + env.close(); + BEAST_EXPECT(!env.current()->exists(trustLineKey)); + BEAST_EXPECT(env.ownerCount(alice) == 1); + + // Leave alice holding the reserve for exactly one owned object. That + // is the escrow now and the re-created trust line after the cancel. + auto const oneObject = env.current()->fees().accountReserve(1, 1); + auto const twoObjects = env.current()->fees().accountReserve(2, 1); + auto const balance = env.balance(alice).value().xrp(); + auto const feeCushion = baseFee.drops() * 20; + env(pay(alice, bob, drops(balance.drops() - oneObject.drops() - feeCushion))); + env.close(); + BEAST_EXPECT(env.balance(alice).value().xrp() >= oneObject); + BEAST_EXPECT(env.balance(alice).value().xrp() < twoObjects); + + for (; env.now() < cancelAfter; env.close()) + { + } + env.close(); + env.close(); + + auto const expectedResult = fixEnabled ? Ter(tesSUCCESS) : Ter(tecNO_LINE_INSUF_RESERVE); + env(escrow::cancel(alice, alice, seq), Fee(baseFee), expectedResult); + env.close(); + + auto const escrowKey = keylet::escrow(alice.id(), SeqProxy::rawSequence(seq)); + if (fixEnabled) + { + BEAST_EXPECT(!env.le(escrowKey)); + BEAST_EXPECT(env.current()->exists(trustLineKey)); + BEAST_EXPECT(env.balance(alice, usd) == usd(10'000)); + BEAST_EXPECT(env.ownerCount(alice) == 1); + } + else + { + // The tec keeps the escrow, so one more owner reserve lets the + // retry through. + BEAST_EXPECT(env.le(escrowKey) != nullptr); + BEAST_EXPECT(!env.current()->exists(trustLineKey)); + BEAST_EXPECT(env.ownerCount(alice) == 1); + + env(pay(bob, alice, drops(twoObjects.drops() - oneObject.drops()))); + env.close(); + env(escrow::cancel(alice, alice, seq), Fee(baseFee), Ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(!env.le(escrowKey)); + BEAST_EXPECT(env.balance(alice, usd) == usd(10'000)); + } + } + void testIOUBalances(FeatureBitset features) { @@ -4250,6 +4343,8 @@ public: } testMPTSplitEscrowTransferFee(all - fixCleanup3_4_0); testMPTSplitEscrowTransferFee(all); + testIOUCancelReserveRecycle(all - fixCleanup3_4_0); + testIOUCancelReserveRecycle(all); } }; diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index 593edfe0a4..9f07e9bdfe 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -5467,13 +5467,12 @@ public: using namespace test::jtx; using namespace std::chrono_literals; - // Finishing a self-escrow (source == destination) whose trust line - // was deleted while the escrow was outstanding auto-creates the line, - // and the outcome of that reserve check depends on whether the escrow - // reserve is released before delivery (Sponsor) or after (legacy). - // With the source's balance in the one-increment window - // [reserve(1), reserve(2)), the legacy order requires reserve(2) and - // fails, while the Sponsor order requires reserve(1) and succeeds. + // Finishing a self-escrow (source == destination) whose trust line was + // deleted while the escrow was outstanding auto-creates the line. With + // the source's balance in the one-increment window + // [reserve(1), reserve(2)), the finish succeeds only when the escrow + // reserve is released before delivery, which either featureSponsor or + // fixCleanup3_4_0 does. auto runTest = [&](FeatureBitset features, TER expected) { Account const alice("alice"); Account const gw("gw"); @@ -5538,11 +5537,9 @@ public: } }; - // Pre-amendment: legacy order — the escrow still counts against the - // reserve while the auto-created line is checked. - runTest(testableAmendments() - featureSponsor, tecNO_LINE_INSUF_RESERVE); - - // Post-amendment: the escrow reserve is recycled into the new line. + runTest(testableAmendments() - featureSponsor - fixCleanup3_4_0, tecNO_LINE_INSUF_RESERVE); + runTest(testableAmendments() - featureSponsor, tesSUCCESS); + runTest(testableAmendments() - fixCleanup3_4_0, tesSUCCESS); runTest(testableAmendments(), tesSUCCESS); } From 986065c16fea3082410e271cb4ad28e2b8b80588 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Thu, 3 Sep 2026 16:54:54 +0000 Subject: [PATCH 02/16] build: Verify glibc version was determined in debian package (#8170) --- .github/scripts/strategy-matrix/linux.json | 4 ++-- package/debian/rules | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/scripts/strategy-matrix/linux.json b/.github/scripts/strategy-matrix/linux.json index d8cdbdfa52..f2cddac488 100644 --- a/.github/scripts/strategy-matrix/linux.json +++ b/.github/scripts/strategy-matrix/linux.json @@ -74,7 +74,7 @@ "extra_cmake_args": "-Dvalidator_keys=ON", "package": { "type": "deb", - "image": "ghcr.io/xrplf/xrpld/packaging-debian:sha-b6a8995" + "image": "ghcr.io/xrplf/xrpld/packaging-debian:sha-49cdc10" } } ], @@ -88,7 +88,7 @@ "extra_cmake_args": "-Dvalidator_keys=ON", "package": { "type": "rpm", - "image": "ghcr.io/xrplf/xrpld/packaging-rhel:sha-b6a8995" + "image": "ghcr.io/xrplf/xrpld/packaging-rhel:sha-49cdc10" } } ] diff --git a/package/debian/rules b/package/debian/rules index 4a9e4ab281..dd6d1e66b9 100755 --- a/package/debian/rules +++ b/package/debian/rules @@ -37,6 +37,10 @@ override_dh_shlibdeps: for binary in xrpld validator-keys; do \ needed=$$(readelf --dyn-syms --wide $$binary \ | grep -o 'GLIBC_[0-9.]*' | sed 's/GLIBC_//' | sort -uV | tail -1); \ + if [ -z "$$needed" ]; then \ + echo "$$binary: no GLIBC_ symbol versions read, cannot check LIBC_MIN" >&2; \ + exit 1; \ + fi; \ if dpkg --compare-versions "$$needed" gt "$(LIBC_MIN)"; then \ echo "$$binary needs glibc $$needed, above LIBC_MIN $(LIBC_MIN)" >&2; \ exit 1; \ From 58a59c37edf80e5e17ac21b262a6cb1a5d373a07 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Thu, 3 Sep 2026 17:19:07 +0000 Subject: [PATCH 03/16] fix: Add signature prefixes for sfCounterpartySignature and sfSponsorSignature (#8162) --- API-CHANGELOG.md | 2 + include/xrpl/core/HashRouter.h | 5 +- include/xrpl/protocol/HashPrefix.h | 20 ++ include/xrpl/protocol/STTx.h | 42 ++- include/xrpl/protocol/Sign.h | 59 +++- src/libxrpl/protocol/STTx.cpp | 76 ++--- src/libxrpl/protocol/Sign.cpp | 68 ++++- src/libxrpl/tx/apply.cpp | 73 ++++- src/test/app/Sponsor_test.cpp | 16 +- src/test/app/lending/LoanLifecycle_test.cpp | 10 +- src/test/app/lending/LoanMisc_test.cpp | 35 ++- src/test/app/lending/LoanSecurity_test.cpp | 66 +++++ src/test/app/lending/LoanValidation_test.cpp | 17 +- src/test/app/tx/apply_test.cpp | 141 ++++++++++ src/test/jtx/impl/multisign.cpp | 8 +- src/test/jtx/impl/sig.cpp | 10 +- src/test/jtx/impl/utility.cpp | 16 +- src/test/jtx/utility.h | 22 +- src/test/protocol/STTx_test.cpp | 282 ++++++++++++++++++- src/xrpld/app/misc/NetworkOPs.cpp | 26 +- src/xrpld/overlay/detail/PeerImp.cpp | 23 +- src/xrpld/rpc/detail/TransactionSign.cpp | 31 +- 22 files changed, 924 insertions(+), 124 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index df58282f76..6f746e85cf 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -41,6 +41,8 @@ This section contains changes targeting a future version. ### Bugfixes +- `sign`, `sign_for`, `submit`: `signature_target` now returns `invalidParams` unless it names `CounterpartySignature` or `SponsorSignature`. It previously accepted any inner object field, such as `Book` or `NFToken`, and signed into it. +- `sign`, `sign_for`, `submit`, `submit_multisigned`: With `fixCleanup3_4_0` enabled, a signature in `CounterpartySignature` or `SponsorSignature` covers a different prefix than the transaction's own signature, so a signature can no longer be moved from one of those roles into another. Clients that build these signatures themselves must use the new prefixes: `CPT` and `CPM` (single- and multi-signing) for `CounterpartySignature`, and `SPN` and `SPM` for `SponsorSignature`. - `get_aggregate_price`: Duplicate entries in the `oracles` request array are now ignored. [#6586](https://github.com/XRPLF/rippled/pull/6586) - Peer Crawler: The `port` field in `overlay.active[]` now consistently returns an integer instead of a string for outbound peers. [#6318](https://github.com/XRPLF/rippled/pull/6318) - `ping`: The `ip` field is no longer returned as an empty string for proxied connections without a forwarded-for header. It is now omitted, consistent with the behavior for identified connections. [#6730](https://github.com/XRPLF/rippled/pull/6730) diff --git a/include/xrpl/core/HashRouter.h b/include/xrpl/core/HashRouter.h index 20aafecc5f..25e4df3d0d 100644 --- a/include/xrpl/core/HashRouter.h +++ b/include/xrpl/core/HashRouter.h @@ -34,7 +34,10 @@ enum class HashRouterFlags : std::uint16_t { PRIVATE4 = 0x0800, // Used in EscrowFinish.cpp PRIVATE5 = 0x1000, - PRIVATE6 = 0x2000 + PRIVATE6 = 0x2000, + // Used in apply.cpp + PRIVATE7 = 0x4000, + PRIVATE8 = 0x8000 }; constexpr HashRouterFlags diff --git a/include/xrpl/protocol/HashPrefix.h b/include/xrpl/protocol/HashPrefix.h index 9d4471d05c..e77e891b04 100644 --- a/include/xrpl/protocol/HashPrefix.h +++ b/include/xrpl/protocol/HashPrefix.h @@ -92,6 +92,26 @@ enum class HashPrefix : std::uint32_t { * Batch */ Batch = detail::makeHashPrefix('B', 'C', 'H'), + + /** + * inner transaction to sign as the counterparty + */ + CounterpartyTxSign = detail::makeHashPrefix('C', 'P', 'T'), + + /** + * inner transaction to multi-sign as the counterparty + */ + CounterpartyTxMultiSign = detail::makeHashPrefix('C', 'P', 'M'), + + /** + * inner transaction to sign as the sponsor + */ + SponsorTxSign = detail::makeHashPrefix('S', 'P', 'N'), + + /** + * inner transaction to multi-sign as the sponsor + */ + SponsorTxMultiSign = detail::makeHashPrefix('S', 'P', 'M'), }; template diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index e213d4e0b7..e6291ae950 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -13,6 +14,7 @@ #include #include #include +#include #include #include @@ -105,14 +107,36 @@ public: [[nodiscard]] json::Value getJson(JsonOptions options, bool binary) const; + /** + * Sign the transaction as its account. + * + * @param publicKey The public key for signing. + * @param secretKey The secret key for signing. + */ + void + sign(PublicKey const& publicKey, SecretKey const& secretKey); + + /** + * Sign the transaction in one of its signature fields. + * + * The signature is bound to the role that made it, so it cannot be moved + * into another role. + * + * @param publicKey The public key for signing. + * @param secretKey The secret key for signing. + * @param role The role signing the transaction. + * @param rules The current ledger rules. + */ void sign( PublicKey const& publicKey, SecretKey const& secretKey, - std::optional> signatureTarget = {}); + SignatureRole role, + Rules const& rules); /** * Check the signature. + * * @param rules The current ledger rules. * @return `true` if valid signature. If invalid, the error message string. */ @@ -120,7 +144,7 @@ public: checkSign(Rules const& rules) const; [[nodiscard]] std::expected - checkBatchSign(Rules const& rules) const; + checkBatchSign() const; // SQL Functions with metadata. static std::string const& @@ -162,28 +186,28 @@ public: private: /** * Check the signature. + * * @param rules The current ledger rules. * @param sigObject Reference to object that contains the signature fields. * Will be *this more often than not. + * @param role The role that made the signature in sigObject. Determines + * the signing prefix, which binds the signature to that role. * @return `true` if valid signature. If invalid, the error message string. */ [[nodiscard]] std::expected - checkSign(Rules const& rules, STObject const& sigObject) const; + checkSign(Rules const& rules, STObject const& sigObject, SignatureRole role) const; [[nodiscard]] std::expected - checkSingleSign(STObject const& sigObject) const; + checkSingleSign(STObject const& sigObject, HashPrefix prefix) const; [[nodiscard]] std::expected - checkMultiSign(Rules const& rules, STObject const& sigObject) const; + checkMultiSign(STObject const& sigObject, HashPrefix prefix) const; [[nodiscard]] std::expected checkBatchSingleSign(STObject const& batchSigner, std::vector const& txIds) const; [[nodiscard]] std::expected - checkBatchMultiSign( - STObject const& batchSigner, - Rules const& rules, - std::vector const& txIds) const; + checkBatchMultiSign(STObject const& batchSigner, std::vector const& txIds) const; void buildBatchTxns(); diff --git a/include/xrpl/protocol/Sign.h b/include/xrpl/protocol/Sign.h index fad2c35c9e..b7a318eb35 100644 --- a/include/xrpl/protocol/Sign.h +++ b/include/xrpl/protocol/Sign.h @@ -4,13 +4,65 @@ #include #include #include +#include #include #include #include #include +#include + namespace xrpl { +/** + * The signature slots on a transaction. + * + * Each role signs different bytes, so a signature cannot be moved from the + * role that made it into another role. See signingPrefix. + */ +enum class SignatureRole { + /** + * The transaction's own signature, in sfTxnSignature or sfSigners. + */ + Transaction, + /** + * The counterparty's signature, in sfCounterpartySignature. + */ + Counterparty, + /** + * The sponsor's signature, in sfSponsorSignature. + */ + Sponsor +}; + +/** + * The field that holds this role's signature. + * + * @return The signature field, or nullptr for SignatureRole::Transaction, + * whose signature lives at the top level of the transaction. + */ +[[nodiscard]] SField const* +signatureField(SignatureRole role); + +/** + * The role that signs into the given field. + * + * @return The role, or an unseated optional if the field does not hold a + * transaction signature. + */ +[[nodiscard]] std::optional +signatureRole(SField const& sigField); + +/** + * The hash prefix that binds a transaction signature to the role that made it. + * + * @param role The role making the signature. + * @param multiSigning Whether the signature is a multi-signature. + * @param rules The current ledger rules. + */ +[[nodiscard]] HashPrefix +signingPrefix(SignatureRole role, bool multiSigning, Rules const& rules); + /** * Sign an STObject * @@ -49,9 +101,12 @@ verify( /** * Return a Serializer suitable for computing a multisigning TxnSignature. + * + * @param prefix Prefix to insert before the serialized object. Get it from + * signingPrefix, so that the signature is bound to the role making it. */ Serializer -buildMultiSigningData(STObject const& obj, AccountID const& signingID); +buildMultiSigningData(STObject const& obj, AccountID const& signingID, HashPrefix prefix); /** * Break the multi-signing hash computation into 2 parts for optimization. @@ -67,7 +122,7 @@ buildMultiSigningData(STObject const& obj, AccountID const& signingID); * signer's unique data. */ Serializer -startMultiSigningData(STObject const& obj); +startMultiSigningData(STObject const& obj, HashPrefix prefix); inline void finishMultiSigningData(AccountID const& signingID, Serializer& s) diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index ce672b515d..3db6a3dc6c 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -168,10 +168,10 @@ STTx::getMentionedAccounts() const } static Blob -getSigningData(STTx const& that) +getSigningData(STTx const& that, HashPrefix prefix) { Serializer s; - s.add32(HashPrefix::TxSign); + s.add32(prefix); that.addWithoutSigningFields(s); return s.getData(); } @@ -212,30 +212,42 @@ STTx::getSeqProxy() const return SeqProxy::rawTicket(*ticketSeq); } +void +STTx::sign(PublicKey const& publicKey, SecretKey const& secretKey) +{ + // The account's own signature always covers the plain transaction prefix; + // see signingPrefix for the role signatures that do not. + auto const data = getSigningData(*this, HashPrefix::TxSign); + + setFieldVL(sfTxnSignature, xrpl::sign(publicKey, secretKey, makeSlice(data))); + tid_ = getHash(HashPrefix::TransactionId); +} + void STTx::sign( PublicKey const& publicKey, SecretKey const& secretKey, - std::optional> signatureTarget) + SignatureRole role, + Rules const& rules) { - auto const data = getSigningData(*this); + auto const data = getSigningData(*this, signingPrefix(role, false, rules)); auto const sig = xrpl::sign(publicKey, secretKey, makeSlice(data)); - if (signatureTarget) + if (auto const target = signatureField(role)) { - auto& target = peekFieldObject(*signatureTarget); - target.setFieldVL(sfTxnSignature, sig); + peekFieldObject(*target).setFieldVL(sfTxnSignature, sig); } else { setFieldVL(sfTxnSignature, sig); } + tid_ = getHash(HashPrefix::TransactionId); } std::expected -STTx::checkSign(Rules const& rules, STObject const& sigObject) const +STTx::checkSign(Rules const& rules, STObject const& sigObject, SignatureRole role) const { try { @@ -244,8 +256,10 @@ STTx::checkSign(Rules const& rules, STObject const& sigObject) const // multi-signing. Otherwise we're single-signing. Blob const& signingPubKey = sigObject.getFieldVL(sfSigningPubKey); - return signingPubKey.empty() ? checkMultiSign(rules, sigObject) - : checkSingleSign(sigObject); + bool const multiSigning = signingPubKey.empty(); + auto const prefix = signingPrefix(role, multiSigning, rules); + return multiSigning ? checkMultiSign(sigObject, prefix) + : checkSingleSign(sigObject, prefix); } catch (...) { @@ -256,20 +270,20 @@ STTx::checkSign(Rules const& rules, STObject const& sigObject) const std::expected STTx::checkSign(Rules const& rules) const { - if (auto const ret = checkSign(rules, *this); !ret) + if (auto const ret = checkSign(rules, *this, SignatureRole::Transaction); !ret) return ret; if (isFieldPresent(sfCounterpartySignature)) { auto const counterSig = getFieldObject(sfCounterpartySignature); - if (auto const ret = checkSign(rules, counterSig); !ret) + if (auto const ret = checkSign(rules, counterSig, SignatureRole::Counterparty); !ret) return std::unexpected("Counterparty: " + ret.error()); } if (isFieldPresent(sfSponsorSignature)) { auto const sponsorSignatureObj = getFieldObject(sfSponsorSignature); - if (auto const ret = checkSign(rules, sponsorSignatureObj); !ret) + if (auto const ret = checkSign(rules, sponsorSignatureObj, SignatureRole::Sponsor); !ret) return std::unexpected("Sponsor: " + ret.error()); } @@ -277,14 +291,14 @@ STTx::checkSign(Rules const& rules) const // of signature checking. if (isFieldPresent(sfBatchSigners)) { - if (auto const ret = checkBatchSign(rules); !ret) + if (auto const ret = checkBatchSign(); !ret) return ret; } return {}; } std::expected -STTx::checkBatchSign(Rules const& rules) const +STTx::checkBatchSign() const { try { @@ -318,7 +332,7 @@ STTx::checkBatchSign(Rules const& rules) const for (auto const& signer : signers) { Blob const& signingPubKey = signer.getFieldVL(sfSigningPubKey); - auto const result = signingPubKey.empty() ? checkBatchMultiSign(signer, rules, txIds) + auto const result = signingPubKey.empty() ? checkBatchMultiSign(signer, txIds) : checkBatchSingleSign(signer, txIds); if (!result) @@ -447,9 +461,9 @@ singleSignHelper(STObject const& sigObject, Slice const& data) } std::expected -STTx::checkSingleSign(STObject const& sigObject) const +STTx::checkSingleSign(STObject const& sigObject, HashPrefix prefix) const { - auto const data = getSigningData(*this); + auto const data = getSigningData(*this, prefix); return singleSignHelper(sigObject, makeSlice(data)); } @@ -467,8 +481,7 @@ std::expected multiSignHelper( STObject const& sigObject, std::optional txnAccountID, - std::function makeMsg, - Rules const& rules) + std::function makeMsg) { // Make sure the MultiSigners are present. Otherwise they are not // attempting multi-signing and we just have a bad SigningPubKey. @@ -541,10 +554,7 @@ multiSignHelper( } std::expected -STTx::checkBatchMultiSign( - STObject const& batchSigner, - Rules const& rules, - std::vector const& txIds) const +STTx::checkBatchMultiSign(STObject const& batchSigner, std::vector const& txIds) const { XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchMultiSign : batch transaction"); // We can ease the computational load inside the loop a bit by @@ -555,18 +565,15 @@ STTx::checkBatchMultiSign( serializeBatch(dataStart, getAccountID(sfAccount), getSeqProxy().value(), getFlags(), txIds); dataStart.addBitString(batchSignerAccount); return multiSignHelper( - batchSigner, - batchSignerAccount, - [&dataStart](AccountID const& accountID) -> Serializer { + batchSigner, batchSignerAccount, [&dataStart](AccountID const& accountID) -> Serializer { Serializer s = dataStart; finishMultiSigningData(accountID, s); return s; - }, - rules); + }); } std::expected -STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const +STTx::checkMultiSign(STObject const& sigObject, HashPrefix prefix) const { // Used inside the loop in multiSignHelper to enforce that // the account owner may not multisign for themselves. @@ -578,16 +585,13 @@ STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const // 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. - Serializer dataStart = startMultiSigningData(*this); + Serializer dataStart = startMultiSigningData(*this, prefix); return multiSignHelper( - sigObject, - txnAccountID, - [&dataStart](AccountID const& accountID) -> Serializer { + sigObject, txnAccountID, [&dataStart](AccountID const& accountID) -> Serializer { Serializer s = dataStart; finishMultiSigningData(accountID, s); return s; - }, - rules); + }); } void diff --git a/src/libxrpl/protocol/Sign.cpp b/src/libxrpl/protocol/Sign.cpp index 9e7ef7f999..ce3dabde33 100644 --- a/src/libxrpl/protocol/Sign.cpp +++ b/src/libxrpl/protocol/Sign.cpp @@ -1,17 +1,77 @@ #include +#include #include +#include #include #include #include +#include #include #include #include #include #include +#include + namespace xrpl { +SField const* +signatureField(SignatureRole role) +{ + switch (role) + { + case SignatureRole::Transaction: + return nullptr; + case SignatureRole::Counterparty: + return &sfCounterpartySignature; + case SignatureRole::Sponsor: + return &sfSponsorSignature; + } + UNREACHABLE("xrpl::signatureField : unknown SignatureRole"); + return nullptr; +} + +std::optional +signatureRole(SField const& sigField) +{ + if (sigField == sfCounterpartySignature) + return SignatureRole::Counterparty; + if (sigField == sfSponsorSignature) + return SignatureRole::Sponsor; + return std::nullopt; +} + +// Signature validity depends on fixCleanup3_4_0: a role signature covers +// different bytes before and after the amendment activates. checkValidity +// caches its verdict per transaction ID, so it keeps two separate cache slots +// for role-signature transactions (kSfSiggoodOldPrefix / kSfSigbadOldPrefix in +// tx/apply.cpp) to keep a pre-fix verdict from being reused in the post-fix +// era, and vice versa. See the block comment in tx/apply.cpp for the details +// and the reason both directions matter. +HashPrefix +signingPrefix(SignatureRole role, bool multiSigning, Rules const& rules) +{ + // Before fixCleanup3_4_0 every signature on a transaction covered the same + // bytes, so a signature could be moved from one role to another. + if (!rules.enabled(fixCleanup3_4_0)) + return multiSigning ? HashPrefix::TxMultiSign : HashPrefix::TxSign; + + switch (role) + { + case SignatureRole::Transaction: + return multiSigning ? HashPrefix::TxMultiSign : HashPrefix::TxSign; + case SignatureRole::Counterparty: + return multiSigning ? HashPrefix::CounterpartyTxMultiSign + : HashPrefix::CounterpartyTxSign; + case SignatureRole::Sponsor: + return multiSigning ? HashPrefix::SponsorTxMultiSign : HashPrefix::SponsorTxSign; + } + UNREACHABLE("xrpl::signingPrefix : unknown SignatureRole"); + return multiSigning ? HashPrefix::TxMultiSign : HashPrefix::TxSign; +} + void sign( STObject& st, @@ -70,18 +130,18 @@ verify(STObject const& st, HashPrefix const& prefix, PublicKey const& pk, SF_VL // So, if we support multiple levels of signing, then we'll need to // incorporate the "signing for" accounts into the signing data as well. Serializer -buildMultiSigningData(STObject const& obj, AccountID const& signingID) +buildMultiSigningData(STObject const& obj, AccountID const& signingID, HashPrefix prefix) { - Serializer s{startMultiSigningData(obj)}; + Serializer s{startMultiSigningData(obj, prefix)}; finishMultiSigningData(signingID, s); return s; } Serializer -startMultiSigningData(STObject const& obj) +startMultiSigningData(STObject const& obj, HashPrefix prefix) { Serializer s; - s.add32(HashPrefix::TxMultiSign); + s.add32(prefix); obj.addWithoutSigningFields(s); return s; } diff --git a/src/libxrpl/tx/apply.cpp b/src/libxrpl/tx/apply.cpp index f93b19a158..688c585e2a 100644 --- a/src/libxrpl/tx/apply.cpp +++ b/src/libxrpl/tx/apply.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -23,13 +24,38 @@ namespace xrpl { -// These are the same flags defined as HashRouterFlags::PRIVATE1-4 in -// HashRouter.h +// This file owns HashRouterFlags::PRIVATE1-4 and PRIVATE7-8 in HashRouter.h. +// These are the first four; the other two are below. constexpr HashRouterFlags kSfSigbad = HashRouterFlags::PRIVATE1; // Signature is bad constexpr HashRouterFlags kSfSiggood = HashRouterFlags::PRIVATE2; // Signature is good constexpr HashRouterFlags kSfLocalbad = HashRouterFlags::PRIVATE3; // Local checks failed constexpr HashRouterFlags kSfLocalgood = HashRouterFlags::PRIVATE4; // Local checks passed +// Before fixCleanup3_4_0, a signature in an alternate role field, such as +// sfSponsorSignature, covered the same bytes as the top level signature. Which +// bytes a role signature must cover therefore depends on whether the fix is +// enabled, but the four flags above record only the verdict, not the rules that +// produced it. A verdict reached under one prefix would otherwise be reused +// under the other. +// +// The two flags below hold the verdict for the pre-fix prefixes, so the pre-fix +// and post-fix verdicts occupy separate slots and neither is ever read in the +// other's era. Nothing is cleared when the amendment activates: setFlags only +// sets bits, so a stale pre-fix verdict simply stops being read and ages out +// with the rest of the routing table. +// +// This is not one switchover at a single instant. The era is chosen per call +// from the rules passed in, and callers do not agree on the rules: relay and +// submit verify against the validated rules, which lag the open ledger rules +// that preflight2 verifies against. At the amendment's flag ledger the same +// transaction can therefore be checked under both prefixes, on the same node, +// at the same time. +// +// Remove these two flags, and oldPrefixSig below, when Cleanup3_4_0 is retired +// in features.macro. +constexpr HashRouterFlags kSfSigbadOldPrefix = HashRouterFlags::PRIVATE7; +constexpr HashRouterFlags kSfSiggoodOldPrefix = HashRouterFlags::PRIVATE8; + //------------------------------------------------------------------------------ std::pair @@ -48,21 +74,41 @@ checkValidity(HashRouter& router, STTx const& tx, Rules const& rules) return {Validity::SigBad, "Batch inner transactions are never considered validly signed."}; } - if (any(flags & kSfSigbad)) + // Pick the cache slot for this call's era; see kSfSiggoodOldPrefix above. + // Only a transaction that carries a role signature, and only while the fix + // is disabled, uses the separate slot. Every other transaction, and every + // transaction once the fix is enabled, uses the ordinary flags and verifies + // exactly once, so there is no steady state cost. + // + // Both directions matter. A good verdict from before the fix must not let a + // signature moved between roles survive the amendment, and a bad verdict + // from before the fix must not condemn a transaction that the new prefixes + // accept. + // + // Whether a transaction carries a role signature is fixed for its ID: the + // fields are kNotSigning, so they are excluded from the signed bytes, but + // they are still covered by the transaction ID. Repeat calls for one ID + // therefore always agree on which slot pair to use. + bool const oldPrefixSig = !rules.enabled(fixCleanup3_4_0) && + (tx.isFieldPresent(sfSponsorSignature) || tx.isFieldPresent(sfCounterpartySignature)); + auto const sigbadFlag = oldPrefixSig ? kSfSigbadOldPrefix : kSfSigbad; + auto const siggoodFlag = oldPrefixSig ? kSfSiggoodOldPrefix : kSfSiggood; + + if (any(flags & sigbadFlag)) { // Signature is known bad return {Validity::SigBad, "Transaction has bad signature."}; } - if (!any(flags & kSfSiggood)) + if (!any(flags & siggoodFlag)) { auto const sigVerify = tx.checkSign(rules); if (!sigVerify) { - router.setFlags(id, kSfSigbad); + router.setFlags(id, sigbadFlag); return {Validity::SigBad, sigVerify.error()}; } - router.setFlags(id, kSfSiggood); + router.setFlags(id, siggoodFlag); } // Signature is now known good @@ -94,6 +140,19 @@ checkValidity(HashRouter& router, STTx const& tx, Rules const& rules) void forceValidity(HashRouter& router, uint256 const& txid, Validity validity) { + // Callers reach here when they deliberately skip signature verification, + // such as a cluster peer that trusts its neighbor's checks, or a + // configuration that turns signature checks off. Nothing was verified, so + // there is no prefix era to record. Mark both of checkValidity's signature + // slots good: otherwise the forced verdict is ignored for a role-signature + // transaction until fixCleanup3_4_0 is enabled, and the signature the + // caller meant to skip gets verified after all. Marking both cannot leak a + // verdict across eras, because no verdict was reached, and this is the only + // place the distinction can be recorded: kSfSiggood alone does not say + // whether checkValidity verified a post-fix signature or a caller forced + // the result. An already cached bad verdict still wins, since checkValidity + // tests its bad flag first. Drop kSfSiggoodOldPrefix when Cleanup3_4_0 is + // retired. HashRouterFlags flags = HashRouterFlags::UNDEFINED; switch (validity) { @@ -101,7 +160,7 @@ forceValidity(HashRouter& router, uint256 const& txid, Validity validity) flags |= kSfLocalgood; [[fallthrough]]; case Validity::SigGoodOnly: - flags |= kSfSiggood; + flags |= kSfSiggood | kSfSiggoodOldPrefix; [[fallthrough]]; case Validity::SigBad: // would be silly to call directly diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index 9f07e9bdfe..71d968014f 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -376,11 +376,11 @@ public: } void - testSingleSigning() + testSingleSigning(FeatureBitset features) { testcase("Single signing"); using namespace test::jtx; - Env env{*this, testableAmendments()}; + Env env{*this, features}; Account const alice("alice"); Account const sponsor("sponsor"); Account const invalid("invalid"); @@ -415,11 +415,11 @@ public: } void - testMultiSigning() + testMultiSigning(FeatureBitset features) { testcase("Multi signing"); using namespace test::jtx; - Env env{*this, testableAmendments()}; + Env env{*this, features}; Account const alice("alice"); Account const bob("bob"); Account const sponsor("sponsor"); @@ -5675,8 +5675,12 @@ protected: testInvalidSponsorshipSet(); testPseudoAccountSponsorship(); - testSingleSigning(); - testMultiSigning(); + // The signing prefix of an alternate signature field changes with + // fixCleanup3_4_0, so sign and verify under both rule sets. + testSingleSigning(jtx::testableAmendments()); + testSingleSigning(jtx::testableAmendments() - fixCleanup3_4_0); + testMultiSigning(jtx::testableAmendments()); + testMultiSigning(jtx::testableAmendments() - fixCleanup3_4_0); testInvalidSponsorField(); diff --git a/src/test/app/lending/LoanLifecycle_test.cpp b/src/test/app/lending/LoanLifecycle_test.cpp index dae6f6ce16..45420529c6 100644 --- a/src/test/app/lending/LoanLifecycle_test.cpp +++ b/src/test/app/lending/LoanLifecycle_test.cpp @@ -205,8 +205,14 @@ private: if (!BEAST_EXPECT(!createJson.isMember(jss::Signers))) counterpartyJson[sfSigners] = createJson[sfSigners]; - // The duplicated signature works - createJson = env.json(createJson, Json(sfCounterpartySignature, counterpartyJson)); + // The duplicated signature does not work: the counterparty signs a + // different prefix than the account. + env(env.json(createJson, Json(sfCounterpartySignature, counterpartyJson)), + Ter(telENV_RPC_FAILED)); + + // Signing the counterparty field itself works, even though the lender + // is both the borrower and the counterparty. + createJson = env.json(createJson, Sig(sfCounterpartySignature, lender)); env(createJson); env.close(); diff --git a/src/test/app/lending/LoanMisc_test.cpp b/src/test/app/lending/LoanMisc_test.cpp index c5a7d54311..7c4db3e2bf 100644 --- a/src/test/app/lending/LoanMisc_test.cpp +++ b/src/test/app/lending/LoanMisc_test.cpp @@ -113,21 +113,26 @@ private: txJson[sfTransactionType] = "AccountSet"; txJson[sfAccount] = borrower.human(); - auto const borrowerSignParams = [&]() { - json::Value params{json::ValueType::Object}; - params[jss::passphrase] = borrowerPass; - params[jss::key_type] = "ed25519"; - params[jss::signature_target] = "Destination"; - params[jss::tx_json] = txJson; - return params; - }(); - auto const jSignBorrower = env.rpc("json", "sign", to_string(borrowerSignParams)); - BEAST_EXPECT( - jSignBorrower.isMember(jss::result) && - jSignBorrower[jss::result].isMember(jss::error) && - jSignBorrower[jss::result][jss::error] == "invalidParams" && - jSignBorrower[jss::result].isMember(jss::error_message) && - jSignBorrower[jss::result][jss::error_message] == "Destination"); + // "Destination" is not an inner object at all. "Book" is one, but + // it holds no transaction signature, so it is not a target either. + for (char const* target : {"Destination", "Book", "Signer"}) + { + auto const borrowerSignParams = [&]() { + json::Value params{json::ValueType::Object}; + params[jss::passphrase] = borrowerPass; + params[jss::key_type] = "ed25519"; + params[jss::signature_target] = target; + params[jss::tx_json] = txJson; + return params; + }(); + auto const jSignBorrower = env.rpc("json", "sign", to_string(borrowerSignParams)); + BEAST_EXPECT( + jSignBorrower.isMember(jss::result) && + jSignBorrower[jss::result].isMember(jss::error) && + jSignBorrower[jss::result][jss::error] == "invalidParams" && + jSignBorrower[jss::result].isMember(jss::error_message) && + jSignBorrower[jss::result][jss::error_message] == target); + } } { testcase("RPC LoanSet - sign and submit borrower initiated"); diff --git a/src/test/app/lending/LoanSecurity_test.cpp b/src/test/app/lending/LoanSecurity_test.cpp index 463273e227..54c972b505 100644 --- a/src/test/app/lending/LoanSecurity_test.cpp +++ b/src/test/app/lending/LoanSecurity_test.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -1090,9 +1091,74 @@ private: } } + // Every signature on a transaction covered the same bytes before + // fixCleanup3_4_0, so a signature could be moved from the role that made + // it into another role. Here the lender signs a LoanSet as the + // counterparty, and the borrower copies that signature into the + // SponsorSignature, making the lender pay the fee without the lender ever + // agreeing to sponsor it. + void + testSignatureCopiedBetweenRoles(bool fixEnabled) + { + testcase( + std::string("Counterparty signature copied into the sponsor slot") + + (fixEnabled ? "" : " (pre-amendment)")); + + using namespace jtx; + using namespace loan; + + Env env(*this, fixEnabled ? all_ : all_ - fixCleanup3_4_0); + BEAST_EXPECT(env.enabled(fixCleanup3_4_0) == fixEnabled); + + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(100'000'000), lender, borrower); + env.close(); + + PrettyAsset const xrpAsset{xrpIssue(), 1'000'000}; + auto const broker = createVaultAndBroker(env, xrpAsset, lender); + + auto const feeAmt = XRP(1); + + // The lender agrees to the loan by signing the Counterparty slot of a + // LoanSet that names the lender as the fee sponsor. The lender signs + // nothing else. + auto loanSet = env.json( + set(borrower, broker.brokerID, broker.asset(Number{1, 3}).value()), + sponsor::As(lender, spfSponsorFee), + Sig(sfCounterpartySignature, lender), + Fee(feeAmt)); + + // The borrower copies the lender's signature into the sponsor slot. + loanSet[sfSponsorSignature.jsonName] = loanSet[sfCounterpartySignature.jsonName]; + + auto const lenderBalance = env.balance(lender); + auto const borrowerBalance = env.balance(borrower); + + env(loanSet, Ter(fixEnabled ? TER{telENV_RPC_FAILED} : TER{tesSUCCESS})); + env.close(); + + if (fixEnabled) + { + // The copied signature does not verify in the sponsor slot, so + // nothing happens at all. + BEAST_EXPECT(env.balance(lender) == lenderBalance); + BEAST_EXPECT(env.balance(borrower) == borrowerBalance); + } + else + { + // The lender paid the fee, and the borrower got the loan. + BEAST_EXPECT(env.balance(lender) == lenderBalance - feeAmt); + BEAST_EXPECT(env.balance(borrower).value() > borrowerBalance.value()); + } + } + void runAmendmentIndependent() { + testSignatureCopiedBetweenRoles(true); + testSignatureCopiedBetweenRoles(false); testRIPD3901(); testImpairmentPaymentDateUnchanged(); testImpairmentPaymentDatePreAmendment(); diff --git a/src/test/app/lending/LoanValidation_test.cpp b/src/test/app/lending/LoanValidation_test.cpp index 169a02c462..566633b690 100644 --- a/src/test/app/lending/LoanValidation_test.cpp +++ b/src/test/app/lending/LoanValidation_test.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -522,15 +521,13 @@ private: auto const loanSetFee = Fee(env.current()->fees().base * 2); Number const principalRequest{1, 3}; - auto createJson = env.json(set(lender, broker.brokerID, principalRequest), Fee(loanSetFee)); - - json::Value counterpartyJson{json::ValueType::Object}; - counterpartyJson[sfTxnSignature] = createJson[sfTxnSignature]; - counterpartyJson[sfSigningPubKey] = createJson[sfSigningPubKey]; - if (!BEAST_EXPECT(!createJson.isMember(jss::Signers))) - counterpartyJson[sfSigners] = createJson[sfSigners]; - - createJson = env.json(createJson, Json(sfCounterpartySignature, counterpartyJson)); + // The lender is both the borrower and the counterparty here, but the + // two roles sign different bytes, so each signature must be made for + // the field it goes into. + auto const createJson = env.json( + set(lender, broker.brokerID, principalRequest), + Sig(sfCounterpartySignature, lender), + Fee(loanSetFee)); env(createJson); env.close(); diff --git a/src/test/app/tx/apply_test.cpp b/src/test/app/tx/apply_test.cpp index 8f71d47fcf..39289a6264 100644 --- a/src/test/app/tx/apply_test.cpp +++ b/src/test/app/tx/apply_test.cpp @@ -1,12 +1,23 @@ // Copyright (c) 2020 Dev Null Productions +#include #include +#include +#include +#include +#include +#include +#include +#include #include #include #include +#include +#include #include #include +#include #include #include @@ -22,6 +33,136 @@ public: { testcase("Require Fully Canonical Signature"); testFullyCanonicalSigs(); + testRoleSignatureCacheIsEraSpecific(); + testForcedValidityIgnoresPrefixEra(); + } + + // forceValidity means the caller verified nothing and wants the result + // trusted, so it has to hold in both prefix eras. If it marked only the + // ordinary slot, a role-signature transaction would still be verified + // under pre-fix rules, defeating the cluster path and the configurations + // that turn signature checks off. + void + testForcedValidityIgnoresPrefixEra() + { + testcase("Forced validity ignores the prefix era"); + + using namespace test::jtx; + + Env preFix{*this, testableAmendments() - fixCleanup3_4_0}; + Env postFix{*this, testableAmendments()}; + auto const preFixRules = preFix.current()->rules(); + + Account const alice{"alice"}; + Account const sponsor{"sponsor"}; + postFix.fund(XRP(10'000), alice, sponsor); + postFix.close(); + + // Signed under the post-fix rules, so this signature does not verify + // under the pre-fix prefix. Only the forced verdict can make the check + // below pass. + auto const jt = postFix.jt( + noop(alice), + Fee(XRP(1)), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor)); + if (!BEAST_EXPECT(jt.stx)) + return; + + // A router that has never seen this transaction, so the only cached + // state is what forceValidity writes. + auto& router = preFix.app().getHashRouter(); + forceValidity(router, jt.stx->getTransactionID(), Validity::SigGoodOnly); + BEAST_EXPECT(checkValidity(router, *jt.stx, preFixRules).first != Validity::SigBad); + } + + // A signature verdict reached under one prefix era must not be honored in + // the other, because the two eras require the sponsor signature to cover + // different bytes. Each direction below uses one HashRouter and differs + // only in the rules, which is what the flag ledger looks like in practice: + // relay and submit verify against the validated rules, which lag the open + // ledger rules that preflight2 verifies against, so one transaction gets + // checked under both prefixes at the same time. + void + testRoleSignatureCacheIsEraSpecific() + { + testcase("Role signature cache is era specific"); + + using namespace test::jtx; + + Env preFix{*this, testableAmendments() - fixCleanup3_4_0}; + Env postFix{*this, testableAmendments()}; + auto const preFixRules = preFix.current()->rules(); + auto const postFixRules = postFix.current()->rules(); + + Account const alice{"alice"}; + Account const sponsor{"sponsor"}; + Account const counterparty{"counterparty"}; + for (auto* env : {&preFix, &postFix}) + { + env->fund(XRP(10'000), alice, sponsor, counterparty); + env->close(); + } + + // Both directions for a transaction whose role signature sits in the + // field that makeTx signs. makeTx builds the transaction in the Env it + // is given, so the role signature carries that era's prefix. + auto checkBothDirections = [&](std::function const& makeTx) { + // Direction 1: a good verdict under the old prefix must not let a + // signature moved between roles survive the amendment. + { + auto const jt = makeTx(preFix); + if (!BEAST_EXPECT(jt.stx)) + return; + + auto& router = preFix.app().getHashRouter(); + BEAST_EXPECT(checkValidity(router, *jt.stx, preFixRules).first == Validity::Valid); + + // Same router, asked again under the post-fix rules. The Valid + // verdict above was reached under the old prefix and must not + // be reused, or a signature moved between roles would survive + // the amendment. + BEAST_EXPECT( + checkValidity(router, *jt.stx, postFixRules).first == Validity::SigBad); + } + + // Direction 2: a bad verdict under the old prefix must not condemn + // a transaction that the new prefixes accept. A node whose + // validated rules still lag the open ledger will run this check + // pre-fix first and reject a correctly new-prefix-signed + // transaction; the post-fix check must then verify it afresh + // instead of reusing the pre-fix verdict. + { + auto const jt = makeTx(postFix); + if (!BEAST_EXPECT(jt.stx)) + return; + + auto& router = postFix.app().getHashRouter(); + BEAST_EXPECT(checkValidity(router, *jt.stx, preFixRules).first == Validity::SigBad); + BEAST_EXPECT(checkValidity(router, *jt.stx, postFixRules).first == Validity::Valid); + } + }; + + // sfSponsorSignature, which uses the SPN and SPM prefixes. + checkBothDirections([&](Env& env) { + return env.jt( + noop(alice), + Fee(XRP(1)), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor)); + }); + + // sfCounterpartySignature, which uses its own prefixes, CPT and CPM, + // and only appears on a LoanSet. The transaction does not have to be + // applicable: checkValidity verifies signatures without consulting the + // ledger, so a placeholder LoanBrokerID is enough. + checkBothDirections([&](Env& env) { + return env.jt( + loan::set(alice, uint256{1}, Number{1}), + loan::kCounterparty(counterparty), + Fee(XRP(1)), + Sig(sfCounterpartySignature, counterparty)); + }); } void diff --git a/src/test/jtx/impl/multisign.cpp b/src/test/jtx/impl/multisign.cpp index d948042bda..e04e1bb58a 100644 --- a/src/test/jtx/impl/multisign.cpp +++ b/src/test/jtx/impl/multisign.cpp @@ -60,10 +60,12 @@ signers(Account const& account, NoneT) //------------------------------------------------------------------------------ void -Msig::operator()(Env& env, JTx& jt) const +Msig::operator()(Env&, JTx& jt) const { auto const mySigners = signers; - auto callback = [subField = subField, mySigners, &env](Env&, JTx& jtx) { + auto callback = [subField = subField, mySigners](Env& env, JTx& jtx) { + auto const prefix = + signingPrefix(jtx::signatureRole(subField), true, env.current()->rules()); // Where to put the signature. Supports sfCounterPartySignature and // sfSponsorSignature. auto& sigObject = subField ? jtx[*subField] : jtx.jv; @@ -95,7 +97,7 @@ Msig::operator()(Env& env, JTx& jt) const jo[jss::Account] = e.acct.human(); jo[jss::SigningPubKey] = strHex(e.sig.pk().slice()); - Serializer const ss{buildMultiSigningData(*st, e.acct.id())}; + Serializer const ss{buildMultiSigningData(*st, e.acct.id(), prefix)}; auto const sig = xrpl::sign(*publicKeyType(e.sig.pk().slice()), e.sig.sk(), ss.slice()); jo[sfTxnSignature.getJsonName()] = strHex(Slice{sig.data(), sig.size()}); } diff --git a/src/test/jtx/impl/sig.cpp b/src/test/jtx/impl/sig.cpp index e0123073b1..41833c8802 100644 --- a/src/test/jtx/impl/sig.cpp +++ b/src/test/jtx/impl/sig.cpp @@ -4,6 +4,8 @@ #include #include +#include + namespace xrpl::test::jtx { void @@ -17,11 +19,15 @@ Sig::operator()(Env&, JTx& jt) const { // VFALCO Inefficient pre-C++14 auto const account = *account_; - auto callback = [subField = subField_, account](Env&, JTx& jtx) { + auto callback = [subField = subField_, account](Env& env, JTx& jtx) { // Where to put the signature. Supports sfCounterPartySignature and sfSponsorSignature. auto& sigObject = subField ? jtx[*subField] : jtx.jv; - jtx::sign(jtx.jv, account, sigObject); + jtx::sign( + jtx.jv, + account, + sigObject, + signingPrefix(jtx::signatureRole(subField), false, env.current()->rules())); }; if (subField_ == nullptr) { diff --git a/src/test/jtx/impl/utility.cpp b/src/test/jtx/impl/utility.cpp index c298cee684..f83cb7772c 100644 --- a/src/test/jtx/impl/utility.cpp +++ b/src/test/jtx/impl/utility.cpp @@ -19,9 +19,11 @@ #include #include #include +#include #include #include +#include #include #include @@ -36,12 +38,22 @@ parse(json::Value const& jv) return std::move(*p.object); } +SignatureRole +signatureRole(SField const* subField) +{ + if (subField == nullptr) + return SignatureRole::Transaction; + if (auto const role = xrpl::signatureRole(*subField)) + return *role; + Throw(subField->getName() + " does not hold a transaction signature."); +} + void -sign(json::Value& jv, Account const& account, json::Value& sigObject) +sign(json::Value& jv, Account const& account, json::Value& sigObject, HashPrefix prefix) { sigObject[jss::SigningPubKey] = strHex(account.pk().slice()); Serializer ss; - ss.add32(HashPrefix::TxSign); + ss.add32(prefix); parse(jv).addWithoutSigningFields(ss); auto const sig = xrpl::sign(account.pk(), account.sk(), ss.slice()); sigObject[jss::TxnSignature] = strHex(Slice{sig.data(), sig.size()}); diff --git a/src/test/jtx/utility.h b/src/test/jtx/utility.h index 289afec8b3..5a37abd920 100644 --- a/src/test/jtx/utility.h +++ b/src/test/jtx/utility.h @@ -5,7 +5,10 @@ #include #include #include +#include +#include #include +#include #include #include @@ -33,12 +36,29 @@ struct ParseError : std::logic_error STObject parse(json::Value const& jv); +/** + * The role that signs into an optional signature subfield. + * + * @param subField The signature field, or nullptr for the transaction's own + * signature. Throws if the field does not hold a transaction signature. + */ +SignatureRole +signatureRole(SField const* subField); + /** * Sign automatically into a specific Json field of the jv object. + * + * @param prefix Prefix to insert before the serialized transaction when + * hashing. Use signingPrefix to get the prefix that matches the field that + * holds sigObject. * @note This only works on accounts with multi-signing off. */ void -sign(json::Value& jv, Account const& account, json::Value& sigObject); +sign( + json::Value& jv, + Account const& account, + json::Value& sigObject, + HashPrefix prefix = HashPrefix::TxSign); /** * Sign automatically. diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 777234be83..f310274e56 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -1,9 +1,13 @@ +#include #include #include +#include +#include #include #include #include // IWYU pragma: keep #include +#include #include #include #include @@ -11,10 +15,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -24,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -55,6 +62,279 @@ public: testSTTx(KeyType::Ed25519); testObjectCtorErrors(); testBatchInnerCtorErrors(); + testSigningPrefixes(); + testRoleSignatureBinding(); + testRoleMultiSignatureBinding(); + } + + // Rules with no amendments enabled, and rules with only fixCleanup3_4_0 + // enabled. Rules keep a reference to the presets, so the presets must + // outlive the Rules; both are returned together. + struct RulesFixture + { + std::unordered_set> const noPresets; + std::unordered_set> const fixPresets{fixCleanup3_4_0}; + Rules const legacy{noPresets}; + Rules const fixed{fixPresets}; + }; + + // A transaction with fixed contents, so its signing data is stable from + // run to run. + static STTx + makeFixedTx() + { + auto const keypair = generateKeyPair(KeyType::Secp256k1, generateSeed("masterpassphrase")); + return STTx(ttACCOUNT_SET, [&keypair](auto& obj) { + obj.setAccountID(sfAccount, calcAccountID(keypair.first)); + obj.setFieldAmount(sfFee, STAmount(10ull)); + obj.setFieldU32(sfSequence, 1); + obj.setFieldVL(sfSigningPubKey, keypair.first.slice()); + }); + } + + void + testSigningPrefixes() + { + testcase("signing prefixes"); + + // The prefixes are protocol constants. Spell them out so that a typo + // in a prefix character fails here, and not in a downstream library. + static_assert(safeCast(HashPrefix::TxSign) == 0x53545800); + static_assert(safeCast(HashPrefix::TxMultiSign) == 0x534D5400); + static_assert(safeCast(HashPrefix::CounterpartyTxSign) == 0x43505400); + static_assert(safeCast(HashPrefix::CounterpartyTxMultiSign) == 0x43504D00); + static_assert(safeCast(HashPrefix::SponsorTxSign) == 0x53504E00); + static_assert(safeCast(HashPrefix::SponsorTxMultiSign) == 0x53504D00); + + RulesFixture const r; + + // Every role gets its own prefix once the fix is enabled. + BEAST_EXPECT( + signingPrefix(SignatureRole::Transaction, false, r.fixed) == HashPrefix::TxSign); + BEAST_EXPECT( + signingPrefix(SignatureRole::Transaction, true, r.fixed) == HashPrefix::TxMultiSign); + BEAST_EXPECT( + signingPrefix(SignatureRole::Counterparty, false, r.fixed) == + HashPrefix::CounterpartyTxSign); + BEAST_EXPECT( + signingPrefix(SignatureRole::Counterparty, true, r.fixed) == + HashPrefix::CounterpartyTxMultiSign); + BEAST_EXPECT( + signingPrefix(SignatureRole::Sponsor, false, r.fixed) == HashPrefix::SponsorTxSign); + BEAST_EXPECT( + signingPrefix(SignatureRole::Sponsor, true, r.fixed) == HashPrefix::SponsorTxMultiSign); + + // Before the fix, every role signs the same bytes. + for (auto const role : + {SignatureRole::Transaction, SignatureRole::Counterparty, SignatureRole::Sponsor}) + { + BEAST_EXPECT(signingPrefix(role, false, r.legacy) == HashPrefix::TxSign); + BEAST_EXPECT(signingPrefix(role, true, r.legacy) == HashPrefix::TxMultiSign); + } + + // Each role's field, and each signature field's role, agree. + BEAST_EXPECT(signatureField(SignatureRole::Transaction) == nullptr); + BEAST_EXPECT(*signatureField(SignatureRole::Counterparty) == sfCounterpartySignature); + BEAST_EXPECT(*signatureField(SignatureRole::Sponsor) == sfSponsorSignature); + BEAST_EXPECT(signatureRole(sfCounterpartySignature) == SignatureRole::Counterparty); + BEAST_EXPECT(signatureRole(sfSponsorSignature) == SignatureRole::Sponsor); + BEAST_EXPECT(!signatureRole(sfBook)); + BEAST_EXPECT(!signatureRole(sfSigner)); + + // The bytes signed by an ordinary transaction must not move. Both the + // single- and the multi-signing data are pinned, and neither depends + // on the amendment. + auto const tx = makeFixedTx(); + for (Rules const& rules : {r.legacy, r.fixed}) + { + Serializer single; + single.add32(signingPrefix(SignatureRole::Transaction, false, rules)); + tx.addWithoutSigningFields(single); + // 53545800 STX prefix, 120003 AccountSet, 2400000001 Sequence, + // 68400000000000000A Fee, 7321... SigningPubKey, 8114... Account. + BEAST_EXPECT( + strHex(single.peekData()) == + "53545800" + "120003" + "2400000001" + "68400000000000000A" + "73210330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020" + "8114B5F762798A53D543A014CAF8B297CFF8F2F937E8"); + BEAST_EXPECT( + to_string(single.getSHA512Half()) == + "AB7473EA8D05527A7465229447B0E9B05365C72B87E921762AD30742B253F3E6"); + + auto const signer = calcAccountID( + generateKeyPair(KeyType::Secp256k1, generateSeed("multisigner")).first); + Serializer const multi = buildMultiSigningData( + tx, signer, signingPrefix(SignatureRole::Transaction, true, rules)); + + // The multi-signing data is the single-signing data with a + // different prefix and the signer's account appended. + Serializer expected; + expected.add32(HashPrefix::TxMultiSign); + tx.addWithoutSigningFields(expected); + expected.addBitString(signer); + BEAST_EXPECT(strHex(multi.peekData()) == strHex(expected.peekData())); + } + } + + void + testRoleSignatureBinding() + { + testcase("role signature binding"); + + RulesFixture const r; + + auto const keypair = generateKeyPair(KeyType::Secp256k1, generateSeed("masterpassphrase")); + auto const account = calcAccountID(keypair.first); + + // A transaction signed by its own account, with that signature copied + // into an alternate signature field. sfSponsorSignature is a common + // field; sfCounterpartySignature is only on a LoanSet. + auto makeCopiedSig = [&keypair, &account](SField const& sigField) { + bool const counterparty = sigField == sfCounterpartySignature; + STTx tx(counterparty ? ttLOAN_SET : ttACCOUNT_SET, [&](auto& obj) { + obj.setAccountID(sfAccount, account); + obj.setFieldAmount(sfFee, STAmount(10ull)); + obj.setFieldU32(sfSequence, 1); + obj.setFieldVL(sfSigningPubKey, keypair.first.slice()); + if (counterparty) + { + obj.setFieldH256(sfLoanBrokerID, uint256{1}); + obj.setFieldNumber( + sfPrincipalRequested, STNumber{sfPrincipalRequested, Number{1}}); + } + else + { + obj.setAccountID(sfSponsor, account); + obj.setFieldU32(sfSponsorFlags, 0); + } + }); + tx.sign(keypair.first, keypair.second); + + STObject sigObject(sigField); + sigObject.setFieldVL(sfSigningPubKey, keypair.first.slice()); + sigObject.setFieldVL(sfTxnSignature, tx.getSignature()); + + // NOLINTNEXTLINE(cppcoreguidelines-slicing) + STObject copy{tx}; + copy.setFieldObject(sigField, sigObject); + return STTx{std::move(copy)}; + }; + + for (SField const& sigField : + {std::cref(sfCounterpartySignature), std::cref(sfSponsorSignature)}) + { + auto const tx = makeCopiedSig(sigField); + + // Before the fix, the copied signature verifies in the other role. + BEAST_EXPECT(tx.checkSign(r.legacy)); + + // With the fix, it does not, and the error names the role that + // failed so the two role checks cannot be confused. + auto const ret = tx.checkSign(r.fixed); + BEAST_EXPECT(!ret); + if (!ret) + { + char const* const rolePrefix = + sigField == sfCounterpartySignature ? "Counterparty: " : "Sponsor: "; + BEAST_EXPECT(ret.error().starts_with(rolePrefix)); + BEAST_EXPECT(matches(ret.error().c_str(), "Invalid signature")); + } + } + } + + // Multi-sign analogue of testRoleSignatureBinding. Both the top-level + // Signers array and a role slot's Signers array carry the same signer + // entry, signed under HashPrefix::TxMultiSign. Before the fix every role + // uses that same prefix, so the copied entry verifies in the role slot; + // after the fix the role slot verifies against CounterpartyTxMultiSign + // (CPM) or SponsorTxMultiSign (SPM) and the entry no longer matches. + void + testRoleMultiSignatureBinding() + { + testcase("role multi-signature binding"); + + RulesFixture const r; + + auto const acctKp = generateKeyPair(KeyType::Secp256k1, generateSeed("masterpassphrase")); + auto const account = calcAccountID(acctKp.first); + // The signer must differ from the top-level account so that the + // multiSignHelper "account owner may not multisign for themselves" + // check passes for the top-level Signers array. + auto const signerKp = generateKeyPair(KeyType::Secp256k1, generateSeed("multisigner")); + auto const signerId = calcAccountID(signerKp.first); + + auto makeCopiedMultiSig = [&](SField const& sigField) { + bool const counterparty = sigField == sfCounterpartySignature; + STTx const tx(counterparty ? ttLOAN_SET : ttACCOUNT_SET, [&](auto& obj) { + obj.setAccountID(sfAccount, account); + obj.setFieldAmount(sfFee, STAmount(10ull)); + obj.setFieldU32(sfSequence, 1); + // Empty SigningPubKey selects the multi-sign path. + obj.setFieldVL(sfSigningPubKey, Slice{}); + if (counterparty) + { + obj.setFieldH256(sfLoanBrokerID, uint256{1}); + obj.setFieldNumber( + sfPrincipalRequested, STNumber{sfPrincipalRequested, Number{1}}); + } + else + { + obj.setAccountID(sfSponsor, account); + obj.setFieldU32(sfSponsorFlags, 0); + } + }); + + // Sign the top-level tx with TxMultiSign, which is what a + // multi-signer of the transaction itself would use. + Serializer const ss = buildMultiSigningData(tx, signerId, HashPrefix::TxMultiSign); + auto const sig = xrpl::sign(signerKp.first, signerKp.second, ss.slice()); + + STObject entry(sfSigner); + entry.setAccountID(sfAccount, signerId); + entry.setFieldVL(sfSigningPubKey, signerKp.first.slice()); + entry.setFieldVL(sfTxnSignature, sig); + STArray signers(sfSigners, 1); + signers.pushBack(entry); + + // Attach the Signers array to the top level so its own signature + // check succeeds, then copy the identical array into the role + // slot. Both arrays carry TxMultiSign-based signatures. + // NOLINTNEXTLINE(cppcoreguidelines-slicing) + STObject copy{tx}; + copy.setFieldArray(sfSigners, signers); + + STObject sigObject(sigField); + sigObject.setFieldVL(sfSigningPubKey, Slice{}); + sigObject.setFieldArray(sfSigners, signers); + copy.setFieldObject(sigField, sigObject); + return STTx{std::move(copy)}; + }; + + for (SField const& sigField : + {std::cref(sfCounterpartySignature), std::cref(sfSponsorSignature)}) + { + auto const tx = makeCopiedMultiSig(sigField); + + // Before the fix, both signature checks use TxMultiSign, so the + // copied Signers array verifies in the role slot as well. + BEAST_EXPECT(tx.checkSign(r.legacy)); + + // With the fix, the role slot uses its own multi-sign prefix + // (CPM or SPM) and the copied signature no longer verifies. The + // error must name the role that failed. + auto const ret = tx.checkSign(r.fixed); + BEAST_EXPECT(!ret); + if (!ret) + { + char const* const rolePrefix = + sigField == sfCounterpartySignature ? "Counterparty: " : "Sponsor: "; + BEAST_EXPECT(ret.error().starts_with(rolePrefix)); + BEAST_EXPECT(matches(ret.error().c_str(), "Invalid signature")); + } + } } void @@ -1555,7 +1835,7 @@ public: auto const id2 = calcAccountID(kp2.first); // Get the stream of the transaction for use in multi-signing. - Serializer const s = buildMultiSigningData(txn, id2); + Serializer const s = buildMultiSigningData(txn, id2, HashPrefix::TxMultiSign); auto const saMultiSignature = sign(kp2.first, kp2.second, s.slice()); diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index b492906a86..a440c9ad1c 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1460,11 +1460,17 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) // NOTE ximinez - I think this check is redundant, // but I'm not 100% sure yet. - // If so, only cost is looking up HashRouter flags. - auto const [validity, reason] = - checkValidity(registry_.get().getHashRouter(), sttx, view->rules()); - XRPL_ASSERT( - validity == Validity::Valid, "xrpl::NetworkOPsImp::processTransaction : valid validity"); + // + // For an ordinary transaction it is: the relay and submit paths have + // already run checkValidity, so this costs a HashRouter lookup. It is not + // redundant for a role-signature transaction while fixCleanup3_4_0 is + // activating. Those paths verify against the validated rules, which lag + // the open ledger rules used here, and checkValidity scopes a cached + // verdict to the rules that reached it, so this call can verify the + // signature again and come to a different answer. SigBad is therefore + // reachable, and the handler below is the correct response to it. + auto const& viewRules = view->rules(); + auto const [validity, reason] = checkValidity(registry_.get().getHashRouter(), sttx, viewRules); // Not concerned with local checks at this point. if (validity == Validity::SigBad) @@ -1472,7 +1478,15 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) JLOG(journal_.info()) << "Transaction has bad signature: " << reason; transaction->setStatus(TransStatus::INVALID); transaction->setResult(temBAD_SIGNATURE); - registry_.get().getHashRouter().setFlags(transaction->getID(), HashRouterFlags::BAD); + // See the matching guard in PeerImp::checkTransaction: only cache + // BAD for a role-signature transaction once fixCleanup3_4_0 is + // enabled on this node. Remove together with the amendment. + if (viewRules.enabled(fixCleanup3_4_0) || + (!sttx.isFieldPresent(sfSponsorSignature) && + !sttx.isFieldPresent(sfCounterpartySignature))) + { + registry_.get().getHashRouter().setFlags(transaction->getID(), HashRouterFlags::BAD); + } return false; } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 3f0b4453b8..c56ea5797f 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -3087,8 +3088,9 @@ PeerImp::checkTransaction( if (checkSignature) { // Check the signature before handing off to the job queue. - if (auto [valid, validReason] = checkValidity( - app_.getHashRouter(), *stx, app_.getLedgerMaster().getValidatedRules()); + auto const& validatedRules = app_.getLedgerMaster().getValidatedRules(); + if (auto [valid, validReason] = + checkValidity(app_.getHashRouter(), *stx, validatedRules); valid != Validity::Valid) { if (!validReason.empty()) @@ -3096,9 +3098,20 @@ PeerImp::checkTransaction( JLOG(pJournal_.debug()) << "Exception checking transaction: " << validReason; } - // Probably not necessary to set HashRouterFlags::BAD, but - // doesn't hurt. - app_.getHashRouter().setFlags(stx->getTransactionID(), HashRouterFlags::BAD); + // For a role-signature transaction, only cache BAD once + // fixCleanup3_4_0 is enabled on this node: the SigBad verdict + // then covers the post-fix prefix and cannot flip back. + // Before the amendment activates, checkValidity's own + // era-scoped cache handles the repeat lookups; setting BAD + // would block a correctly new-prefix-signed transaction until + // the router entry ages out. Remove the guard together with + // the amendment. + if (validatedRules.enabled(fixCleanup3_4_0) || + (!stx->isFieldPresent(sfSponsorSignature) && + !stx->isFieldPresent(sfCounterpartySignature))) + { + app_.getHashRouter().setFlags(stx->getTransactionID(), HashRouterFlags::BAD); + } charge(resource::kFeeInvalidSignature, "check transaction signature failure"); return; } diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 9c97577b27..3e9f62214e 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -460,7 +460,8 @@ transactionPreProcessImpl( Role role, SigningForParams& signingArgs, std::chrono::seconds validatedLedgerAge, - Application& app) + Application& app, + Rules const& rules) { auto j = app.getJournal("RPCHandler"); @@ -482,13 +483,16 @@ transactionPreProcessImpl( }(); // Make sure the signature target field is valid, if specified, and save the - // template for use later + // template for use later. Only a field that holds a transaction signature + // is a valid target; the signature is bound to that field's role. auto const signatureTemplate = signatureTarget ? InnerObjectFormats::getInstance().findSOTemplateBySField(*signatureTarget) : nullptr; + auto const signatureRoleOpt = + signatureTarget ? signatureRole(signatureTarget->get()) : SignatureRole::Transaction; if (signatureTarget) { - if (signatureTemplate == nullptr) + if (!signatureRoleOpt || signatureTemplate == nullptr) { // Invalid target field return rpc::makeError(RpcInvalidParams, signatureTarget->get().getName()); } @@ -687,7 +691,8 @@ transactionPreProcessImpl( // If multisign then return multiSignature, else set TxnSignature field. if (signingArgs.isMultiSigning()) { - Serializer const s = buildMultiSigningData(*stTx, signingArgs.getSigner()); + Serializer const s = buildMultiSigningData( + *stTx, signingArgs.getSigner(), signingPrefix(*signatureRoleOpt, true, rules)); auto multisig = xrpl::sign(pk, sk, s.slice()); @@ -695,7 +700,7 @@ transactionPreProcessImpl( } else if (signingArgs.isSingleSigning()) { - stTx->sign(pk, sk, signatureTarget); + stTx->sign(pk, sk, *signatureRoleOpt, rules); } return TransactionPreProcessResult{std::move(stTx)}; @@ -1007,18 +1012,20 @@ transactionSign( { using namespace detail; + // Sign and verify against the same ruleset: a ledger close in between + // could change the signing prefix of an alternate signature field. + std::shared_ptr const ledger = app.getOpenLedger().current(); auto j = app.getJournal("RPCHandler"); JLOG(j.debug()) << "transactionSign: " << jvRequest; // Add and amend fields based on the transaction type. SigningForParams signForParams; - TransactionPreProcessResult const preprocResult = - transactionPreProcessImpl(jvRequest, role, signForParams, validatedLedgerAge, app); + TransactionPreProcessResult const preprocResult = transactionPreProcessImpl( + jvRequest, role, signForParams, validatedLedgerAge, app, ledger->rules()); if (!preprocResult.second) return preprocResult.first; - std::shared_ptr const ledger = app.getOpenLedger().current(); // Make sure the STTx makes a legitimate Transaction. std::pair const txn = transactionConstructImpl(preprocResult.second, ledger->rules(), app); @@ -1050,8 +1057,8 @@ transactionSubmit( // Add and amend fields based on the transaction type. SigningForParams signForParams; - TransactionPreProcessResult const preprocResult = - transactionPreProcessImpl(jvRequest, role, signForParams, validatedLedgerAge, app); + TransactionPreProcessResult const preprocResult = transactionPreProcessImpl( + jvRequest, role, signForParams, validatedLedgerAge, app, ledger->rules()); if (!preprocResult.second) return preprocResult.first; @@ -1218,8 +1225,8 @@ transactionSignFor( // Add and amend fields based on the transaction type. SigningForParams signForParams(*signerAccountID); - TransactionPreProcessResult const preprocResult = - transactionPreProcessImpl(jvRequest, role, signForParams, validatedLedgerAge, app); + TransactionPreProcessResult const preprocResult = transactionPreProcessImpl( + jvRequest, role, signForParams, validatedLedgerAge, app, ledger->rules()); if (!preprocResult.second) return preprocResult.first; From 827b50f169eeb53b3f5696e7b01079c5d6a8940e Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:56:30 +0000 Subject: [PATCH 04/16] fix: Flaky online delete tests, and cover the health checks added in #5531 (#8137) --- src/test/app/LedgerMaster_test.cpp | 149 +++- src/test/app/SHAMapStore_test.cpp | 1085 +++++++++++++++++++++++++++- 2 files changed, 1194 insertions(+), 40 deletions(-) diff --git a/src/test/app/LedgerMaster_test.cpp b/src/test/app/LedgerMaster_test.cpp index ece25356fd..2f2f81bb8a 100644 --- a/src/test/app/LedgerMaster_test.cpp +++ b/src/test/app/LedgerMaster_test.cpp @@ -11,14 +11,18 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include +#include +#include #include #include @@ -115,6 +119,96 @@ class LedgerMaster_test : public beast::unit_test::Suite } } + // Wait until the SHAMapStore has finished processing the ledger that the + // preceding env.close() produced. + // + // env.close() returns as soon as the ledger_accept RPC returns, but the + // validated ledger path -- LedgerMaster::setValidLedger() -> + // SHAMapStore::onLedgerClosed() -- runs on a job queue thread. Without + // draining the job queue first, the store may not have been handed the + // ledger at all, in which case rendezvous() observes working_ == false and + // returns immediately, before any work has been done. + [[nodiscard]] static bool + syncStore(jtx::Env& env) + { + // Drain the job queue first, so that onLedgerClosed() has run and + // working_ is set. Then use the store's timeout overload, so a store + // that never finishes fails this test instead of blocking on it. + // + // Only the second wait is bounded: JobQueue::rendezvous() has no + // timeout overload, so a job that never completes hangs here. That is + // pre-existing -- ~AppBundle waits on it the same way for every jtx + // test -- but it does mean this helper is not hang-proof end to end. + env.app().getJobQueue().rendezvous(); + return env.app().getSHAMapStore().rendezvous(std::chrono::seconds{60}); + } + + // Bring the SHAMapStore to the point where it has been handed a validated + // ledger and initialized lastRotated, and report how many extra ledgers had + // to be closed to get it there (normally none). Returns std::nullopt if + // syncStore() itself failed. + // + // syncStore() alone does not guarantee that, because + // SHAMapStoreImp::run()'s loop does not use the notification and the + // working_ flag safely: + // + // * onLedgerClosed() notifies cond_ whether or not run()'s thread is + // parked on it, and run() waits on cond_ without a predicate, so a + // notification that lands while the thread is still starting up -- + // before it first reaches that wait -- is lost. + // * run() clears working_ at the top of its loop without checking + // whether newLedger_ is still set, so rendezvous() can report the + // store idle with a validated ledger queued. + // + // Either way the store ends up parked with work pending, and only another + // notification gets it moving again. In a standalone test nothing else + // closes ledgers, so that has to come from here: this closes a ledger + // rather than polling getLastRotated(), because polling would just time + // out. onLedgerClosed() keeps only the most recent ledger in newLedger_, + // so the ledger the store picks up -- and therefore lastRotated -- is a + // timing detail, which is why the caller derives its expectations from the + // value it observes instead of assuming one. + // + // run() is deliberately left as it is. In production the only effect is + // latency: the trigger is validatedSeq >= lastRotated + deleteInterval, so + // a lost notification delays rotation to the next validated ledger and + // nothing is skipped or accumulated -- starting at 513 instead of 512 does + // not matter. Two consequences do follow from leaving it in place, and both + // hold today: nothing in production decides anything from working_ or + // rendezvous() (rendezvous() has no production callers at all), and a node + // whose ledgers only advance on demand -- standalone, driven by + // ledger_accept -- can sit on a queued ledger until something closes the + // next one, which is exactly the situation this helper is working around. + // + // So this helper is permanent rather than a stopgap. Working around the + // race must not make it invisible, so every extra close is logged. That + // keeps how often it is actually hit observable in the unit test output -- + // which is the only signal left once this testcase stops flaking on it. + [[nodiscard]] std::optional + initializeStore(jtx::Env& env, int const maxExtraCloses = 3) + { + auto& store = env.app().getSHAMapStore(); + + for (int extraCloses = 0;; ++extraCloses) + { + if (!syncStore(env)) + return std::nullopt; + if (store.getLastRotated() != 0 || extraCloses == maxExtraCloses) + { + if (extraCloses != 0) + { + log << "initializeStore: the store needed " << extraCloses + << " extra ledger close(s) to pick up a validated ledger. " + "SHAMapStoreImp::run() dropped the notification for the " + "first one; see the comment on initializeStore()." + << std::endl; + } + return extraCloses; + } + env.close(); + } + } + void testCompleteLedgerRange(FeatureBitset features) { @@ -136,15 +230,42 @@ class LedgerMaster_test : public beast::unit_test::Suite auto& lm = env.app().getLedgerMaster(); LedgerIndex minSeq = 2; - LedgerIndex maxSeq = env.closed()->header().seq; auto& store = env.app().getSHAMapStore(); - BEAST_EXPECT(store.rendezvous()); + // Which of the existing complete ledgers the store initializes + // lastRotated from is a timing detail; all this test needs is that it is + // one of them. Everything below derives from the observed value rather + // than assuming a particular one. + // + // The range check and the initializeStore() one both end the testcase + // rather than merely reporting, because lastRotated is the only value + // from the store that enters minSeq. A lastRotated of 0 -- the value + // getLastRotated() reports until the store has been handed a + // validated ledger -- makes minSeq 0 below, and the minSeq - 1 and + // minSeq - 2 ranges then underflow to first > last, which aborts a + // Debug build inside missingFromCompleteLedgerRange(). + auto const extraCloses = initializeStore(env); + if (!BEAST_EXPECT(extraCloses.has_value())) + return; + LedgerIndex maxSeq = env.closed()->header().seq; LedgerIndex lastRotated = store.getLastRotated(); - BEAST_EXPECTS(maxSeq == 3, to_string(maxSeq)); - BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers()); - BEAST_EXPECTS(lastRotated == 3, to_string(lastRotated)); + if (!BEAST_EXPECTS(lastRotated >= minSeq && lastRotated <= maxSeq, to_string(lastRotated))) + return; + // The BEAST_EXPECT above already returned if this is nullopt, but that + // is invisible to clang-tidy's optional model. + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + BEAST_EXPECTS(maxSeq == 3 + *extraCloses, to_string(maxSeq)); + std::stringstream initialRange; + initialRange << minSeq << "-" << maxSeq; + BEAST_EXPECTS(lm.getCompleteLedgers() == initialRange.str(), lm.getCompleteLedgers()); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0); - BEAST_EXPECT(minSeq + 1 > maxSeq - 1); + // The inner range is empty unless initializeStore() had to close extra + // ledgers, and missingFromCompleteLedgerRange() treats first > last as a + // precondition violation that aborts a Debug build via UNREACHABLE, so + // only check it when it is well formed. + if (minSeq + 1 <= maxSeq - 1) + { + BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0); + } BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2); @@ -157,7 +278,7 @@ class LedgerMaster_test : public beast::unit_test::Suite env(noop(alice)); } env.close(); - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ++maxSeq; @@ -173,7 +294,19 @@ class LedgerMaster_test : public beast::unit_test::Suite expectedRange << minSeq << "-" << maxSeq; BEAST_EXPECTS(lm.getCompleteLedgers() == expectedRange.str(), lm.getCompleteLedgers()); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0); - BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0); + // missingFromCompleteLedgerRange() treats first > last as a + // precondition violation and aborts a Debug build via UNREACHABLE. + // The range can only collapse if this test's model of minSeq / + // maxSeq has desynced from the store, so report that as a failure + // instead of taking down the whole unit test job. + if (minSeq + 1 <= maxSeq - 1) + { + BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0); + } + else + { + BEAST_EXPECTS(false, to_string(minSeq) + "-" + to_string(maxSeq)); + } BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2); diff --git a/src/test/app/SHAMapStore_test.cpp b/src/test/app/SHAMapStore_test.cpp index 0a8c51c56a..4a69ac17f9 100644 --- a/src/test/app/SHAMapStore_test.cpp +++ b/src/test/app/SHAMapStore_test.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -11,10 +12,13 @@ #include #include +#include #include #include +#include #include #include +#include #include #include #include @@ -26,14 +30,21 @@ #include #include +#include #include +#include +#include #include #include +#include #include +#include #include #include #include +#include #include +#include #include #include #include @@ -44,7 +55,170 @@ namespace xrpl::test { class SHAMapStore_test : public beast::unit_test::Suite { - static auto const kDeleteInterval = 8; + static constexpr int kDeleteInterval = 8; + + // Mirrors SHAMapStoreImp::kMinimumDeletionIntervalSa, the floor that + // online_delete is held to in standalone mode. Note that the + // max_waiting_ledgers floor is derived from this minimum rather than from + // the configured interval -- SHAMapStoreImp.cpp computes it as + // minInterval / 4 -- so both constants below stay put if kDeleteInterval is + // ever raised. + static constexpr int kMinDeleteInterval = 8; + static constexpr int kMinWaitingLedgers = kMinDeleteInterval / 4; + static_assert(kDeleteInterval >= kMinDeleteInterval); + + // The two wait durations healthWait() can choose from, spelled as they + // appear in its log message. onlineDelete() below sets + // recovery_wait_seconds to 1, so the full wait is 1000ms and the shortened + // wait is a tenth of that. Note that "Waiting 1000ms" does not contain + // "Waiting 100ms", so the two are distinguishable by substring. + static constexpr char const* kFullWait = "Waiting 1000ms for node to stabilize"; + static constexpr char const* kShortWait = "Waiting 100ms for node to stabilize"; + + // Distinctive fragments of the other messages the tests below key off. Each + // is unique among everything SHAMapStoreImp logs, so a substring match + // identifies the message unambiguously. + // + // kRotating is logged once run() has committed to a rotation, immediately + // after the health check that gates it, and kFinished once a rotation has + // run to completion. kExpired is logged by healthWait() when the circuit + // breaker trips. + static constexpr char const* kRotating = "rotating"; + static constexpr char const* kFinished = "finished rotation"; + static constexpr char const* kExpired = "unable to make progress"; + + // A Logs implementation that records every message the store's own + // partition emits, keeping each message's severity alongside its text, and + // lets a test block until a given message has appeared. + // + // Severity is recorded because healthWait() picks the severity and the wait + // duration together, so the pair identifies which of its three logging + // branches ran: warn at the full wait when the server is unhealthy for a + // reason that is not expected to resolve on its own, trace at a tenth of + // the wait when the only missing ledger is the one currently being built, + // and info at the full wait otherwise. Matching on the pair is what lets + // the tests below assert which branch was taken instead of merely that some + // wait happened. + // + // waitFor() exists because run() logs on entry to a rotation, which is the + // only signal a test has that the store has passed the health check gating + // the rotation and is now inside it. Several of the branches under test are + // only reachable from there, and no other handle on the store exposes it. + class StoreLogs : public Logs + { + mutable std::mutex mutex_; + std::condition_variable cond_; + std::vector> messages_; + + class Sink : public beast::Journal::Sink + { + StoreLogs& owner_; + + public: + Sink(beast::Severity threshold, StoreLogs& owner) + : beast::Journal::Sink(threshold, false), owner_(owner) + { + } + + // Env::AppBundle calls Logs::threshold() after the Application is + // built, which would otherwise raise this sink above Trace and + // discard the messages the buildingIndex branch logs. + void + threshold(beast::Severity) override + { + } + + void + write(beast::Severity level, std::string const& text) override + { + { + std::scoped_lock const lock(owner_.mutex_); + owner_.messages_.emplace_back(level, text); + } + owner_.cond_.notify_all(); + } + + void + writeAlways(beast::Severity level, std::string const& text) override + { + write(level, text); + } + }; + + // Caller must hold mutex_. A nullopt severity matches any severity. + [[nodiscard]] std::size_t + countLocked(std::optional severity, std::string const& text) const + { + return std::count_if(messages_.begin(), messages_.end(), [&](auto const& message) { + return (!severity || message.first == *severity) && + message.second.find(text) != std::string::npos; + }); + } + + public: + StoreLogs() : Logs(beast::Severity::Trace) + { + } + + // Only the store's own partition is logged at Trace; everything else + // is silenced, so that enabling trace for this one branch does not pay + // for formatting every trace message in the server. + std::unique_ptr + makeSink(std::string const& partition, beast::Severity) override + { + return std::make_unique( + partition == "SHAMapStore" ? beast::Severity::Trace : beast::Severity::Disabled, + *this); + } + + // How many recorded messages were logged at `severity` and contain + // `text`. + [[nodiscard]] std::size_t + count(beast::Severity severity, std::string const& text) const + { + std::scoped_lock const lock(mutex_); + return countLocked(severity, text); + } + + // How many recorded messages contain `text`, at any severity. + [[nodiscard]] std::size_t + count(std::string const& text) const + { + std::scoped_lock const lock(mutex_); + return countLocked(std::nullopt, text); + } + + // Blocks until `text` has been logged at least `expected` times at + // `severity` -- or at any severity, if that is nullopt -- or until the + // timeout expires. Returns whether it got there. + // + // Waiting rather than sleeping-then-counting matters for the branches + // that are only reached after a rotation has started: the store gets + // there when it gets there, so a fixed sleep has to be sized for the + // slowest plausible machine, whereas this returns as soon as the + // message appears. + [[nodiscard]] bool + waitFor( + std::optional severity, + std::string const& text, + std::chrono::milliseconds timeout, + std::size_t expected = 1) + { + std::unique_lock lock(mutex_); + return cond_.wait_for( + lock, timeout, [&] { return countLocked(severity, text) >= expected; }); + } + + // As above, at any severity. + [[nodiscard]] bool + waitFor( + std::string const& text, + std::chrono::milliseconds timeout, + std::size_t expected = 1) + { + return waitFor(std::nullopt, text, timeout, expected); + } + }; static auto onlineDelete(std::unique_ptr cfg) @@ -54,6 +228,32 @@ class SHAMapStore_test : public beast::unit_test::Suite return cfg; } + // online delete tuned so that a rotation, once it has started, spends a + // long time in clearPrior() before reaching the first health check inside + // the rotation body. + // + // clearSql() sleeps back_off_milliseconds at the top of every iteration and + // advances by delete_batch rows per iteration, so a delete_batch of 1 costs + // one sleep per ledger removed, for each of the three tables it is called + // on. That is what gives parkMidRotation() below a window measured in + // seconds rather than in microseconds. delete_batch is therefore the actual + // lever; back_off_milliseconds is set to the value SHAMapStoreImp already + // defaults to, and is spelled out only so the arithmetic above can be + // checked against the config rather than against the implementation. + // + // max_waiting_ledgers is pinned to its floor so that tripping the circuit + // breaker takes the fewest possible ledger closes. + static auto + slowOnlineDelete(std::unique_ptr cfg) + { + cfg = onlineDelete(std::move(cfg)); + auto& section = cfg->section(Sections::kNodeDatabase); + section.set(Keys::kDeleteBatch, "1"); + section.set(Keys::kBackOffMilliseconds, "100"); + section.set(Keys::kMaxWaitingLedgers, std::to_string(kMinWaitingLedgers)); + return cfg; + } + static auto advisoryDelete(std::unique_ptr cfg) { @@ -141,6 +341,96 @@ class SHAMapStore_test : public beast::unit_test::Suite BEAST_EXPECT(env.app().getRelationalDatabase().getAccountTransactionCount() == rows); } + // Wait until the SHAMapStore has finished processing the ledger that the + // preceding env.close() produced. + // + // env.close() returns as soon as the ledger_accept RPC returns, but the + // validated ledger path -- LedgerMaster::setValidLedger() -> + // SHAMapStore::onLedgerClosed() -- runs on a job queue thread. Without + // draining the job queue first, the store may not have been handed the + // ledger at all, in which case rendezvous() observes working_ == false and + // returns immediately, before any work has been done. + [[nodiscard]] static bool + syncStore(jtx::Env& env) + { + // Drain the job queue first, so that onLedgerClosed() has run and + // working_ is set. Then use the store's timeout overload, so a store + // that never finishes fails this test instead of blocking on it. + // + // Only the second wait is bounded: JobQueue::rendezvous() has no + // timeout overload, so a job that never completes hangs here. That is + // pre-existing -- ~AppBundle waits on it the same way for every jtx + // test -- but it does mean this helper is not hang-proof end to end. + env.app().getJobQueue().rendezvous(); + return env.app().getSHAMapStore().rendezvous(std::chrono::seconds{60}); + } + + // Bring the SHAMapStore to the point where it has been handed a validated + // ledger and initialized lastRotated, and report how many extra ledgers had + // to be closed to get it there (normally none). Returns std::nullopt if + // syncStore() itself failed. + // + // syncStore() alone does not guarantee that, because + // SHAMapStoreImp::run()'s loop does not use the notification and the + // working_ flag safely: + // + // * onLedgerClosed() notifies cond_ whether or not run()'s thread is + // parked on it, and run() waits on cond_ without a predicate, so a + // notification that lands while the thread is still starting up -- + // before it first reaches that wait -- is lost. + // * run() clears working_ at the top of its loop without checking + // whether newLedger_ is still set, so rendezvous() can report the + // store idle with a validated ledger queued. + // + // Either way the store ends up parked with work pending, and only another + // notification gets it moving again. In a standalone test nothing else + // closes ledgers, so that has to come from here: this closes a ledger + // rather than polling getLastRotated(), because polling would just time + // out. onLedgerClosed() keeps only the most recent ledger in newLedger_, + // so the ledger the store picks up -- and therefore lastRotated -- is a + // timing detail, which is why the callers derive their expectations from + // the value they observe instead of assuming one. + // + // run() is deliberately left as it is. In production the only effect is + // latency: the trigger is validatedSeq >= lastRotated + deleteInterval, so + // a lost notification delays rotation to the next validated ledger and + // nothing is skipped or accumulated -- starting at 513 instead of 512 does + // not matter. Two consequences do follow from leaving it in place, and both + // hold today: nothing in production decides anything from working_ or + // rendezvous() (rendezvous() has no production callers at all), and a node + // whose ledgers only advance on demand -- standalone, driven by + // ledger_accept -- can sit on a queued ledger until something closes the + // next one, which is exactly the situation this helper is working around. + // + // So this helper is permanent rather than a stopgap. Working around the + // race must not make it invisible, so every extra close is logged. That + // keeps how often it is actually hit observable in the unit test output -- + // which is the only signal left once these testcases stop flaking on it. + [[nodiscard]] std::optional + initializeStore(jtx::Env& env, int const maxExtraCloses = 3) + { + auto& store = env.app().getSHAMapStore(); + + for (int extraCloses = 0;; ++extraCloses) + { + if (!syncStore(env)) + return std::nullopt; + if (store.getLastRotated() != 0 || extraCloses == maxExtraCloses) + { + if (extraCloses != 0) + { + log << "initializeStore: the store needed " << extraCloses + << " extra ledger close(s) to pick up a validated ledger. " + "SHAMapStoreImp::run() dropped the notification for the " + "first one; see the comment on initializeStore()." + << std::endl; + } + return extraCloses; + } + env.close(); + } + } + int waitForReady(jtx::Env& env) { @@ -149,11 +439,11 @@ class SHAMapStore_test : public beast::unit_test::Suite auto& store = env.app().getSHAMapStore(); int ledgerSeq = 3; - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); BEAST_EXPECT(!store.getLastRotated()); env.close(); - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); auto ledger = env.rpc("ledger", "validated"); BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++))); @@ -162,7 +452,417 @@ class SHAMapStore_test : public beast::unit_test::Suite return ledgerSeq; } + // Construct an Env whose config has online_delete enabled and is then + // mutated by `tweak`, and report how SHAMapStoreImp's constructor judged + // it: the message of the exception it threw, or std::nullopt if the Env was + // constructed successfully. + // + // SHAMapStoreImp is built from ApplicationImp's member initializer list, so + // a configuration it rejects surfaces as an exception thrown out of the Env + // constructor rather than as a failure at some later point. + // + // Note that ~AppBundle does not run when the Env constructor throws, so the + // global debug log sink it installed -- which holds a reference to this + // suite -- would outlive the suite. The catch below clears it, so callers + // are free to end on a configuration that is rejected. + std::optional + storeConfigResult(std::function const& tweak) + { + using namespace test::jtx; + + try + { + Env const env{ + *this, + envconfig([&tweak](std::unique_ptr cfg) { + cfg = onlineDelete(std::move(cfg)); + tweak(*cfg); + return cfg; + }), + nullptr, + beast::Severity::Disabled}; + return std::nullopt; + } + // Deliberately broader than the std::runtime_error that + // SHAMapStoreImp throws: an unexpected exception type then shows up as + // a message mismatch naming the actual failure, rather than escaping + // this testcase. + catch (std::exception const& e) + { + // ~AppBundle did not run, so drop the sink it installed by hand + // rather than leaving a reference to this suite live in a global. + setDebugLogSink(nullptr); + return std::string{e.what()}; + } + } + + void + expectConfigRejected(std::string const& expected, std::function const& tweak) + { + auto const result = storeConfigResult(tweak); + BEAST_EXPECTS(result == expected, result.value_or("")); + } + + void + expectConfigAccepted(std::function const& tweak) + { + auto const result = storeConfigResult(tweak); + BEAST_EXPECTS(!result, result.value_or("")); + } + + // The state parkInHealthWait() leaves behind. + struct Parked + { + // Value of getLastRotated() before the rotation attempt began. The + // store must still report this for as long as it stays parked. + LedgerIndex lastRotated = 0; + // The validated ledger the store is waiting on, and the sequence + // getLastRotated() will report once the rotation finally completes. + LedgerIndex validated = 0; + // Sequence removed from LedgerMaster to create the gap, or 0 if + // `createGap` was false. + LedgerIndex gap = 0; + }; + + // Drive the store to the point where it is parked inside healthWait(), + // unable to proceed with a rotation: close ledgers until one more close + // would make the store due to rotate, optionally remove the newest ledger + // from LedgerMaster so that the attempt sees a gap in the range, close the + // triggering ledger, and then set the operating mode to `modeAfterClose`. + // + // Once parked, the store stays parked indefinitely. Its wait loop reruns + // every recovery_wait_seconds and exits only when the server looks healthy, + // when it is stopped, or when the validated ledger index reaches the + // circuit breaker -- and that index only advances when this test closes + // another ledger. So a caller can establish any server state it likes, + // hold it, and be sure the store observes it. That is what makes the tests + // below state machines rather than races. + // + // Returns std::nullopt if the setup did not reach a parked store, having + // already reported the failure. + std::optional + parkInHealthWait(jtx::Env& env, bool createGap, OperatingMode modeAfterClose) + { + using namespace std::chrono_literals; + using namespace test::jtx; + + auto& lm = env.app().getLedgerMaster(); + auto& store = env.app().getSHAMapStore(); + auto& netOPs = env.app().getOPs(); + + env.fund(XRP(1000), Account("alice")); + env.close(); + if (!BEAST_EXPECT(initializeStore(env).has_value())) + return std::nullopt; + + Parked parked; + // The store adopts the first validated ledger it sees as lastRotated, + // and which one that is depends on timing, so read it rather than + // assuming a value. + parked.lastRotated = store.getLastRotated(); + if (!BEAST_EXPECT(parked.lastRotated)) + return std::nullopt; + + // Close ledgers until the next close is the one that makes + // validatedSeq reach lastRotated + deleteInterval. + LedgerIndex maxSeq = env.closed()->header().seq; + while (maxSeq + 1 < parked.lastRotated + kDeleteInterval) + { + env.close(); + ++maxSeq; + if (!BEAST_EXPECT(syncStore(env))) + return std::nullopt; + if (!BEAST_EXPECTS( + store.getLastRotated() == parked.lastRotated, + std::to_string(store.getLastRotated()))) + return std::nullopt; + } + + // Drop out of FULL before touching LedgerMaster's internals, matching + // testLedgerGaps. This also keeps the store from rotating on the + // triggering close before the caller has set the state it wants + // observed. + netOPs.setMode(OperatingMode::CONNECTED); + + // The gap goes one below the sequence that the close further down makes + // validated, never at that sequence itself: healthWait() derives + // buildingIndex from numMissing == 1 && !haveLedger(index), so a gap at + // the validated index reads as "that ledger is about to be built" and + // takes the short trace wait, whereas a gap below it reads as a + // genuinely incomplete range and takes the full wait. Nothing refills + // the gap, so the wait loop ends only when the circuit breaker trips or + // stop() intervenes. + if (createGap) + { + std::size_t iterations = 30; + while (!lm.haveLedger(maxSeq) && --iterations > 0) + { + std::this_thread::sleep_for(10ms); + } + if (!BEAST_EXPECTS(lm.haveLedger(maxSeq), std::to_string(maxSeq))) + return std::nullopt; + + // Give the server a moment to finish any internal work on the + // ledger about to be removed, as testLedgerGaps does. + std::this_thread::sleep_for(250ms); + + lm.clearLedger(maxSeq); + if (!BEAST_EXPECT(!lm.haveLedger(maxSeq))) + return std::nullopt; + parked.gap = maxSeq; + } + + // This close makes the store due to rotate. + env.close(); + ++maxSeq; + parked.validated = maxSeq; + netOPs.setMode(modeAfterClose); + + // Drain the job queue so that onLedgerClosed() has handed the ledger to + // the store. Without this, working_ may still be false from the + // previous cycle and rendezvous() would report "done" before the store + // has even looked at this ledger. + env.app().getJobQueue().rendezvous(); + + if (!BEAST_EXPECT(!store.rendezvous(1s))) + return std::nullopt; + if (!BEAST_EXPECTS( + store.getLastRotated() == parked.lastRotated, + std::to_string(store.getLastRotated()))) + return std::nullopt; + + return parked; + } + + // Drive the store to the point where it has passed the health check that + // gates a rotation and is inside the rotation body, then make the server + // unhealthy so that the next health check in there parks it. + // + // The gap cannot be created up front the way parkInHealthWait() does it, + // because the gating check would see it and refuse to start the rotation at + // all -- which is what testLedgerGaps() exercises. So this waits for the + // message run() logs immediately after that check, which is the store + // publishing that it is committed to the rotation, and creates the gap then. + // + // The margin that makes that safe is clearPrior(), which runs between the + // log and the first health check inside the rotation. Under + // slowOnlineDelete() it works through three tables one sequence at a time, + // sleeping back_off_milliseconds before each, and checks health after every + // one of those sleeps. So the store spends on the order of a second per + // table repeatedly asking whether it is healthy, against the microseconds + // this function needs to clear a ledger once waitFor() has returned. + // + // The gap has to be the validated ledger itself. healthWait() counts missing + // ledgers over the range from lastGoodValidatedLedger_ to the validated + // index, and run() sets the former to the latter just before starting the + // rotation, so for the duration of the rotation that range begins as a + // single sequence and grows only as the caller closes more ledgers. + // + // Which health check inside the rotation ends up observing the gap is not + // pinned down, and does not need to be: whichever one it is returns the same + // answer, clearPrior() gives up, and run() reaches its first switch on + // healthWait() with the condition still in force. Every assertion below + // holds for any of them. + // + // Returns std::nullopt if the setup did not reach a parked store, having + // already reported the failure. + std::optional + parkMidRotation(jtx::Env& env, StoreLogs& log) + { + using namespace std::chrono_literals; + using namespace test::jtx; + + auto& lm = env.app().getLedgerMaster(); + auto& store = env.app().getSHAMapStore(); + + auto const alice = Account("alice"); + env.fund(XRP(1000), alice); + env.close(); + if (!BEAST_EXPECT(initializeStore(env).has_value())) + return std::nullopt; + + LedgerIndex maxSeq = env.closed()->header().seq; + // Close one ledger, carrying a transaction so that the sequence has rows + // in all three of the tables clearSql() works through. + auto closeOne = [&]() -> bool { + env(noop(alice)); + env.close(); + ++maxSeq; + return BEAST_EXPECT(syncStore(env)); + }; + + // Let one rotation complete before setting up the one to be parked. The + // window this helper depends on only exists once the tables hold a full + // delete interval of rows: on the very first rotation there is at most + // one sequence to remove, so clearSql() sleeps once or not at all. + LedgerIndex const firstRotated = store.getLastRotated(); + if (!BEAST_EXPECT(firstRotated)) + return std::nullopt; + while (store.getLastRotated() == firstRotated) + { + if (!closeOne()) + return std::nullopt; + // The rotation is due once maxSeq reaches firstRotated + + // kDeleteInterval. Allow one close beyond that before giving up, + // rather than closing ledgers forever. + if (!BEAST_EXPECTS(maxSeq <= firstRotated + kDeleteInterval, std::to_string(maxSeq))) + return std::nullopt; + } + + Parked parked; + parked.lastRotated = store.getLastRotated(); + + // Close ledgers until the next close is the one that makes the store due + // to rotate again. + while (maxSeq + 1 < parked.lastRotated + kDeleteInterval) + { + if (!closeOne()) + return std::nullopt; + if (!BEAST_EXPECTS( + store.getLastRotated() == parked.lastRotated, + std::to_string(store.getLastRotated()))) + return std::nullopt; + } + + // One rotation has already been logged, so wait for the next one rather + // than for the first. + auto const rotationsBefore = log.count(kRotating); + + // This close makes the store due to rotate. Deliberately do not drain + // the store here: the point is to interrupt it partway through. + env(noop(alice)); + env.close(); + ++maxSeq; + parked.validated = maxSeq; + + // Draining the job queue, on the other hand, is required. In standalone + // mode switchLCL() inserts the closed ledger into LedgerMaster's + // complete range and then posts an advance job, and publishing the + // ledger from that job inserts it a second time. Clearing the ledger + // between those two inserts does not leave a lasting gap: publication + // puts it straight back, the rotation's health checks see a healthy + // node, and the rotation runs to completion. Waiting for the queue to + // drain closes that window, because publication is what advances + // pubLedger_ -- once it has happened, the ledger is never published, and + // so never inserted, again. + // + // This waits only for the job queue, not for the store, whose thread is + // its own and is the thing being interrupted here. + env.app().getJobQueue().rendezvous(); + + // Publishing the ledger is what advances pubLedger_, so this is the + // observable confirmation that the window above has closed. Asserting it + // here means that if anything ever reopens it, this setup step says so + // directly instead of the tests below failing for reasons that look + // nothing like the cause. + auto const published = lm.getPublishedLedger(); + if (!BEAST_EXPECTS( + published && published->header().seq >= parked.validated, + std::to_string(published ? published->header().seq : 0))) + return std::nullopt; + + if (!BEAST_EXPECT(log.waitFor(kRotating, 10s, rotationsBefore + 1))) + return std::nullopt; + + // The store is now inside clearPrior(). Remove the validated ledger so + // that every health check from here on reports a gap. + if (!BEAST_EXPECTS(lm.haveLedger(parked.validated), std::to_string(parked.validated))) + return std::nullopt; + lm.clearLedger(parked.validated); + parked.gap = parked.validated; + if (!BEAST_EXPECT(!lm.haveLedger(parked.gap))) + return std::nullopt; + + // The rotation must now be stuck. Wait longer than + // recovery_wait_seconds, so that this is a settled state rather than a + // store that has yet to reach its next health check. + if (!BEAST_EXPECT(!store.rendezvous(1500ms))) + return std::nullopt; + if (!BEAST_EXPECTS( + store.getLastRotated() == parked.lastRotated, + std::to_string(store.getLastRotated()))) + return std::nullopt; + // The rotation started, has not finished, and has not yet given up. + if (!BEAST_EXPECTS( + log.count(kRotating) == rotationsBefore + 1, std::to_string(log.count(kRotating)))) + return std::nullopt; + if (!BEAST_EXPECTS(log.count(kFinished) == 1, std::to_string(log.count(kFinished)))) + return std::nullopt; + if (!BEAST_EXPECTS(log.count(kExpired) == 0, std::to_string(log.count(kExpired)))) + return std::nullopt; + + return parked; + } + public: + // Cover the [node_db] validation that SHAMapStoreImp performs when it is + // constructed. The rejected cases stop inside SHAMapStoreImp's constructor, + // so they cost only a partial Application construction; the accepted ones + // start a full node and immediately tear it down. + void + testConfig() + { + testcase("config validation"); + + // online_delete below the standalone minimum. ledger_history is still + // kDeleteInterval here, so it is too large for this online_delete as + // well; the assertion pins which of the two errors wins. + expectConfigRejected( + "online_delete must be at least " + std::to_string(kMinDeleteInterval), + [](Config& cfg) { + cfg.section(Sections::kNodeDatabase) + .set(Keys::kOnlineDelete, std::to_string(kMinDeleteInterval - 1)); + }); + + // ledger_history above online_delete asks the node to retain more + // history than online delete is allowed to keep. + expectConfigRejected( + "online_delete must not be less than ledger_history (currently " + + std::to_string(kDeleteInterval + 1) + ")", + [](Config& cfg) { cfg.ledgerHistory = kDeleteInterval + 1; }); + + // recovery_wait_seconds is the interval at which online delete rechecks + // the node's health while it waits for missing ledgers to arrive, so a + // zero wait would turn that into a spin. + expectConfigRejected("recovery_wait_seconds must be at least 1 second", [](Config& cfg) { + cfg.section(Sections::kNodeDatabase).set(Keys::kRecoveryWaitSeconds, "0"); + }); + + // max_waiting_ledgers is the circuit breaker that eventually lets + // online delete stop waiting, so it has a floor rather than being + // free-form. + auto const tooFewWaiting = + "max_waiting_ledgers must be at least " + std::to_string(kMinWaitingLedgers); + expectConfigRejected(tooFewWaiting, [](Config& cfg) { + cfg.section(Sections::kNodeDatabase) + .set(Keys::kMaxWaitingLedgers, std::to_string(kMinWaitingLedgers - 1)); + }); + // 0 is not a magic "never give up" value, just a value below the floor. + expectConfigRejected(tooFewWaiting, [](Config& cfg) { + cfg.section(Sections::kNodeDatabase).set(Keys::kMaxWaitingLedgers, "0"); + }); + + // The floor itself is accepted, and so is a value far above + // online_delete: there is no upper bound. + expectConfigAccepted([](Config& cfg) { + cfg.section(Sections::kNodeDatabase) + .set(Keys::kMaxWaitingLedgers, std::to_string(kMinWaitingLedgers)); + }); + expectConfigAccepted([](Config& cfg) { + cfg.section(Sections::kNodeDatabase) + .set(Keys::kMaxWaitingLedgers, std::to_string(kDeleteInterval * 100)); + }); + + // All of the above is gated on online_delete being enabled. With it + // turned off, the same values are ignored rather than rejected. + expectConfigAccepted([](Config& cfg) { + auto& section = cfg.section(Sections::kNodeDatabase); + section.set(Keys::kOnlineDelete, "0"); + section.set(Keys::kMaxWaitingLedgers, "0"); + section.set(Keys::kRecoveryWaitSeconds, "0"); + }); + } + void testClear() { @@ -233,7 +933,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(kDeleteInterval + 4))); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); BEAST_EXPECT(store.getLastRotated() == kDeleteInterval + 3); lastRotated = store.getLastRotated(); @@ -260,7 +960,7 @@ public: !getHash(ledgers[i]).empty()); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); BEAST_EXPECT(store.getLastRotated() == kDeleteInterval + lastRotated); @@ -298,7 +998,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); // The database will always have back to ledger 2, // regardless of lastRotated. @@ -313,7 +1013,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); BEAST_EXPECT(lastRotated != store.getLastRotated()); @@ -329,7 +1029,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, kDeleteInterval + 1, lastRotated); BEAST_EXPECT(lastRotated != store.getLastRotated()); @@ -368,7 +1068,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - 2, 2); BEAST_EXPECT(lastRotated == store.getLastRotated()); @@ -378,7 +1078,7 @@ public: BEAST_EXPECT(!rpc::containsError(canDelete[jss::result])); BEAST_EXPECT(canDelete[jss::result][jss::can_delete] == ledgerSeq + (kDeleteInterval / 2)); - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - 2, 2); BEAST_EXPECT(store.getLastRotated() == lastRotated); @@ -391,7 +1091,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); @@ -407,7 +1107,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); BEAST_EXPECT(store.getLastRotated() == lastRotated); @@ -419,7 +1119,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - firstBatch, firstBatch); @@ -441,7 +1141,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); BEAST_EXPECT(store.getLastRotated() == lastRotated); @@ -453,7 +1153,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); @@ -474,7 +1174,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); BEAST_EXPECT(store.getLastRotated() == lastRotated); @@ -486,7 +1186,7 @@ public: BEAST_EXPECT(goodLedger(env, ledger, std::to_string(ledgerSeq++), true)); } - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); @@ -634,19 +1334,45 @@ public: auto& lm = env.app().getLedgerMaster(); LedgerIndex minSeq = 2; - LedgerIndex maxSeq = env.closed()->header().seq; auto& store = env.app().getSHAMapStore(); - LedgerIndex lastRotated = store.getLastRotated(); auto& netOPs = env.app().getOPs(); - while (lastRotated != 3) - { - BEAST_EXPECT(store.rendezvous()); - lastRotated = store.getLastRotated(); - } - BEAST_EXPECTS(maxSeq == 3, std::to_string(maxSeq)); - BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers()); + // Which of the existing complete ledgers the store initializes + // lastRotated from is a timing detail, so everything below derives from + // the observed value rather than assuming a particular one. Spinning + // until it equals a hard-coded value never terminates when a different + // one legitimately wins. + // + // The range check and the initializeStore() one both end the testcase + // rather than merely reporting, because lastRotated is the only value + // from the store that enters minSeq. A lastRotated of 0 -- the value + // getLastRotated() reports until the store has been handed a validated + // ledger -- makes minSeq 0 below, and the minSeq - 1 and minSeq - 2 + // ranges then underflow to first > last, which aborts a Debug build + // inside missingFromCompleteLedgerRange(). + auto const extraCloses = initializeStore(env); + if (!BEAST_EXPECT(extraCloses.has_value())) + return; + LedgerIndex maxSeq = env.closed()->header().seq; + LedgerIndex lastRotated = store.getLastRotated(); + if (!BEAST_EXPECTS( + lastRotated >= minSeq && lastRotated <= maxSeq, std::to_string(lastRotated))) + return; + // The BEAST_EXPECT above already returned if this is nullopt, but that + // is invisible to clang-tidy's optional model. + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + BEAST_EXPECTS(maxSeq == 3 + *extraCloses, std::to_string(maxSeq)); + std::stringstream initialRange; + initialRange << minSeq << "-" << maxSeq; + BEAST_EXPECTS(lm.getCompleteLedgers() == initialRange.str(), lm.getCompleteLedgers()); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0); - BEAST_EXPECT(minSeq + 1 > maxSeq - 1); + // The inner range is empty unless initializeStore() had to close extra + // ledgers, and missingFromCompleteLedgerRange() treats first > last as a + // precondition violation that aborts a Debug build via UNREACHABLE, so + // only check it when it is well formed. + if (minSeq + 1 <= maxSeq - 1) + { + BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0); + } BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2); BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2); @@ -759,7 +1485,7 @@ public: env(noop(alice)); } env.close(); - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); ++maxSeq; @@ -825,7 +1551,7 @@ public: lm.getCompleteLedgers())); // The circuit breaker has been triggered. - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); } { // Recover before the circuit breaker triggers, so the test can continue. @@ -870,7 +1596,7 @@ public: deleteSeqs.pop_back(); // Wait for the rotation to finish - BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECT(syncStore(env)); minSeq = lastRotated; while (deleteSeqs.front() < minSeq) @@ -894,8 +1620,20 @@ public: failureMessage("CompleteLedgers", expected, lm.getCompleteLedgers())); } BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == deleteSeqs.size()); - BEAST_EXPECT( - lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == deleteSeqs.size()); + // missingFromCompleteLedgerRange() treats first > last as a + // precondition violation and aborts a Debug build via UNREACHABLE. + // The range can only collapse if this test's model of minSeq / + // maxSeq has desynced from the store, so report that as a failure + // instead of taking down the whole unit test job. + if (minSeq + 1 <= maxSeq - 1) + { + BEAST_EXPECT( + lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == deleteSeqs.size()); + } + else + { + BEAST_EXPECTS(false, failureMessage("range collapsed", minSeq, maxSeq)); + } BEAST_EXPECT( lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == deleteSeqs.size() + 2); BEAST_EXPECT( @@ -905,14 +1643,297 @@ public: } } + // Cover the branches of SHAMapStoreImp::healthWait() that decide whether + // the server is healthy enough to rotate, and how loudly to complain while + // it is not. testLedgerGaps() covers the case where a gap holds the + // rotation back until the circuit breaker trips; these cover the rest of + // the decision table. + void + testHealthWaitState() + { + testcase("healthWait server state"); + + using namespace std::chrono_literals; + using namespace test::jtx; + + auto logs = std::make_unique(); + // Not `auto const*`: waitFor() blocks, so it is not const. + auto* const log = logs.get(); + Env env{*this, envconfig(onlineDelete), std::move(logs), beast::Severity::Trace}; + + auto& store = env.app().getSHAMapStore(); + auto& netOPs = env.app().getOPs(); + + // No gap: the only thing holding the store back is the operating mode. + auto const parked = parkInHealthWait(env, false, OperatingMode::CONNECTED); + if (!parked) + return; + + // Hold the non-FULL mode until the store has logged that it is waiting + // on it. With no gap, a fresh validated ledger and a mode that is not + // DISCONNECTED, the only check left that can report unhealthy is + // "mode != FULL", and a mode that is not FULL is not expected to fix + // itself, so the wait is logged at warn, for the full duration. + // + // Waiting for the message rather than sleeping past it is what keeps + // this from depending on how quickly the store gets around to sampling. + // The store cannot leave the wait loop while the mode stays put -- the + // validated ledger index does not advance, so the circuit breaker is + // never reached -- so the rendezvous() below is not racing it. + BEAST_EXPECT(log->waitFor(beast::Severity::Warning, kFullWait, 10s)); + BEAST_EXPECT(!store.rendezvous(10ms)); + BEAST_EXPECT(netOPs.getOperatingMode() != OperatingMode::FULL); + BEAST_EXPECT(netOPs.getOperatingMode() != OperatingMode::DISCONNECTED); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + + // Now make the mode FULL but the validated ledger stale. Advancing the + // clock without closing a ledger ages the validated ledger past + // age_threshold_seconds, which defaults to 60. The mode check can no + // longer be the reason the store is unhealthy, so the age check is. + // + // This one does sleep: what is being asserted is that the store did not + // rotate, and 1500ms is long enough for it to have re-sampled the server + // at least once -- the full wait is 1000ms -- so the age check, not a + // stale sample of the old mode, is what held it back. + auto const closeTime = env.now(); + env.timeKeeper().set(closeTime + 2min); + netOPs.setMode(OperatingMode::FULL); + BEAST_EXPECT(netOPs.getOperatingMode() == OperatingMode::FULL); + BEAST_EXPECT(!store.rendezvous(1500ms)); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + + // Restore the clock. Nothing is wrong any more, so the rotation that + // has been waiting all along runs to completion. + env.timeKeeper().set(closeTime); + BEAST_EXPECT(syncStore(env)); + BEAST_EXPECTS( + store.getLastRotated() == parked->validated, std::to_string(store.getLastRotated())); + } + + void + testHealthWaitGapLevels() + { + testcase("healthWait gap wait levels"); + + using namespace std::chrono_literals; + using namespace test::jtx; + + auto logs = std::make_unique(); + // Not `auto const*`: waitFor() blocks, so it is not const. + auto* const log = logs.get(); + Env env{*this, envconfig(onlineDelete), std::move(logs), beast::Severity::Trace}; + + auto& lm = env.app().getLedgerMaster(); + auto& store = env.app().getSHAMapStore(); + + auto const parked = parkInHealthWait(env, true, OperatingMode::FULL); + if (!parked) + return; + + // The missing ledger is an older one; the validated ledger itself is + // present. The store has no reason to think the gap will close on its + // own, so it waits the full duration and says so at info -- not warn, + // because the server is otherwise healthy and has not been waiting long + // enough to have fallen behind. + BEAST_EXPECT(lm.haveLedger(parked->validated)); + BEAST_EXPECT(!lm.haveLedger(parked->gap)); + BEAST_EXPECT(log->waitFor(beast::Severity::Info, kFullWait, 10s)); + // Nothing so far should have looked like a ledger being built. + BEAST_EXPECT(log->count(beast::Severity::Trace, kShortWait) == 0); + // Nothing fills the gap in, so the store is still in the wait loop. + BEAST_EXPECT(!store.rendezvous(10ms)); + + // Move the gap onto the validated ledger itself. That is the one case + // the store treats as transient -- the ledger is expected to be built + // shortly -- so it drops to trace and waits a tenth as long. Asserting + // that the shortened wait appears only after this swap is what pins the + // branch to the buildingIndex condition, rather than to anything + // incidental about a store that happens to be waiting. + lm.setLedgerRangePresent(parked->gap, parked->gap); + lm.clearLedger(parked->validated); + BEAST_EXPECT(lm.haveLedger(parked->gap)); + BEAST_EXPECT(!lm.haveLedger(parked->validated)); + BEAST_EXPECT(log->waitFor(beast::Severity::Trace, kShortWait, 10s)); + BEAST_EXPECT(!store.rendezvous(10ms)); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + + // Fill it in and the rotation completes. + lm.setLedgerRangePresent(parked->validated, parked->validated); + BEAST_EXPECT(syncStore(env)); + BEAST_EXPECTS( + store.getLastRotated() == parked->validated, std::to_string(store.getLastRotated())); + } + + void + testHealthWaitDisconnected() + { + testcase("healthWait disconnected"); + + using namespace std::chrono_literals; + using namespace test::jtx; + + Env env{*this, envconfig(onlineDelete)}; + + auto& lm = env.app().getLedgerMaster(); + auto& store = env.app().getSHAMapStore(); + auto& netOPs = env.app().getOPs(); + + auto const parked = parkInHealthWait(env, true, OperatingMode::FULL); + if (!parked) + return; + + // While the server is FULL, the gap holds the rotation back. + BEAST_EXPECT(!store.rendezvous(1500ms)); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + + // A disconnected server is not doing any ledger I/O, so the gap cannot + // have been caused by its own activity and will not close until it has + // peers again. The store deliberately takes advantage of that to get as + // much rotation done as possible: this is the one case where a gap does + // not hold online delete back at all. + netOPs.setMode(OperatingMode::DISCONNECTED); + BEAST_EXPECT(netOPs.getOperatingMode() == OperatingMode::DISCONNECTED); + BEAST_EXPECT(syncStore(env)); + BEAST_EXPECTS( + store.getLastRotated() == parked->validated, std::to_string(store.getLastRotated())); + // The rotation ran with the gap still present -- nothing filled it in. + BEAST_EXPECT(!lm.haveLedger(parked->gap)); + } + + void + testHealthWaitStop() + { + testcase("healthWait stop"); + + using namespace test::jtx; + + Env env{*this, envconfig(onlineDelete)}; + + auto& store = env.app().getSHAMapStore(); + + auto const parked = parkInHealthWait(env, true, OperatingMode::FULL); + if (!parked) + return; + + // Stopping the store has to break it out of the wait loop, which it + // would otherwise never leave: the gap is never filled in and the + // validated ledger index never advances to reach the circuit breaker. + // + // stop() joins the store's thread, so its return is the + // synchronisation point here. rendezvous() afterwards is only a + // cross-check, and cannot block: the store is parked in the health + // check that gates a rotation, so Stopping there merely leaves + // readyToRotate false, and run() falls through to the top of its loop, + // where it clears working_ and notifies before returning on stop_. + store.stop(); + BEAST_EXPECT(store.rendezvous()); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + } + + // The two tests below cover the health check that run() performs between the + // stages of a rotation it has already committed to, which is a different + // decision from the one that gates the rotation in the first place: giving + // up here means abandoning work in progress. run() makes it at four points + // -- after clearing prior ledgers, after copying the validated ledger, after + // freshening the caches, and after clearing them -- with the same three-way + // switch each time, and parkMidRotation() parks the store at the first of + // them. + void + testHealthWaitExpiredMidRotation() + { + testcase("healthWait circuit breaker mid-rotation"); + + using namespace test::jtx; + + auto logs = std::make_unique(); + auto* const log = logs.get(); + Env env{*this, envconfig(slowOnlineDelete), std::move(logs), beast::Severity::Trace}; + + auto& lm = env.app().getLedgerMaster(); + auto& store = env.app().getSHAMapStore(); + + auto const parked = parkMidRotation(env, *log); + if (!parked) + return; + + // Advance the validated ledger index past the circuit breaker. The store + // has had no successful health check since the gap appeared, so once the + // index has moved max_waiting_ledgers on from the last one that did + // succeed, it abandons the rotation instead of waiting for the gap + // forever. Nothing here fills the gap in. + for (int i = 0; i < kMinWaitingLedgers; ++i) + { + env.close(); + BEAST_EXPECT(!lm.haveLedger(parked->gap)); + } + + // Abandoning the rotation returns the store to waiting for work, so it + // reports itself idle -- but with lastRotated left where it started, + // unlike the completed rotation parkMidRotation() drove first. + BEAST_EXPECT(syncStore(env)); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + BEAST_EXPECT(log->count(kExpired) > 0); + BEAST_EXPECTS(log->count(kFinished) == 1, std::to_string(log->count(kFinished))); + BEAST_EXPECT(!lm.haveLedger(parked->gap)); + } + + void + testHealthWaitStopMidRotation() + { + testcase("healthWait stop mid-rotation"); + + using namespace test::jtx; + + auto logs = std::make_unique(); + auto* const log = logs.get(); + Env env{*this, envconfig(slowOnlineDelete), std::move(logs), beast::Severity::Trace}; + + auto& store = env.app().getSHAMapStore(); + + auto const parked = parkMidRotation(env, *log); + if (!parked) + return; + + // Stopping has to break the store out of the rotation, which it would + // otherwise never leave: the gap is never filled in and the validated + // ledger index never advances to reach the circuit breaker. Note that + // being stopped outranks being healthy -- the health check reports it + // even when nothing is wrong with the server -- so this does not depend + // on the store still being parked when stop() lands. + // + // stop() joins the store's thread, so its return is the synchronisation + // point. Deliberately do not call the untimed rendezvous() afterwards: + // run() returns without clearing working_, so it would block forever. + store.stop(); + BEAST_EXPECTS( + store.getLastRotated() == parked->lastRotated, std::to_string(store.getLastRotated())); + // The rotation was abandoned rather than completed, and the circuit + // breaker was not what abandoned it. + BEAST_EXPECTS(log->count(kFinished) == 1, std::to_string(log->count(kFinished))); + BEAST_EXPECTS(log->count(kExpired) == 0, std::to_string(log->count(kExpired))); + } + void run() override { + testConfig(); testClear(); testAutomatic(); testCanDelete(); testRotate(); testLedgerGaps(); + testHealthWaitState(); + testHealthWaitGapLevels(); + testHealthWaitDisconnected(); + testHealthWaitStop(); + testHealthWaitExpiredMidRotation(); + testHealthWaitStopMidRotation(); } }; From 3e54e7d00b1be743e5d711904b1f64e4796c7335 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Thu, 3 Sep 2026 19:20:38 +0000 Subject: [PATCH 05/16] fix: Don't use github.workspace as it slow downs gcovr (#8173) --- .github/actions/setup-nix-env/action.yml | 9 +++++---- .github/workflows/build-nix-images.yml | 2 +- .github/workflows/build-packaging-images.yml | 2 +- .github/workflows/build-pre-commit-image.yml | 2 +- .github/workflows/check-tools.yml | 2 +- .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/reusable-package.yml | 4 ++-- .github/workflows/reusable-upload-recipe.yml | 2 +- .github/workflows/upload-conan-deps.yml | 2 +- 12 files changed, 17 insertions(+), 16 deletions(-) diff --git a/.github/actions/setup-nix-env/action.yml b/.github/actions/setup-nix-env/action.yml index a95053e536..38b8365649 100644 --- a/.github/actions/setup-nix-env/action.yml +++ b/.github/actions/setup-nix-env/action.yml @@ -40,10 +40,11 @@ runs: # Unlike the Linux nix images, macOS needs no SSL_CERT_FILE: it has its # own trust store, and pinning would break TLS to hosts relying on it. - # Workspace-local, so `cleanup-workspace` clears it, but not the - # `.conan2` prepare-runner hands the system toolchain: that Conan is a - # different version, and the two would migrate each other's cache. - echo "CONAN_HOME=${{ github.workspace }}/.conan2-nix" >>"${GITHUB_ENV}" + # In RUNNER_TEMP, which the runner empties per job, like the `.conan2` + # prepare-runner hands the system toolchain - but under its own name: + # that Conan is a different version, and the two would migrate each + # other's cache. + echo "CONAN_HOME=${RUNNER_TEMP}/.conan2-nix" >>"${GITHUB_ENV}" # Config, profiles and remote, exactly as the dev shell sets them up on # entry; the `setup-conan` action is skipped for this toolchain. diff --git a/.github/workflows/build-nix-images.yml b/.github/workflows/build-nix-images.yml index a528786dd4..e47a1f93ff 100644 --- a/.github/workflows/build-nix-images.yml +++ b/.github/workflows/build-nix-images.yml @@ -60,7 +60,7 @@ jobs: base_image: debian:bookworm - name: rhel base_image: registry.access.redhat.com/ubi9/ubi:latest - uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@65d5a0bd72be4ecea95cff0673a6e0672ab5243a + uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@034e87065fcd0100214cf0672923bd38d193cf78 with: image_name: xrpld/nix-${{ matrix.distro.name }} dockerfile: nix/docker/Dockerfile diff --git a/.github/workflows/build-packaging-images.yml b/.github/workflows/build-packaging-images.yml index e099decc12..45fe338097 100644 --- a/.github/workflows/build-packaging-images.yml +++ b/.github/workflows/build-packaging-images.yml @@ -41,7 +41,7 @@ jobs: # AlmaLinux rather than UBI, which does not ship rpm-sign. - name: rhel base_image: almalinux:10 - uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@65d5a0bd72be4ecea95cff0673a6e0672ab5243a + uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@034e87065fcd0100214cf0672923bd38d193cf78 with: image_name: xrpld/packaging-${{ matrix.distro.name }} dockerfile: package/docker/Dockerfile diff --git a/.github/workflows/build-pre-commit-image.yml b/.github/workflows/build-pre-commit-image.yml index 71f083b686..fc2d3f2ab2 100644 --- a/.github/workflows/build-pre-commit-image.yml +++ b/.github/workflows/build-pre-commit-image.yml @@ -30,7 +30,7 @@ jobs: permissions: contents: read packages: write - uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@65d5a0bd72be4ecea95cff0673a6e0672ab5243a + uses: XRPLF/actions/.github/workflows/build-multiarch-image.yml@034e87065fcd0100214cf0672923bd38d193cf78 with: image_name: xrpld/pre-commit dockerfile: bin/pre-commit/Dockerfile diff --git a/.github/workflows/check-tools.yml b/.github/workflows/check-tools.yml index 148ee9a781..4b1a81de17 100644 --- a/.github/workflows/check-tools.yml +++ b/.github/workflows/check-tools.yml @@ -79,7 +79,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index c3bbe79109..25063b9dfa 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@be22f05caab3cd98d06012368197fd6cc0635aab + uses: XRPLF/actions/.github/workflows/pre-commit.yml@279ec358f4a1be4088be3e024b07916fa97c75b6 with: runs_on: ubuntu-latest container: '{ "image": "ghcr.io/xrplf/xrpld/pre-commit:sha-473fe44" }' diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index 072d861456..6bc803f1e4 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -47,7 +47,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 13e20b2211..a37ab386b8 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -129,7 +129,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: ${{ inputs.ccache_enabled }} diff --git a/.github/workflows/reusable-clang-tidy.yml b/.github/workflows/reusable-clang-tidy.yml index 5f45fcf732..68ab531882 100644 --- a/.github/workflows/reusable-clang-tidy.yml +++ b/.github/workflows/reusable-clang-tidy.yml @@ -43,7 +43,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false diff --git a/.github/workflows/reusable-package.yml b/.github/workflows/reusable-package.yml index c3df348faa..58adc53dc8 100644 --- a/.github/workflows/reusable-package.yml +++ b/.github/workflows/reusable-package.yml @@ -81,7 +81,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false @@ -261,7 +261,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false diff --git a/.github/workflows/reusable-upload-recipe.yml b/.github/workflows/reusable-upload-recipe.yml index beb1104ab0..69de12db0e 100644 --- a/.github/workflows/reusable-upload-recipe.yml +++ b/.github/workflows/reusable-upload-recipe.yml @@ -50,7 +50,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index 99e25c8914..526b647429 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -68,7 +68,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded + uses: XRPLF/actions/prepare-runner@b3e255d74d785d053e4903da8ac90983cd7d9e82 with: enable_ccache: false From f7f50caa6e2c5cc5cb7f4cc59187e52841493570 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Thu, 3 Sep 2026 19:48:15 +0000 Subject: [PATCH 06/16] docs: Backfill API-CHANGELOG.md for 3.1.1 through 3.3.0 (#8159) --- API-CHANGELOG.md | 72 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 16 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 6f746e85cf..ed27312023 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -22,14 +22,46 @@ API version 2 is available in `xrpld` version 2.0.0 and later. See [API-VERSION- This version is supported by all `xrpld` versions. For WebSocket and HTTP JSON-RPC requests, it is currently the default API version used when no `api_version` is specified. -## Unreleased +## XRP Ledger server version 3.4.0 -This section contains changes targeting a future version. +Version 3.4.0 is not yet released. These changes are available in the 3.4.0 beta releases. -### Additions +### Additions in 3.4.0 + +- `ledger`: `nftoken_id`, `nftoken_ids`, and `offer_id` are now included in transaction metadata when transactions are expanded (`expand`, or admin-only `full`), matching the `tx`, `account_tx`, and `subscribe` (`transactions` stream) responses. ([#5706](https://github.com/XRPLF/rippled/pull/5706)) + +### Bugfixes in 3.4.0 + +- `sign`, `sign_for`, `submit`: `signature_target` now returns `invalidParams` unless it names `CounterpartySignature` or `SponsorSignature`. It previously accepted any inner object field, such as `Book` or `NFToken`, and signed into it. +- `sign`, `sign_for`, `submit`, `submit_multisigned`: With `fixCleanup3_4_0` enabled, a signature in `CounterpartySignature` or `SponsorSignature` covers a different prefix than the transaction's own signature, so a signature can no longer be moved from one of those roles into another. Clients that build these signatures themselves must use the new prefixes: `CPT` and `CPM` (single- and multi-signing) for `CounterpartySignature`, and `SPN` and `SPM` for `SponsorSignature`. +- `get_aggregate_price`: Duplicate entries in the `oracles` request array are now ignored. [#6586](https://github.com/XRPLF/rippled/pull/6586) +- `vault_info`: Errors now identify what the request got wrong instead of reporting every failure as the unregistered token `malformedRequest`, and the `error`, `error_code` and `error_message` fields now agree with each other. An invalid `vault_id` or `seq` returns `invalidParams`, an invalid `owner` returns `actMalformed`, and a request that mixes `vault_id` with `owner`/`seq` or supplies neither returns `invalidParams` with a message naming the accepted combinations. [#8015](https://github.com/XRPLF/rippled/pull/8015) +- `vault_info`: A well-formed all-zero `vault_id` now returns `entryNotFound` instead of being rejected as malformed, and `entryNotFound` responses now include `error_code` and `error_message`. Clients that request `ripplerpc` 3.0 or above therefore receive HTTP 400 with that error rather than HTTP 200. [#8015](https://github.com/XRPLF/rippled/pull/8015) +- `vault_info`: `vault_id` and `owner` must now be strings, matching how `ledger_entry` reads the same fields. An object or an array in either field previously produced an internal error, and a number was silently converted to its decimal text; `vault_id` now returns `invalidParams` and `owner` returns `actMalformed`. [#8015](https://github.com/XRPLF/rippled/pull/8015) +- `gateway_balances`: The `account` and `ident` fields now return an `invalidParams` error if the value is not a string, instead of an `internal` error. [#7655](https://github.com/XRPLF/rippled/pull/7655) +- `account_lines`: The `peer` field now returns an error if the value is not a string. [#7728](https://github.com/XRPLF/rippled/pull/7728) +- `ledger`: `delivered_amount` is now included in the metadata of successful `AccountDelete` transactions when transactions are expanded (`expand`, or admin-only `full`). Previously it was only added for `Payment` and `CheckCash`, which made `ledger` inconsistent with `tx` and `account_tx`. [#5706](https://github.com/XRPLF/rippled/pull/5706) + +## XRP Ledger server version 3.3.0 + +[Version 3.3.0](https://github.com/XRPLF/rippled/releases/tag/3.3.0) was released on Aug 6, 2026. + +### Additions in 3.3.0 + +- `account_tx`: Added an optional `delegate` request object to filter delegated transactions. The object requires `delegate_filter`, which must be either `actor` for transactions owned by the requested account but signed by another account, or `authorizer` for transactions signed by the requested account on behalf of another account. The optional `counter_party` account narrows the results to a specific signer/delegate for `actor` or a specific owner/delegator for `authorizer`. Malformed `delegate`, `delegate_filter`, and `counter_party` values return standard invalid field errors, and invalid account IDs return `actMalformed`. When paginating delegate-filtered queries, a marker from a delegate-filtered query includes a `delegate` flag and is only valid for follow-up requests that also supply `delegate` (mixing marker conventions returns `invalidParams`). Because filtering is applied after the ledger scan, a page may contain fewer results than `limit` (possibly zero) while still returning a marker, so callers must continue until no marker is present. ([#6126](https://github.com/XRPLF/rippled/pull/6126)) + +## XRP Ledger server version 3.2.1 + +[Version 3.2.1](https://github.com/XRPLF/rippled/releases/tag/3.2.1) was released on Aug 1, 2026. + +This release contains bug fixes only and no API changes. + +## XRP Ledger server version 3.2.0 + +[Version 3.2.0](https://github.com/XRPLF/rippled/releases/tag/3.2.0) was released on Jun 16, 2026. + +### Additions in 3.2.0 -- `account_tx`: Added an optional `delegate` request object to filter delegated transactions. The object requires `delegate_filter`, which must be either `actor` for transactions owned by the requested account but signed by another account, or `authorizer` for transactions signed by the requested account on behalf of another account. The optional `counter_party` account narrows the results to a specific signer/delegate for `actor` or a specific owner/delegator for `authorizer`. Malformed `delegate`, `delegate_filter`, and `counter_party` values return standard invalid field errors, and invalid account IDs return `actMalformed`. - When paginating delegate-filtered queries, a marker from a delegate-filtered query includes a `delegate` flag and is only valid for follow-up requests that also supply `delegate` (mixing marker conventions returns `invalidParams`). Because filtering is applied after the ledger scan, a page may contain fewer results than `limit` (possibly zero) while still returning a marker, so callers must continue until no marker is present. - `ledger_entry`, `account_objects`: The `Delegate` ledger entry now includes an optional `DestinationNode` field, which stores the index into the authorized account's owner directory. This field is present on entries created after bidirectional directory tracking was introduced and may appear in RPC responses for those entries. ([#6681](https://github.com/XRPLF/rippled/pull/6681)) - `server_definitions`: Added the following new sections to the response ([#6321](https://github.com/XRPLF/rippled/pull/6321)): - `TRANSACTION_FORMATS`: Describes the fields and their optionality for each transaction type, including common fields shared across all transactions. @@ -37,13 +69,9 @@ This section contains changes targeting a future version. - `TRANSACTION_FLAGS`: Maps transaction type names to their supported flags and flag values. - `LEDGER_ENTRY_FLAGS`: Maps ledger entry type names to their flags and flag values. - `ACCOUNT_SET_FLAGS`: Maps AccountSet flag names (asf flags) to their numeric values. -- `ledger`: `nftoken_id`, `nftoken_ids`, and `offer_id` are now included in transaction metadata when transactions are expanded (`expand`, or admin-only `full`), matching the `tx`, `account_tx`, and `subscribe` (`transactions` stream) responses. ([#5706](https://github.com/XRPLF/rippled/pull/5706)) -### Bugfixes +### Bugfixes in 3.2.0 -- `sign`, `sign_for`, `submit`: `signature_target` now returns `invalidParams` unless it names `CounterpartySignature` or `SponsorSignature`. It previously accepted any inner object field, such as `Book` or `NFToken`, and signed into it. -- `sign`, `sign_for`, `submit`, `submit_multisigned`: With `fixCleanup3_4_0` enabled, a signature in `CounterpartySignature` or `SponsorSignature` covers a different prefix than the transaction's own signature, so a signature can no longer be moved from one of those roles into another. Clients that build these signatures themselves must use the new prefixes: `CPT` and `CPM` (single- and multi-signing) for `CounterpartySignature`, and `SPN` and `SPM` for `SponsorSignature`. -- `get_aggregate_price`: Duplicate entries in the `oracles` request array are now ignored. [#6586](https://github.com/XRPLF/rippled/pull/6586) - Peer Crawler: The `port` field in `overlay.active[]` now consistently returns an integer instead of a string for outbound peers. [#6318](https://github.com/XRPLF/rippled/pull/6318) - `ping`: The `ip` field is no longer returned as an empty string for proxied connections without a forwarded-for header. It is now omitted, consistent with the behavior for identified connections. [#6730](https://github.com/XRPLF/rippled/pull/6730) - gRPC `GetLedgerDiff`: Fixed error message that incorrectly said "base ledger not validated" when the desired ledger was not validated. [#6730](https://github.com/XRPLF/rippled/pull/6730) @@ -55,12 +83,24 @@ This section contains changes targeting a future version. - `submit`: The `fail_hard` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) - `subscribe`: The `taker` field in the `books` array now returns `actMalformed` instead of `badIssuer` if the value is not a valid account. [#6529](https://github.com/XRPLF/rippled/pull/6529) - Fixed a bug in `Forwarded` HTTP header parsing where the extracted IP address could be incorrect when no comma or semicolon delimiter follows the address. This could cause the server to misidentify a client's IP address when operating behind a reverse proxy. [#6529](https://github.com/XRPLF/rippled/pull/6529) -- `vault_info`: Errors now identify what the request got wrong instead of reporting every failure as the unregistered token `malformedRequest`, and the `error`, `error_code` and `error_message` fields now agree with each other. An invalid `vault_id` or `seq` returns `invalidParams`, an invalid `owner` returns `actMalformed`, and a request that mixes `vault_id` with `owner`/`seq` or supplies neither returns `invalidParams` with a message naming the accepted combinations. [#8015](https://github.com/XRPLF/rippled/pull/8015) -- `vault_info`: A well-formed all-zero `vault_id` now returns `entryNotFound` instead of being rejected as malformed, and `entryNotFound` responses now include `error_code` and `error_message`. Clients that request `ripplerpc` 3.0 or above therefore receive HTTP 400 with that error rather than HTTP 200. [#8015](https://github.com/XRPLF/rippled/pull/8015) -- `vault_info`: `vault_id` and `owner` must now be strings, matching how `ledger_entry` reads the same fields. An object or an array in either field previously produced an internal error, and a number was silently converted to its decimal text; `vault_id` now returns `invalidParams` and `owner` returns `actMalformed`. [#8015](https://github.com/XRPLF/rippled/pull/8015) -- `gateway_balances`: The `account` and `ident` fields now return an `invalidParams` error if the value is not a string, instead of an `internal` error. [#7655](https://github.com/XRPLF/rippled/pull/7655) -- `account_lines`: The `peer` field now returns an error if the value is not a string. [#7728](https://github.com/XRPLF/rippled/pull/7728) -- `ledger`: `delivered_amount` is now included in the metadata of successful `AccountDelete` transactions when transactions are expanded (`expand`, or admin-only `full`). Previously it was only added for `Payment` and `CheckCash`, which made `ledger` inconsistent with `tx` and `account_tx`. [#5706](https://github.com/XRPLF/rippled/pull/5706) + +## XRP Ledger server version 3.1.3 + +[Version 3.1.3](https://github.com/XRPLF/rippled/releases/tag/3.1.3) was released on May 8, 2026. + +This release contains bug fixes only and no API changes. + +## XRP Ledger server version 3.1.2 + +[Version 3.1.2](https://github.com/XRPLF/rippled/releases/tag/3.1.2) was released on Mar 12, 2026. + +This release contains bug fixes only and no API changes. + +## XRP Ledger server version 3.1.1 + +[Version 3.1.1](https://github.com/XRPLF/rippled/releases/tag/3.1.1) was released on Feb 23, 2026. + +This release contains bug fixes only and no API changes. ## XRP Ledger server version 3.1.0 From 2ad4def35fd8580da027462517ba3375cc005c94 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 3 Sep 2026 20:07:24 +0000 Subject: [PATCH 07/16] chore: Bump version to 3.4.0-rc1 (#8171) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index 0bc02f98ae..bf67defa3b 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -23,7 +23,7 @@ namespace { //------------------------------------------------------------------------------ // clang-format off // NOLINTNEXTLINE(readability-identifier-naming) -char const* const versionString = "3.4.0-b3" +char const* const versionString = "3.4.0-rc1" // clang-format on ; From d5bfe94f15ece0ba1b6c00004ee5b3f90383f2ab Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Thu, 3 Sep 2026 21:13:06 +0000 Subject: [PATCH 08/16] feat: Support key rotation in MPTokenIssuanceSet (#7915) --- include/xrpl/protocol/Protocol.h | 6 + include/xrpl/protocol/detail/features.macro | 1 + .../xrpl/protocol/detail/ledger_entries.macro | 2 + include/xrpl/protocol/detail/sfields.macro | 4 + .../ledger_entries/MPTokenIssuance.h | 70 ++ .../transactors/token/MPTokenIssuanceSet.cpp | 164 ++++- .../app/ConfidentialMPTKeyRotation_test.cpp | 634 ++++++++++++++++++ src/test/app/ConfidentialTransfer_test.cpp | 18 +- src/test/jtx/impl/mpt.cpp | 32 + src/test/jtx/mpt.h | 15 + .../ledger_entries/MPTokenIssuanceTests.cpp | 54 ++ 11 files changed, 956 insertions(+), 44 deletions(-) create mode 100644 src/test/app/ConfidentialMPTKeyRotation_test.cpp diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index ec9b9ba70a..1b88eea456 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -12,6 +12,7 @@ #include #include #include +#include namespace xrpl { @@ -544,6 +545,11 @@ constexpr std::size_t kEcClawbackProofLength = SECP256K1_COMPACT_CLAWBACK_PROOF_ */ constexpr std::uint32_t kConfidentialFeeMultiplier = 9; +/** + * Maximum value a confidential MPT key epoch may reach. + */ +constexpr std::uint32_t kMaxKeyEpoch = std::numeric_limits::max(); + /** * Compressed EC point prefix for even y-coordinate */ diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index ae696a4ea4..fe49a0230f 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,6 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FEATURE(ConfidentialMPTKeyRotation, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_4_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Sponsor, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(BatchV1_1, Supported::Yes, VoteBehavior::DefaultNo) diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index f166473d7f..be12ad6348 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -408,6 +408,8 @@ LEDGER_ENTRY(ltMPTOKEN_ISSUANCE, 0x007e, MPTokenIssuance, mpt_issuance, ({ {sfReferenceHolding, SoeOptional}, {sfIssuerEncryptionKey, SoeOptional}, {sfAuditorEncryptionKey, SoeOptional}, + {sfIssuerKeyEpoch, SoeOptional}, + {sfAuditorKeyEpoch, SoeOptional}, {sfConfidentialOutstandingAmount, SoeDefault}, })) diff --git a/include/xrpl/protocol/detail/sfields.macro b/include/xrpl/protocol/detail/sfields.macro index ec05804253..0a88a6a191 100644 --- a/include/xrpl/protocol/detail/sfields.macro +++ b/include/xrpl/protocol/detail/sfields.macro @@ -119,6 +119,10 @@ TYPED_SFIELD(sfRemainingOwnerCount, UINT32, 73) TYPED_SFIELD(sfSponsorFlags, UINT32, 74) TYPED_SFIELD(sfSubscriptionDate, UINT32, 75) TYPED_SFIELD(sfRedemptionDate, UINT32, 76) +TYPED_SFIELD(sfIssuerKeyEpoch, UINT32, 77) +TYPED_SFIELD(sfAuditorKeyEpoch, UINT32, 78) +TYPED_SFIELD(sfIssuerKeyMirrorEpoch, UINT32, 79) +TYPED_SFIELD(sfAuditorKeyMirrorEpoch, UINT32, 80) // 64-bit integers (common) TYPED_SFIELD(sfIndexNext, UINT64, 1) diff --git a/include/xrpl/protocol_autogen/ledger_entries/MPTokenIssuance.h b/include/xrpl/protocol_autogen/ledger_entries/MPTokenIssuance.h index 6a2caf52ae..74b5e3c4eb 100644 --- a/include/xrpl/protocol_autogen/ledger_entries/MPTokenIssuance.h +++ b/include/xrpl/protocol_autogen/ledger_entries/MPTokenIssuance.h @@ -351,6 +351,54 @@ public: return this->sle_->isFieldPresent(sfAuditorEncryptionKey); } + /** + * @brief Get sfIssuerKeyEpoch (SoeOptional) + * @return The field value, or std::nullopt if not present. + */ + [[nodiscard]] + protocol_autogen::Optional + getIssuerKeyEpoch() const + { + if (hasIssuerKeyEpoch()) + return this->sle_->at(sfIssuerKeyEpoch); + return std::nullopt; + } + + /** + * @brief Check if sfIssuerKeyEpoch is present. + * @return True if the field is present, false otherwise. + */ + [[nodiscard]] + bool + hasIssuerKeyEpoch() const + { + return this->sle_->isFieldPresent(sfIssuerKeyEpoch); + } + + /** + * @brief Get sfAuditorKeyEpoch (SoeOptional) + * @return The field value, or std::nullopt if not present. + */ + [[nodiscard]] + protocol_autogen::Optional + getAuditorKeyEpoch() const + { + if (hasAuditorKeyEpoch()) + return this->sle_->at(sfAuditorKeyEpoch); + return std::nullopt; + } + + /** + * @brief Check if sfAuditorKeyEpoch is present. + * @return True if the field is present, false otherwise. + */ + [[nodiscard]] + bool + hasAuditorKeyEpoch() const + { + return this->sle_->isFieldPresent(sfAuditorKeyEpoch); + } + /** * @brief Get sfConfidentialOutstandingAmount (SoeDefault) * @return The field value, or std::nullopt if not present. @@ -600,6 +648,28 @@ public: return *this; } + /** + * @brief Set sfIssuerKeyEpoch (SoeOptional) + * @return Reference to this builder for method chaining. + */ + MPTokenIssuanceBuilder& + setIssuerKeyEpoch(std::decay_t const& value) + { + object_[sfIssuerKeyEpoch] = value; + return *this; + } + + /** + * @brief Set sfAuditorKeyEpoch (SoeOptional) + * @return Reference to this builder for method chaining. + */ + MPTokenIssuanceBuilder& + setAuditorKeyEpoch(std::decay_t const& value) + { + object_[sfAuditorKeyEpoch] = value; + return *this; + } + /** * @brief Set sfConfidentialOutstandingAmount (SoeDefault) * @return Reference to this builder for method chaining. diff --git a/src/libxrpl/tx/transactors/token/MPTokenIssuanceSet.cpp b/src/libxrpl/tx/transactors/token/MPTokenIssuanceSet.cpp index e8fd2e22b6..6bd7140631 100644 --- a/src/libxrpl/tx/transactors/token/MPTokenIssuanceSet.cpp +++ b/src/libxrpl/tx/transactors/token/MPTokenIssuanceSet.cpp @@ -119,7 +119,15 @@ MPTokenIssuanceSet::preflight(PreflightContext const& ctx) if (hasHolder && (hasIssuerElGamalKey || hasAuditorElGamalKey)) return temMALFORMED; - if (hasAuditorElGamalKey && !hasIssuerElGamalKey) + // Pre-ConfidentialMPTKeyRotation amendment, the auditor key could not be + // registered independently of the issuer key. The issuer could either: + // - Register only the issuer key (in which case an auditor key could not be added later), or + // - Register both the issuer and auditor keys simultaneously. + // + // Post-ConfidentialMPTKeyRotation amendment, the auditor key can be + // registered after the issuer key has already been registered. + if (hasAuditorElGamalKey && !hasIssuerElGamalKey && + !ctx.rules.enabled(featureConfidentialMPTKeyRotation)) return temMALFORMED; if (hasIssuerElGamalKey && !isValidCompressedECPoint(ctx.tx[sfIssuerEncryptionKey])) @@ -219,18 +227,57 @@ MPTokenIssuanceSet::preclaim(PreclaimContext const& ctx) return tecNO_PERMISSION; } - // cannot update issuer public key - if (ctx.tx.isFieldPresent(sfIssuerEncryptionKey) && - sleMptIssuance->isFieldPresent(sfIssuerEncryptionKey)) - { - return tecNO_PERMISSION; - } + // Updating an existing encryption key requires the + // ConfidentialMPTKeyRotation amendment. + bool const canRotateKey = ctx.view.rules().enabled(featureConfidentialMPTKeyRotation); - // cannot update auditor public key - if (ctx.tx.isFieldPresent(sfAuditorEncryptionKey) && - sleMptIssuance->isFieldPresent(sfAuditorEncryptionKey)) + bool const txHasIssuerKey = ctx.tx.isFieldPresent(sfIssuerEncryptionKey); + bool const txHasAuditorKey = ctx.tx.isFieldPresent(sfAuditorEncryptionKey); + bool const sleHasIssuerKey = sleMptIssuance->isFieldPresent(sfIssuerEncryptionKey); + bool const sleHasAuditorKey = sleMptIssuance->isFieldPresent(sfAuditorEncryptionKey); + + if (canRotateKey) { - return tecNO_PERMISSION; // LCOV_EXCL_LINE + // Post-ConfidentialMPTKeyRotation amendment, the encryption keys can be updated. + // A first-time auditor key registration requires an issuer key, + // either already on the issuance or set by the same transaction. + bool const registersAuditorKey = txHasAuditorKey && !sleHasAuditorKey; + bool const issuerKeyExists = sleHasIssuerKey || txHasIssuerKey; + if (registersAuditorKey && !issuerKeyExists) + return tecNO_PERMISSION; + + // Rotating a key to its current value is not permitted: a key epoch + // increment must always correspond to an actual key change. + if (txHasIssuerKey && sleHasIssuerKey && + ctx.tx[sfIssuerEncryptionKey] == (*sleMptIssuance)[sfIssuerEncryptionKey]) + return tecDUPLICATE; + + if (txHasAuditorKey && sleHasAuditorKey && + ctx.tx[sfAuditorEncryptionKey] == (*sleMptIssuance)[sfAuditorEncryptionKey]) + return tecDUPLICATE; + + // Key epochs must never wrap. Epoch 0 serves as the sentinel for "never + // rotated." Holders' mirror epochs are checked against it for equality, + // so a wrap would cause stale mirror ciphertexts to appear valid instead + // of failing loudly. + if (txHasIssuerKey && sleHasIssuerKey && + (*sleMptIssuance)[~sfIssuerKeyEpoch].value_or(0) == kMaxKeyEpoch) + return tecNO_PERMISSION; + + if (txHasAuditorKey && sleHasAuditorKey && + (*sleMptIssuance)[~sfAuditorKeyEpoch].value_or(0) == kMaxKeyEpoch) + return tecNO_PERMISSION; + } + else + { + // Pre-ConfidentialMPTKeyRotation amendment, the encryption keys can not be updated. + // cannot update issuer public key + if (txHasIssuerKey && sleHasIssuerKey) + return tecNO_PERMISSION; + + // cannot update auditor public key + if (txHasAuditorKey && sleHasAuditorKey) + return tecNO_PERMISSION; // LCOV_EXCL_LINE } auto const enablesConfidentialBalance = @@ -241,25 +288,30 @@ MPTokenIssuanceSet::preclaim(PreclaimContext const& ctx) // Encryption keys can only be set if confidential amounts are already // enabled on the issuance OR if the transaction is enabling it - if (ctx.tx.isFieldPresent(sfIssuerEncryptionKey) && - !sleMptIssuance->isFlag(lsfMPTCanHoldConfidentialBalance) && !enablesConfidentialBalance) + if (txHasIssuerKey && !sleMptIssuance->isFlag(lsfMPTCanHoldConfidentialBalance) && + !enablesConfidentialBalance) { return tecNO_PERMISSION; } - if (ctx.tx.isFieldPresent(sfAuditorEncryptionKey) && - !sleMptIssuance->isFlag(lsfMPTCanHoldConfidentialBalance) && !enablesConfidentialBalance) + if (txHasAuditorKey && !sleMptIssuance->isFlag(lsfMPTCanHoldConfidentialBalance) && + !enablesConfidentialBalance) { return tecNO_PERMISSION; } - // cannot upload key if there's circulating supply of COA - if ((ctx.tx.isFieldPresent(sfIssuerEncryptionKey) || - ctx.tx.isFieldPresent(sfAuditorEncryptionKey) || enablesConfidentialBalance) && - (*sleMptIssuance)[~sfConfidentialOutstandingAmount].value_or(0) > 0) - { + bool const hasConfidentialOA = + (*sleMptIssuance)[~sfConfidentialOutstandingAmount].value_or(0) > 0; + + // Pre-ConfidentialMPTKeyRotation amendment, keys cannot be uploaded while + // COA > 0. Post-amendment they can be uploaded even if COA > 0. + if (!canRotateKey && (txHasIssuerKey || txHasAuditorKey) && hasConfidentialOA) return tecNO_PERMISSION; // LCOV_EXCL_LINE - } + + // Enabling confidential balances when COA > 0 is not permitted, regardless of + // ConfidentialMPTKeyRotation. + if (enablesConfidentialBalance && hasConfidentialOA) + return tecNO_PERMISSION; return tesSUCCESS; } @@ -377,25 +429,69 @@ MPTokenIssuanceSet::doApply() } } - if (auto const pubKey = ctx_.tx[~sfIssuerEncryptionKey]) - { - // This is enforced in preflight. + // Sets an encryption key on the issuance. Overwriting an existing key + // (a rotation) increments the corresponding key epoch; a first-time + // registration leaves the epoch absent (epoch 0), matching issuances + // whose keys were registered before the ConfidentialMPTKeyRotation + // amendment. + bool const canRotateKey = view().rules().enabled(featureConfidentialMPTKeyRotation); + auto const setEncryptionKey = [&](SF_VL const& keyField, SF_UINT32 const& epochField) -> TER { + auto const pubKey = ctx_.tx[~keyField]; + if (!pubKey) + return tesSUCCESS; + + // This is enforced in preflight, which rejects a transaction carrying + // both sfHolder and an encryption key. XRPL_ASSERT( sle->getType() == ltMPTOKEN_ISSUANCE, "MPTokenIssuanceSet::doApply : modifying MPTokenIssuance"); - sle->setFieldVL(sfIssuerEncryptionKey, *pubKey); - } + // Add sanity check under the amendment ConfidentialMPTKeyRotation. + // Pre-confidentialMPTKeyRotation did not return tecINTERNAL so + // this should be under the amendment guard. + if (canRotateKey && sle->getType() != ltMPTOKEN_ISSUANCE) + return tecINTERNAL; // LCOV_EXCL_LINE - if (auto const pubKey = ctx_.tx[~sfAuditorEncryptionKey]) - { - // This is enforced in preflight. - XRPL_ASSERT( - sle->getType() == ltMPTOKEN_ISSUANCE, - "MPTokenIssuanceSet::doApply : modifying MPTokenIssuance"); + // NOTE: presence must be checked before the key is overwritten below. + bool const isRotation = sle->isFieldPresent(keyField); + sle->setFieldVL(keyField, *pubKey); - sle->setFieldVL(sfAuditorEncryptionKey, *pubKey); - } + if (isRotation) + { + // Preclaim rejects overwriting an existing key unless the amendment is + // enabled. + if (!canRotateKey) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::MPTokenIssuanceSet::doApply : rotation without amendment"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } + + auto const epoch = (*sle)[~epochField].valueOr(0); + + // Preclaim rejects a rotation that would wrap the epoch. So this should never happen. + if (epoch >= kMaxKeyEpoch) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::MPTokenIssuanceSet::doApply : key epoch overflow"); + return tecINTERNAL; + // LCOV_EXCL_STOP + } + + (*sle)[epochField] = epoch + 1; + } + + return tesSUCCESS; + }; + + if (auto const ter = setEncryptionKey(sfIssuerEncryptionKey, sfIssuerKeyEpoch); + !isTesSuccess(ter)) + return ter; // LCOV_EXCL_LINE + + if (auto const ter = setEncryptionKey(sfAuditorEncryptionKey, sfAuditorKeyEpoch); + !isTesSuccess(ter)) + return ter; // LCOV_EXCL_LINE view().update(sle); diff --git a/src/test/app/ConfidentialMPTKeyRotation_test.cpp b/src/test/app/ConfidentialMPTKeyRotation_test.cpp new file mode 100644 index 0000000000..c4e8e607da --- /dev/null +++ b/src/test/app/ConfidentialMPTKeyRotation_test.cpp @@ -0,0 +1,634 @@ +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace xrpl { + +class ConfidentialMPTKeyRotation_test : public ConfidentialTransferTestBase +{ + void + testMPTokenIssuanceSetRotateIssuerKey(FeatureBitset features) + { + testcase("MPTokenIssuanceSet rotate issuer key"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + + // First-time registration. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + }); + + // Verify that no epochs are set when registering for the first time. + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + + // Rotating the issuer key requires the key rotation amendment + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(bob), + .err = rotationEnabled ? TER(tesSUCCESS) : TER(tecNO_PERMISSION), + }); + + // A rotation replaces the issuer key and bumps its epoch. The auditor + // key was never registered, so it and its epoch stay absent. + if (rotationEnabled) + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(bob, std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(1u, std::nullopt)); + } + else + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + } + + if (rotationEnabled) + { + // A second rotation increments the epoch again + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + }); + + BEAST_EXPECT(mptAlice.checkKeyEpochs(2u, std::nullopt)); + } + } + + void + testMPTokenIssuanceSetRotateBothKeys(FeatureBitset features) + { + testcase("MPTokenIssuanceSet rotate both issuer and auditor keys"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + Account const auditor("auditor"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + mptAlice.generateKeyPair(auditor); + + // Register both keys together. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + .auditorPubKey = mptAlice.getPubKey(auditor), + }); + + // Verify that no epochs are set when registering for the first time. + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + + // Rotating both keys requires the amendment + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(bob), + .auditorPubKey = mptAlice.getPubKey(alice), + .err = rotationEnabled ? TER(tesSUCCESS) : TER(tecNO_PERMISSION), + }); + + if (rotationEnabled) + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(bob, alice)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(1u, 1u)); + } + else + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, auditor)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + } + + if (rotationEnabled) + { + // Rotating the issuer key to its current value fails. + // Current issuer key is bob, duplicate. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(bob), + .err = tecDUPLICATE, + }); + + // Rotating the auditor key to its current value fails. + // Current auditor key is alice, duplicate. + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(alice), + .err = tecDUPLICATE, + }); + + // The whole transaction fails when one key is unchanged, even if + // the other key is rotated to a new value. + // Current issuer key is bob, duplicate. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(bob), + .auditorPubKey = mptAlice.getPubKey(auditor), + .err = tecDUPLICATE, + }); + + // Current auditor key is alice, duplicate. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(auditor), + .auditorPubKey = mptAlice.getPubKey(alice), + .err = tecDUPLICATE, + }); + + // Nothing changed: keys and epochs are untouched + BEAST_EXPECT(mptAlice.checkKeyEpochs(1u, 1u)); + + // A second rotation increments both epochs again + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + .auditorPubKey = mptAlice.getPubKey(auditor), + }); + + BEAST_EXPECT(mptAlice.checkKeyEpochs(2u, 2u)); + } + } + + void + testMPTokenIssuanceSetRotateAuditorKeyOnly(FeatureBitset features) + { + testcase("MPTokenIssuanceSet rotate auditor key only"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + Account const auditor("auditor"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + mptAlice.generateKeyPair(auditor); + + // Register both keys together. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + .auditorPubKey = mptAlice.getPubKey(auditor), + }); + + // A transaction carrying only the auditor key fails preflight + // pre-ConfidentialMPTKeyRotation; post-ConfidentialMPTKeyRotation it rotates the auditor + // key + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(bob), + .err = rotationEnabled ? TER(tesSUCCESS) : TER(temMALFORMED), + }); + + // The issuer key keeps unchanged, and rotating only the auditor key + // bumps only its epoch. + if (rotationEnabled) + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, bob)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, 1u)); + } + else + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, auditor)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + } + + if (rotationEnabled) + { + // A second rotation increments the epoch again + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(auditor), + }); + + // The issuer key epoch is still untouched. + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, 2u)); + } + } + + void + testMPTokenIssuanceSetRegisterAuditorKeyLater(FeatureBitset features) + { + testcase("MPTokenIssuanceSet register auditor key after issuer key"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const auditor("auditor"); + MPTTester mptAlice(env, alice); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(auditor); + + // Register the issuer key first. We'll register the auditor key in a separate transaction. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + }); + + // Register the auditor key separately. + // pre-ConfidentialMPTKeyRotation it fails preflight; post-ConfidentialMPTKeyRotation it + // succeeds without touching any epoch because it's a first-time registration. + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(auditor), + .err = rotationEnabled ? TER(tesSUCCESS) : TER(temMALFORMED), + }); + + BEAST_EXPECT(mptAlice.checkEncryptionKeys( + alice, rotationEnabled ? std::optional(auditor) : std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + } + + void + testMPTokenIssuanceSetRegisterAuditorKeyLaterWithCOA(FeatureBitset features) + { + testcase("MPTokenIssuanceSet register auditor key later with circulating supply"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + Account const auditor("auditor"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.authorize({.account = bob}); + mptAlice.pay(alice, bob, 100); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + mptAlice.generateKeyPair(auditor); + + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + }); + + // Convert some of bob's balance so that COA > 0 + mptAlice.convert({ + .account = bob, + .amt = 50, + .holderPubKey = mptAlice.getPubKey(bob), + }); + + auto const sleIssuanceBefore = env.le(keylet::mptokenIssuance(mptAlice.issuanceID())); + if (!BEAST_EXPECT(sleIssuanceBefore)) + return; + auto const coaBefore = (*sleIssuanceBefore)[~sfConfidentialOutstandingAmount].value_or(0); + BEAST_EXPECT(coaBefore > 0); + + // Registering the auditor key for the first time while confidential + // supply is circulating: pre-ConfidentialMPTKeyRotation an auditor-only + // transaction fails preflight; post-ConfidentialMPTKeyRotation it + // succeeds as a first-time late-registration even COA > 0. + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(auditor), + .err = rotationEnabled ? TER(tesSUCCESS) : TER(temMALFORMED), + }); + + auto const sleIssuance = env.le(keylet::mptokenIssuance(mptAlice.issuanceID())); + if (!BEAST_EXPECT(sleIssuance)) + return; + BEAST_EXPECT(mptAlice.checkEncryptionKeys( + alice, rotationEnabled ? std::optional(auditor) : std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + + // The circulating supply itself is not affected. + BEAST_EXPECT((*sleIssuance)[~sfConfidentialOutstandingAmount].value_or(0) == coaBefore); + } + + void + testMPTokenIssuanceSetAuditorKeyWithoutIssuerKey(FeatureBitset features) + { + testcase("MPTokenIssuanceSet auditor key requires issuer key"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const auditor("auditor"); + MPTTester mptAlice(env, alice); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.generateKeyPair(auditor); + // The issuer key was never registered. pre-ConfidentialMPTKeyRotation an auditor-only + // transaction fails preflight; post-ConfidentialMPTKeyRotation it passes preflight + // but preclaim rejects registering an auditor key on an issuance + // without an issuer key. + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(auditor), + .err = rotationEnabled ? TER(tecNO_PERMISSION) : TER(temMALFORMED), + }); + + // The rejected transaction leaves the issuance without either key. + BEAST_EXPECT(mptAlice.checkEncryptionKeys(std::nullopt, std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + } + + void + testMPTokenIssuanceSetRotateWithCOA(FeatureBitset features) + { + testcase("MPTokenIssuanceSet rotate with circulating confidential supply"); + using namespace test::jtx; + + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + Account const carol("carol"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.authorize({.account = bob}); + mptAlice.pay(alice, bob, 100); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + mptAlice.generateKeyPair(carol); + + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + }); + + // Convert some of bob's balance to confidential spending, so that the + // issuance has confidential supply. COA > 0. + mptAlice.convert({ + .account = bob, + .amt = 50, + .holderPubKey = mptAlice.getPubKey(bob), + }); + + auto const sleIssuanceBeforeRotation = + env.le(keylet::mptokenIssuance(mptAlice.issuanceID())); + if (!BEAST_EXPECT(sleIssuanceBeforeRotation)) + return; + auto const coaBeforeRotation = + (*sleIssuanceBeforeRotation)[~sfConfidentialOutstandingAmount].value_or(0); + BEAST_EXPECT(coaBeforeRotation > 0); + + // Rotating key requires the + // amendment. + bool const rotationEnabled = features[featureConfidentialMPTKeyRotation]; + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(carol), + .err = rotationEnabled ? TER(tesSUCCESS) : TER(tecNO_PERMISSION), + }); + + auto const sleIssuance = env.le(keylet::mptokenIssuance(mptAlice.issuanceID())); + if (!BEAST_EXPECT(sleIssuance)) + return; + if (rotationEnabled) + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(carol, std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(1u, std::nullopt)); + } + else + { + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, std::nullopt)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + } + + // The confidential outstanding amount is not affected by the rotation + BEAST_EXPECT( + (*sleIssuance)[~sfConfidentialOutstandingAmount].value_or(0) == coaBeforeRotation); + + // Re-enabling confidential balances while supply is circulating is + // rejected regardless of the ConfidentialMPTKeyRotation amendment. + mptAlice.set({ + .account = alice, + .flags = tfMPTSetCanHoldConfidentialBalance, + .err = tecNO_PERMISSION, + }); + } + + void + testMPTokenIssuanceSetKeyEpochAtMax(FeatureBitset features) + { + using namespace test::jtx; + if (!features[featureConfidentialMPTKeyRotation]) + return; + + testcase("MPTokenIssuanceSet key epoch cannot wrap"); + + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + Account const carol("carol"); + Account const auditor("auditor"); + + // Keep the ledger open so that we can write the key epochs directly into it. + MPTTester mptAlice(env, alice, {.holders = {bob}, .close = false}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanHoldConfidentialBalance, + }); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + mptAlice.generateKeyPair(carol); + mptAlice.generateKeyPair(auditor); + + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + .auditorPubKey = mptAlice.getPubKey(auditor), + }); + + auto const issuanceKeylet = keylet::mptokenIssuance(mptAlice.issuanceID()); + + // Writes the supplied key epochs straight into the open ledger so that + // the maximum epoch is reachable without submitting four billion + // rotations. + auto setEpochs = [&](std::optional const& issuerKeyEpoch, + std::optional const& auditorKeyEpoch) { + env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) { + auto const sle = view.read(issuanceKeylet); + if (!sle) + return false; // LCOV_EXCL_LINE + + auto replacement = std::make_shared(*sle); + if (issuerKeyEpoch) + (*replacement)[sfIssuerKeyEpoch] = *issuerKeyEpoch; + if (auditorKeyEpoch) + (*replacement)[sfAuditorKeyEpoch] = *auditorKeyEpoch; + view.rawReplace(replacement); + return true; + }); + }; + + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, auditor)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, std::nullopt)); + + // Increment the auditor epoch to kMaxKeyEpoch - 1, leaving the issuer epoch absent. + setEpochs(std::nullopt, kMaxKeyEpoch - 1); + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, auditor)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, kMaxKeyEpoch - 1)); + + // Rotating the auditor key to kMaxKeyEpoch succeeds. + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(carol), + }); + + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, carol)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, kMaxKeyEpoch)); + + // A further auditor rotation is rejected because the epoch is exhausted. + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(bob), + .err = tecNO_PERMISSION, + }); + + // Rotating both keys at once is rejected as a whole because the auditor + // epoch is exhausted. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(auditor), + .auditorPubKey = mptAlice.getPubKey(bob), + .err = tecNO_PERMISSION, + }); + + // Both rejections leave every key and epoch as it was. + BEAST_EXPECT(mptAlice.checkEncryptionKeys(alice, carol)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(std::nullopt, kMaxKeyEpoch)); + + // The issuer key is unaffected by the exhausted auditor epoch. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(bob), + }); + + BEAST_EXPECT(mptAlice.checkEncryptionKeys(bob, carol)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(1u, kMaxKeyEpoch)); + + // Increment the issuer epoch to kMaxKeyEpoch - 1. + setEpochs(kMaxKeyEpoch - 1, std::nullopt); + BEAST_EXPECT(mptAlice.checkEncryptionKeys(bob, carol)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(kMaxKeyEpoch - 1, kMaxKeyEpoch)); + + // Rotating the issuer key to kMaxKeyEpoch succeeds. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(auditor), + }); + + BEAST_EXPECT(mptAlice.checkEncryptionKeys(auditor, carol)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(kMaxKeyEpoch, kMaxKeyEpoch)); + + // With both epochs exhausted neither key can be rotated again. + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + .err = tecNO_PERMISSION, + }); + mptAlice.set({ + .account = alice, + .auditorPubKey = mptAlice.getPubKey(bob), + .err = tecNO_PERMISSION, + }); + mptAlice.set({ + .account = alice, + .issuerPubKey = mptAlice.getPubKey(alice), + .auditorPubKey = mptAlice.getPubKey(bob), + .err = tecNO_PERMISSION, + }); + + BEAST_EXPECT(mptAlice.checkEncryptionKeys(auditor, carol)); + BEAST_EXPECT(mptAlice.checkKeyEpochs(kMaxKeyEpoch, kMaxKeyEpoch)); + } + + void + testMPTokenIssuanceSetWithFeats(FeatureBitset features) + { + testMPTokenIssuanceSetRotateIssuerKey(features); + testMPTokenIssuanceSetRotateBothKeys(features); + testMPTokenIssuanceSetRotateAuditorKeyOnly(features); + testMPTokenIssuanceSetRegisterAuditorKeyLater(features); + testMPTokenIssuanceSetRegisterAuditorKeyLaterWithCOA(features); + testMPTokenIssuanceSetAuditorKeyWithoutIssuerKey(features); + testMPTokenIssuanceSetRotateWithCOA(features); + testMPTokenIssuanceSetKeyEpochAtMax(features); + } + +public: + void + run() override + { + using namespace test::jtx; + FeatureBitset const all{testableAmendments()}; + + testMPTokenIssuanceSetWithFeats(all); + testMPTokenIssuanceSetWithFeats(all - featureConfidentialMPTKeyRotation); + } +}; + +BEAST_DEFINE_TESTSUITE(ConfidentialMPTKeyRotation, app, xrpl); + +} // namespace xrpl diff --git a/src/test/app/ConfidentialTransfer_test.cpp b/src/test/app/ConfidentialTransfer_test.cpp index 6450ceeb61..a964193c1a 100644 --- a/src/test/app/ConfidentialTransfer_test.cpp +++ b/src/test/app/ConfidentialTransfer_test.cpp @@ -736,12 +736,8 @@ class ConfidentialTransfer_test : public ConfidentialTransferTestBase .err = temMALFORMED, }); - // Cannot set auditor key without issuer key - mptAlice.set({ - .account = alice, - .auditorPubKey = mptAlice.getPubKey(alice), - .err = temMALFORMED, - }); + // Note: "auditor key without issuer key" (temMALFORMED before + // ConfidentialMPTKeyRotation) is covered in ConfidentialMPTKeyRotation_test // Cannot set Holder and issuer Keys in the same transaction mptAlice.set({ @@ -787,9 +783,9 @@ class ConfidentialTransfer_test : public ConfidentialTransferTestBase }); } - // Cannot update issuer public key once set + // Cannot update issuer public key once set (pre-ConfidentialMPTKeyRotation behavior) { - Env env{*this, features}; + Env env{*this, features - featureConfidentialMPTKeyRotation}; Account const alice("alice"); Account const bob("bob"); MPTTester mptAlice(env, alice, {.holders = {bob}}); @@ -819,8 +815,9 @@ class ConfidentialTransfer_test : public ConfidentialTransferTestBase // Cannot update issuer and auditor public keys once set // Note: trying to set only auditor key fails in preflight (temMALFORMED) // so we must provide both keys, which fails on issuer key check first + // (pre-ConfidentialMPTKeyRotation behavior) { - Env env{*this, features}; + Env env{*this, features - featureConfidentialMPTKeyRotation}; Account const alice("alice"); Account const bob("bob"); Account const auditor("auditor"); @@ -900,8 +897,9 @@ class ConfidentialTransfer_test : public ConfidentialTransferTestBase } // Set issuer key first, then auditor key in a separate tx + // (pre-ConfidentialMPTKeyRotation behavior) { - Env env{*this, features}; + Env env{*this, features - featureConfidentialMPTKeyRotation}; Account const alice("alice"); Account const auditor("auditor"); MPTTester mptAlice(env, alice, {.holders = {}, .auditor = auditor}); diff --git a/src/test/jtx/impl/mpt.cpp b/src/test/jtx/impl/mpt.cpp index 2743084beb..0c1ff14eab 100644 --- a/src/test/jtx/impl/mpt.cpp +++ b/src/test/jtx/impl/mpt.cpp @@ -648,6 +648,38 @@ MPTTester::checkImmutableFlags(std::uint32_t expectedFlags) const }); } +[[nodiscard]] bool +MPTTester::checkKeyEpochs( + std::optional issuerKeyEpoch, + std::optional auditorKeyEpoch) const +{ + return forObject([&](SLEP const& sle) -> bool { + return (*sle)[~sfIssuerKeyEpoch] == issuerKeyEpoch && + (*sle)[~sfAuditorKeyEpoch] == auditorKeyEpoch; + }); +} + +[[nodiscard]] bool +MPTTester::checkEncryptionKeys( + std::optional const& issuerKeyOwner, + std::optional const& auditorKeyOwner) const +{ + auto const matches = + [this](SLEP const& sle, SF_VL const& field, std::optional const& owner) { + if (!owner) + return !sle->isFieldPresent(field); + + auto const expected = getPubKey(*owner); + return expected && sle->isFieldPresent(field) && + strHex((*sle)[field]) == strHex(*expected); + }; + + return forObject([&](SLEP const& sle) -> bool { + return matches(sle, sfIssuerEncryptionKey, issuerKeyOwner) && + matches(sle, sfAuditorEncryptionKey, auditorKeyOwner); + }); +} + void MPTTester::pay( Account const& src, diff --git a/src/test/jtx/mpt.h b/src/test/jtx/mpt.h index 35ab7264bd..26329ad78c 100644 --- a/src/test/jtx/mpt.h +++ b/src/test/jtx/mpt.h @@ -612,6 +612,21 @@ public: [[nodiscard]] bool checkImmutableFlags(std::uint32_t expectedFlags) const; + // Checks both key epochs on the issuance. Pass std::nullopt for an epoch + // that is expected to be absent, which means the key is never rotated. + [[nodiscard]] bool + checkKeyEpochs( + std::optional issuerKeyEpoch, + std::optional auditorKeyEpoch) const; + + // Checks that the issuance carries the encryption keys of the given + // accounts. Pass std::nullopt for a key that is expected to be absent, + // which means the key is never registered. + [[nodiscard]] bool + checkEncryptionKeys( + std::optional const& issuerKeyOwner, + std::optional const& auditorKeyOwner) const; + [[nodiscard]] Account const& issuer() const { diff --git a/src/tests/libxrpl/protocol_autogen/ledger_entries/MPTokenIssuanceTests.cpp b/src/tests/libxrpl/protocol_autogen/ledger_entries/MPTokenIssuanceTests.cpp index 974d0e81d7..e8c1b645d8 100644 --- a/src/tests/libxrpl/protocol_autogen/ledger_entries/MPTokenIssuanceTests.cpp +++ b/src/tests/libxrpl/protocol_autogen/ledger_entries/MPTokenIssuanceTests.cpp @@ -36,6 +36,8 @@ TEST(MPTokenIssuanceTests, BuilderSettersRoundTrip) auto const referenceHoldingValue = canonical_UINT256(); auto const issuerEncryptionKeyValue = canonical_VL(); auto const auditorEncryptionKeyValue = canonical_VL(); + auto const issuerKeyEpochValue = canonical_UINT32(); + auto const auditorKeyEpochValue = canonical_UINT32(); auto const confidentialOutstandingAmountValue = canonical_UINT64(); MPTokenIssuanceBuilder builder{ @@ -57,6 +59,8 @@ TEST(MPTokenIssuanceTests, BuilderSettersRoundTrip) builder.setReferenceHolding(referenceHoldingValue); builder.setIssuerEncryptionKey(issuerEncryptionKeyValue); builder.setAuditorEncryptionKey(auditorEncryptionKeyValue); + builder.setIssuerKeyEpoch(issuerKeyEpochValue); + builder.setAuditorKeyEpoch(auditorKeyEpochValue); builder.setConfidentialOutstandingAmount(confidentialOutstandingAmountValue); builder.setLedgerIndex(index); @@ -184,6 +188,22 @@ TEST(MPTokenIssuanceTests, BuilderSettersRoundTrip) EXPECT_TRUE(entry.hasAuditorEncryptionKey()); } + { + auto const& expected = issuerKeyEpochValue; + auto const actualOpt = entry.getIssuerKeyEpoch(); + ASSERT_TRUE(actualOpt.has_value()); + expectEqualField(expected, *actualOpt, "sfIssuerKeyEpoch"); + EXPECT_TRUE(entry.hasIssuerKeyEpoch()); + } + + { + auto const& expected = auditorKeyEpochValue; + auto const actualOpt = entry.getAuditorKeyEpoch(); + ASSERT_TRUE(actualOpt.has_value()); + expectEqualField(expected, *actualOpt, "sfAuditorKeyEpoch"); + EXPECT_TRUE(entry.hasAuditorKeyEpoch()); + } + { auto const& expected = confidentialOutstandingAmountValue; auto const actualOpt = entry.getConfidentialOutstandingAmount(); @@ -221,6 +241,8 @@ TEST(MPTokenIssuanceTests, BuilderFromSleRoundTrip) auto const referenceHoldingValue = canonical_UINT256(); auto const issuerEncryptionKeyValue = canonical_VL(); auto const auditorEncryptionKeyValue = canonical_VL(); + auto const issuerKeyEpochValue = canonical_UINT32(); + auto const auditorKeyEpochValue = canonical_UINT32(); auto const confidentialOutstandingAmountValue = canonical_UINT64(); auto sle = std::make_shared(MPTokenIssuance::entryType, index); @@ -241,6 +263,8 @@ TEST(MPTokenIssuanceTests, BuilderFromSleRoundTrip) sle->at(sfReferenceHolding) = referenceHoldingValue; sle->at(sfIssuerEncryptionKey) = issuerEncryptionKeyValue; sle->at(sfAuditorEncryptionKey) = auditorEncryptionKeyValue; + sle->at(sfIssuerKeyEpoch) = issuerKeyEpochValue; + sle->at(sfAuditorKeyEpoch) = auditorKeyEpochValue; sle->at(sfConfidentialOutstandingAmount) = confidentialOutstandingAmountValue; MPTokenIssuanceBuilder builderFromSle{sle}; @@ -442,6 +466,32 @@ TEST(MPTokenIssuanceTests, BuilderFromSleRoundTrip) expectEqualField(expected, *fromBuilderOpt, "sfAuditorEncryptionKey"); } + { + auto const& expected = issuerKeyEpochValue; + + auto const fromSleOpt = entryFromSle.getIssuerKeyEpoch(); + auto const fromBuilderOpt = entryFromBuilder.getIssuerKeyEpoch(); + + ASSERT_TRUE(fromSleOpt.has_value()); + ASSERT_TRUE(fromBuilderOpt.has_value()); + + expectEqualField(expected, *fromSleOpt, "sfIssuerKeyEpoch"); + expectEqualField(expected, *fromBuilderOpt, "sfIssuerKeyEpoch"); + } + + { + auto const& expected = auditorKeyEpochValue; + + auto const fromSleOpt = entryFromSle.getAuditorKeyEpoch(); + auto const fromBuilderOpt = entryFromBuilder.getAuditorKeyEpoch(); + + ASSERT_TRUE(fromSleOpt.has_value()); + ASSERT_TRUE(fromBuilderOpt.has_value()); + + expectEqualField(expected, *fromSleOpt, "sfAuditorKeyEpoch"); + expectEqualField(expected, *fromBuilderOpt, "sfAuditorKeyEpoch"); + } + { auto const& expected = confidentialOutstandingAmountValue; @@ -539,6 +589,10 @@ TEST(MPTokenIssuanceTests, OptionalFieldsReturnNullopt) EXPECT_FALSE(entry.getIssuerEncryptionKey().has_value()); EXPECT_FALSE(entry.hasAuditorEncryptionKey()); EXPECT_FALSE(entry.getAuditorEncryptionKey().has_value()); + EXPECT_FALSE(entry.hasIssuerKeyEpoch()); + EXPECT_FALSE(entry.getIssuerKeyEpoch().has_value()); + EXPECT_FALSE(entry.hasAuditorKeyEpoch()); + EXPECT_FALSE(entry.getAuditorKeyEpoch().has_value()); EXPECT_FALSE(entry.hasConfidentialOutstandingAmount()); EXPECT_FALSE(entry.getConfidentialOutstandingAmount().has_value()); } From e3c8996e44921fe3b4e02c65cb41948848bcc7c5 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Sat, 5 Sep 2026 00:06:14 +0000 Subject: [PATCH 09/16] feat: Add fixCleanup3_5_0 amendment placeholder (#8174) --- include/xrpl/protocol/detail/features.macro | 1 + 1 file changed, 1 insertion(+) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index fe49a0230f..84452eb18e 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,6 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FIX (Cleanup3_5_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(ConfidentialMPTKeyRotation, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_4_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Sponsor, Supported::Yes, VoteBehavior::DefaultNo) From 3e4bdf2782d7e076bb71c88091b81159c1dbdaec Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 16:07:56 +0000 Subject: [PATCH 10/16] ci: [DEPENDABOT] bump actions/deploy-pages from 5.0.0 to 5.0.1 in the github-actions group across 1 directory (#8180) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/publish-docs.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index 6bc803f1e4..d4fb6faeca 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -91,4 +91,4 @@ jobs: steps: - name: Deploy to GitHub Pages id: deploy - uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128 # v5.0.0 + uses: actions/deploy-pages@368f82528645a54fb793d4d04e342629a3f51346 # v5.0.1 From 30d165da433e1d73a3e1b3ee9d412dbce03351db Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:43:41 +0100 Subject: [PATCH 11/16] fix(telemetry): bound integration-test assertions to the run under test check_statsd_metric queried rippled_rpc_requests, which no pipeline produces: the collector's statsd receiver runs with is_monotonic_counter, so the Prometheus exporter appends _total. A wrong name returns zero series rather than an error, so the assertion could not be told apart from a broken pipeline. All eight assertions were re-derived from how each metric is created in code; this was the only counter. Tempo searches carried no start/end, and tempo-data is a named volume that `docker compose down` preserves under a one-hour block retention, so the 17 span assertions could pass on an earlier local run's traces. Bound every search to this run, and tear the stack down with -v before starting so no earlier data is present to match. The service-name check now matches a whole line, because the tag-values endpoint ignores start/end. Add a gtest for the StatsD gauge that publishes its initial zero and for the counter that must publish nothing. Assert two metrics the harness never checked: a traffic-category gauge no message reaches, and io_context latency. --- docker/telemetry/integration-test.sh | 53 +++++- .../libxrpl/beast/insight/StatsDCollector.cpp | 153 ++++++++++++++++++ 2 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 src/tests/libxrpl/beast/insight/StatsDCollector.cpp diff --git a/docker/telemetry/integration-test.sh b/docker/telemetry/integration-test.sh index c0327a7508..ddc9e79522 100755 --- a/docker/telemetry/integration-test.sh +++ b/docker/telemetry/integration-test.sh @@ -42,6 +42,11 @@ PROM="http://localhost:9090" PASS=0 FAIL=0 +# Unix seconds just before this run's nodes start. Every Tempo search is +# bounded to this run, so a previous run's traces cannot satisfy an assertion. +# Set in Step 5; check_span refuses to run while it is empty. +RUN_START="" + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -62,11 +67,19 @@ die() { check_span() { local op="$1" local count + [ -n "$RUN_START" ] || die "check_span called before RUN_START was set" # -G is required: it moves the urlencoded params into the query string. # Without it curl POSTs them as a request body, and Tempo answers 200 # while ignoring the query — so every span name would look present. + # + # start/end bound the search to this run. Tempo keeps blocks for + # block_retention (tempo.yaml, 1h) on a named volume, so without a bound + # an older run's spans answer for this one. The end margin covers spans + # exported while this query is in flight. count=$(curl -sfG "$TEMPO/api/search" \ --data-urlencode "q={resource.service.name=\"xrpld\" && name=\"$op\"}" \ + --data-urlencode "start=$RUN_START" \ + --data-urlencode "end=$(($(date +%s) + 60))" \ --data-urlencode "limit=5" | jq '.traces | length' 2>/dev/null || echo 0) if [ "$count" -gt 0 ]; then @@ -88,8 +101,9 @@ cleanup() { done # Also kill any straggling xrpld processes from our workdir pkill -f "$WORKDIR" 2>/dev/null || true - # Stop docker stack - docker compose -f "$COMPOSE_FILE" down 2>/dev/null || true + # Stop docker stack. -v also drops the tempo-data volume: plain `down` + # keeps it, and retained traces would then answer a later run's searches. + docker compose -f "$COMPOSE_FILE" down -v 2>/dev/null || true # Remove workdir rm -rf "$WORKDIR" log "Cleanup complete." @@ -131,6 +145,10 @@ pkill -f "$WORKDIR" 2>/dev/null || true pkill -f "xrpld-telemetry.cfg" 2>/dev/null || true sleep 2 rm -rf "$WORKDIR" +# A run that reached the summary left the stack up, so nothing has torn it +# down. Do it here, with -v: Tempo's traces and Prometheus' samples must not +# survive into this run, or an assertion can pass on the previous run's data. +docker compose -f "$COMPOSE_FILE" down -v 2>/dev/null || true mkdir -p "$WORKDIR" # --------------------------------------------------------------------------- @@ -361,6 +379,10 @@ done # --------------------------------------------------------------------------- log "Starting $NUM_NODES xrpld nodes..." +# Lower bound for every Tempo search below. Only these nodes have a +# [telemetry] section, so nothing before this instant belongs to this run. +RUN_START=$(date +%s) + for i in $(seq 1 "$NUM_NODES"); do NODE_DIR="$WORKDIR/node$i" "$XRPLD" --conf "$NODE_DIR/xrpld.cfg" --start >"$NODE_DIR/stdout.log" 2>&1 & @@ -497,7 +519,10 @@ log "Verifying spans in Tempo..." # Check service registration services=$(curl -sf "$TEMPO/api/v2/search/tag/resource.service.name/values" | jq -r '.tagValues[].value' 2>/dev/null || echo "") -if echo "$services" | grep -q "xrpld"; then +# Whole-line match: a substring match would also accept a value that merely +# contains "xrpld". This endpoint ignores start/end (measured), so its only +# protection against a previous run is the teardown in Step 1. +if echo "$services" | grep -Fxq "xrpld"; then ok "Service 'xrpld' registered in Tempo" else fail "Service 'xrpld' NOT found in Tempo (found: $services)" @@ -598,12 +623,28 @@ check_statsd_metric "rippled_State_Accounting_Full_duration" check_statsd_metric "rippled_Peer_Finder_Active_Inbound_Peers" check_statsd_metric "rippled_Peer_Finder_Active_Outbound_Peers" -# RPC counters (only if RPC was exercised — should be true from Steps 5-8) -check_statsd_metric "rippled_rpc_requests" +# RPC counters (only if RPC was exercised — should be true from Steps 5-8). +# This one is a beast::insight Counter, and the statsd receiver runs with +# is_monotonic_counter: true, so the Prometheus exporter appends _total. The +# gauges above keep their bare name. +check_statsd_metric "rippled_rpc_requests_total" -# Overlay traffic +# Overlay traffic. "total" is the TrafficCount category name, not a Prometheus +# suffix — the metric is a gauge, so nothing is appended. check_statsd_metric "rippled_total_Bytes_In" +# A gauge for a traffic category no message reaches on a private 6-node +# network: ledger replay is off, so nothing is ever counted here. A StatsD +# gauge is only re-sent when its value changes, so this series exists solely +# because a gauge starts dirty and flushes its initial zero. +check_statsd_metric "rippled_replay_delta_request_Messages_In" + +# io_context latency is an Event, so it reaches Prometheus only when notify() +# is called: on the first sample, and after that only at >= 10 ms. This covers +# the metric arriving at all, not the first-sample path on its own — a busy +# startup can also produce a >= 10 ms sample. +check_statsd_metric "rippled_ios_latency_count" + # --------------------------------------------------------------------------- # Step 11: Summary # --------------------------------------------------------------------------- diff --git a/src/tests/libxrpl/beast/insight/StatsDCollector.cpp b/src/tests/libxrpl/beast/insight/StatsDCollector.cpp new file mode 100644 index 0000000000..7024318ec8 --- /dev/null +++ b/src/tests/libxrpl/beast/insight/StatsDCollector.cpp @@ -0,0 +1,153 @@ +#include + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +namespace beast::insight { + +/** + * Reads the datagrams a StatsDCollector sends, over loopback. + * + * StatsDCollector ──UDP──> LoopbackStatsDServer + * │ + * └── owns ──> boost::asio::io_context + * + * Binds an ephemeral port, so a caller must read port() and point the + * collector at it. The collector flushes on a one-second timer, so receive() + * takes a timeout rather than blocking forever. + * + * @code + * // Primary use: read the one datagram a metric produces. + * LoopbackStatsDServer server; + * auto collector = StatsDCollector::make( + * ip::Endpoint::fromString("127.0.0.1:" + std::to_string(server.port())), + * "test", + * Journal(Journal::getNullSink())); + * auto const gauge = collector->makeGauge("g"); + * EXPECT_EQ(server.receive(std::chrono::seconds(10)), "test.g:0|g\n"); + * + * // Edge case: nothing was sent, so the wait runs out and returns empty. + * EXPECT_EQ(server.receive(std::chrono::seconds(3)), std::string()); + * @endcode + * + * @note Not thread-safe, and receive() must not be called concurrently with + * itself. + * @note Returns one datagram per call. A test that needs several must call + * receive() again; there is no accumulation. + */ +class LoopbackStatsDServer +{ +public: + LoopbackStatsDServer() + : socket_( + ioContext_, + boost::asio::ip::udp::endpoint(boost::asio::ip::make_address_v4("127.0.0.1"), 0)) + { + } + + /** + * The loopback port to point the collector at. + * + * @return the ephemeral port this server is bound to. + */ + [[nodiscard]] unsigned short + port() const + { + return socket_.local_endpoint().port(); + } + + /** + * Waits for one datagram. + * + * @param timeout How long to wait before giving up. + * @return the datagram's bytes, or an empty string if none arrived in + * time. + */ + std::string + receive(std::chrono::milliseconds timeout) + { + std::string received; + socket_.async_receive( + boost::asio::buffer(buffer_), + [&received, this](boost::system::error_code const& ec, std::size_t bytes) { + if (!ec) + received.assign(buffer_.data(), bytes); + }); + ioContext_.restart(); + ioContext_.run_for(timeout); + socket_.cancel(); + return received; + } + +private: + /** + * Drives the receive. Restarted per receive() call. + */ + boost::asio::io_context ioContext_; + + /** + * Bound to 127.0.0.1 on an ephemeral port for the object's lifetime. + */ + boost::asio::ip::udp::socket socket_; + + /** + * Landing space for one datagram. Sized well above the collector's + * 1472-byte packet limit. + */ + std::array buffer_{}; +}; + +/** + * A gauge nobody touches still publishes its zero. + * + * A gauge is only marked dirty when its value changes, so a gauge left at zero + * would otherwise never be sent and would never exist downstream. Absent and + * zero must not look the same to an operator. + */ +TEST(StatsDCollector, UntouchedGaugePublishesInitialZero) +{ + LoopbackStatsDServer server; + auto const address = ip::Endpoint::fromString("127.0.0.1:" + std::to_string(server.port())); + + auto collector = StatsDCollector::make(address, "test", Journal(Journal::getNullSink())); + // Created and then left alone: no set(), no increment(). + auto const gauge = collector->makeGauge("untouched"); + + EXPECT_EQ(server.receive(std::chrono::seconds(10)), std::string("test.untouched:0|g\n")); +} + +/** + * A counter nobody increments publishes nothing. + * + * This is the other half of the rule above, and it is why the fix is a gauge + * starting dirty rather than a flush of everything on the first tick. A counter + * reports events, so an unsent counter and a zero counter mean the same thing. + */ +TEST(StatsDCollector, UntouchedCounterPublishesNothing) +{ + LoopbackStatsDServer server; + auto const address = ip::Endpoint::fromString("127.0.0.1:" + std::to_string(server.port())); + + auto collector = StatsDCollector::make(address, "test", Journal(Journal::getNullSink())); + auto const counter = collector->makeCounter("untouched"); + + // Three seconds spans several one-second flush ticks. + EXPECT_EQ(server.receive(std::chrono::seconds(3)), std::string()); +} + +} // namespace beast::insight From 9c9cb9d091558d1efd7140b175e9ace85db257a3 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:54:30 +0100 Subject: [PATCH 12/16] fix(telemetry): check TLS paths and fix the runbook build steps requireReadableFile proved a path readable with getFileContents, which loads the whole file into a std::string and then drops it. One of the three paths it checks is tls_client_key, so a private key was loaded to answer a question that does not need its contents. It now stats the path, rejects anything that is not a regular file, and opens it without reading. The message shape is unchanged: "[telemetry] cannot be read: - ". A path naming a directory used to escape as an ios failure from the stream buffer, naming neither the config key nor the path. It is now rejected as "not a regular file" with both named. The new test covers that case; it fails against the old implementation and against a copy with the file-type branch removed. The runbook's quick start and disable sections both told the reader to run "cmake --preset default". No presets file is tracked, and the only preset Conan generates is conan-release, so each of those steps failed on its first command. Replaced with the flow BUILD.md documents, and noted that telemetry is the current default while still passing the flags. --- docs/telemetry-runbook.md | 19 ++++++--- src/libxrpl/telemetry/TelemetryConfig.cpp | 39 ++++++++++++++----- .../libxrpl/telemetry/TelemetryConfig.cpp | 31 ++++++++++++++- 3 files changed, 74 insertions(+), 15 deletions(-) diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index ca2bf2fac6..07b1245f30 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -34,12 +34,18 @@ traces_endpoint=http://localhost:4318/v1/traces ### 3. Build with telemetry support +Follow [BUILD.md](../BUILD.md), adding `-o telemetry=True` so Conan pulls `opentelemetry-cpp`. From a build directory (`.build/`): + ```bash -conan install . --build=missing -o telemetry=True -cmake --preset default -Dtelemetry=ON -cmake --build --preset default +conan install .. --output-folder . --build missing -o telemetry=True --settings build_type=Release +cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Release -Dxrpld=ON -Dtelemetry=ON .. +cmake --build . --target xrpld ``` +Conan also writes a `conan-release` CMake preset, so `cmake --preset conan-release -Dtelemetry=ON` works instead of the explicit toolchain line. There is no preset named `default`. + +Both telemetry flags are the current default, so omitting them still gives you an instrumented build. Pass them anyway, so the build stays instrumented wherever the default moves. + ## Configuration Reference | Option | Default | Description | @@ -667,10 +673,13 @@ Three dashboards are pre-provisioned in `docker/telemetry/grafana/dashboards/`: ## Disabling Telemetry -Set `enabled=0` in config (runtime disable) or build without the flag: +Set `enabled=0` in config (runtime disable), or compile telemetry out: ```bash -cmake --preset default -Dtelemetry=OFF +conan install .. --output-folder . --build missing -o telemetry=False --settings build_type=Release +cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Release -Dtelemetry=OFF .. ``` +Both flags are needed, and both must be stated. The default is `ON`, so omitting a flag leaves telemetry compiled in. + When telemetry is compiled out, all trace macros expand to no-ops with zero overhead. diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index a5994a173c..005be4f18a 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -8,13 +8,16 @@ * See cfg/xrpld-example.cfg for the full list of available options. */ -#include #include #include #include +#include #include #include +#include +#include +#include #include #include #include @@ -157,12 +160,15 @@ networkTypeFromId(std::uint32_t networkId) } /** - * Throw unless the given path names a file this process can read. + * Throw unless the given path names a regular file this process can read. * - * An empty path means the option is unset, which every caller allows. Reading - * the file proves it is both present and readable; testing existence alone - * would miss a permissions problem. The contents are discarded — nothing here - * checks that they parse as PEM. + * An empty path means the option is unset, which every caller allows. Opening + * the file proves it is present and that the read permission check passes, + * without loading any of its contents — one of these paths names a private key. + * Nothing here checks that the contents parse as PEM. + * + * A path that is not a regular file is rejected before the open, because + * opening a FIFO waits for a writer. * * @param path Path taken from the config, possibly empty. * @param configKey Config key the path came from, named in the message. Not @@ -175,13 +181,28 @@ requireReadableFile(std::string const& path, char const* configKey) if (path.empty()) return; + // Each branch sets the reason and stops. The two that come from the + // operating system reuse its message; the middle one has no errno to read. std::error_code ec; - getFileContents(ec, path); + std::string reason; + auto const fileStatus = std::filesystem::status(path, ec); if (ec) + { + reason = ec.message(); + } + else if (!std::filesystem::is_regular_file(fileStatus)) + { + reason = "not a regular file"; + } + else if (std::ifstream stream{path, std::ios::in}; !stream) + { + reason = std::error_code{errno, std::generic_category()}.message(); + } + + if (!reason.empty()) { Throw( - std::string{"[telemetry] "} + configKey + " cannot be read: " + path + " - " + - ec.message()); + std::string{"[telemetry] "} + configKey + " cannot be read: " + path + " - " + reason); } } diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index 1f258bfcdc..039fae264c 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -45,7 +46,9 @@ namespace { * pairingError, useTlsError and readError are message fragments. All three * guards throw std::runtime_error, so the exception type alone cannot tell * them apart. Each fragment occurs in exactly one of the three messages, so - * matching it proves which guard fired. + * matching it proves which guard fired. notRegularError names the one reason + * the readability guard supplies itself rather than taking from the operating + * system, so matching it proves the file-type branch ran and not the open. */ namespace mtls { constexpr char const* keyClientCert = "tls_client_cert"; @@ -55,6 +58,7 @@ constexpr char const* clientKey = "/etc/ssl/client.key"; constexpr char const* pairingError = "must be set together"; constexpr char const* useTlsError = "require use_tls=1"; constexpr char const* readError = "cannot be read"; +constexpr char const* notRegularError = "not a regular file"; /** * Endpoint values and the message fragment of the scheme guard. @@ -463,6 +467,31 @@ TEST(TelemetryConfig, tls_missing_ca_cert_file_throws) AllOf(HasSubstr(mtls::readError), HasSubstr("tls_ca_cert"), HasSubstr(absentCa)))); } +TEST(TelemetryConfig, tls_client_key_that_is_a_directory_throws) +{ + // A path that exists but is a directory. The check opens the file instead + // of reading it, and opening a directory for input succeeds on Linux, so + // the file-type branch is the only thing that can reject this. The message + // must still name the key and the path, which is what tells the operator + // which setting is wrong. + TempDir const dir; + auto const keyDir = dir.file("keydir"); + ASSERT_TRUE(std::filesystem::create_directory(keyDir)); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, keyDir); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::readError), + HasSubstr(mtls::notRegularError), + HasSubstr(mtls::keyClientKey), + HasSubstr(keyDir)))); +} + TEST(TelemetryConfig, tls_readable_files_are_accepted) { // Full mTLS with all three files present and readable: parsing must From 0ed0c01021cc804d1abf7ec4350ff0f103a4eed5 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:12:40 +0100 Subject: [PATCH 13/16] fix(docs): spell it "XRPL epoch" in the ledger attribute table The rename script rewrites "Ripple epoch" to "XRPL epoch", so the old spelling in a tracked .md makes the check-rename job fail on a dirty tree. The attribute key close_time_ripple_epoch_s is left alone: the script's pattern needs a space, and that key is a cross-layer contract. --- OpenTelemetryPlan/02-design-decisions.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index dae5e8fe82..09484baaae 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -288,15 +288,15 @@ keys (the dotted form is reserved for resource scope per §2.3.3). #### Ledger & Job Attributes -| Key | Type | Description | -| --------------------------- | ------- | --------------------------------- | -| `ledger_hash` | string | Ledger hash | -| `ledger_seq` | int64 | Closed/validated ledger sequence | -| `close_time_ripple_epoch_s` | int64 | Close time (Ripple epoch seconds) | -| `ledger_tx_count` | int64 | Transaction count | -| `job_type` | string | Job type name | -| `job_queue_ms` | float64 | Time spent in queue | -| `job_worker` | int64 | Worker thread ID | +| Key | Type | Description | +| --------------------------- | ------- | -------------------------------- | +| `ledger_hash` | string | Ledger hash | +| `ledger_seq` | int64 | Closed/validated ledger sequence | +| `close_time_ripple_epoch_s` | int64 | Close time (XRPL epoch seconds) | +| `ledger_tx_count` | int64 | Transaction count | +| `job_type` | string | Job type name | +| `job_queue_ms` | float64 | Time spent in queue | +| `job_worker` | int64 | Worker thread ID | #### PathFinding Attributes From 25362d3b6aabe71fe26e3d7ac6fef3b7907cc7d0 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:12:53 +0100 Subject: [PATCH 14/16] fix(docs): spell it "XRPL epoch" in the close-time attribute notes The rename script rewrites "Ripple epoch" to "XRPL epoch", so the old spelling in a tracked .md makes the check-rename job fail on a dirty tree. The attribute keys are left alone: the script's pattern needs a space, and those keys are a cross-layer contract. --- OpenTelemetryPlan/Phase4_taskList.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/OpenTelemetryPlan/Phase4_taskList.md b/OpenTelemetryPlan/Phase4_taskList.md index a20385e664..4b64255e9c 100644 --- a/OpenTelemetryPlan/Phase4_taskList.md +++ b/OpenTelemetryPlan/Phase4_taskList.md @@ -333,13 +333,13 @@ Phase 7's `ValidationTracker` builds metric-level aggregation (1h/24h agreement The `consensus.accept.apply` span captures ledger close time agreement details driven by `avCT_CONSENSUS_PCT` (75% validator agreement threshold): -- **`close_time_ripple_epoch_s`** — Agreed-upon ledger close time (Ripple epoch seconds). When validators disagree (`consensusCloseTime == epoch`), this is synthetically set to `prevCloseTime + 1s`. +- **`close_time_ripple_epoch_s`** — Agreed-upon ledger close time (XRPL epoch seconds). When validators disagree (`consensusCloseTime == epoch`), this is synthetically set to `prevCloseTime + 1s`. - **`close_time_correct`** — `true` if validators reached agreement, `false` if they "agreed to disagree" (close time forced to prev+1s). - **`close_resolution_ms`** — Rounding granularity for close time (starts at 30s, decreases as ledger interval stabilizes). - **`consensus_state`** — `"finished"` (normal) or `"moved_on"` (consensus failed, adopted best available). - **`proposing`** — Whether this node was proposing. - **`round_time_ms`** — Total consensus round duration. -- **`parent_close_time_ripple_epoch_s`** — Previous ledger's close time (Ripple epoch seconds). Enables computing close-time deltas across consecutive rounds without correlating separate spans. +- **`parent_close_time_ripple_epoch_s`** — Previous ledger's close time (XRPL epoch seconds). Enables computing close-time deltas across consecutive rounds without correlating separate spans. - **`close_time_self_ripple_epoch_s`** — This node's own proposed close time before consensus voting. - **`close_time_vote_bins`** — Number of distinct close-time vote bins from peer proposals. Higher values indicate less agreement among validators. - **`resolution_direction`** — Whether close-time resolution `"increased"` (coarser), `"decreased"` (finer), or stayed `"unchanged"` relative to the previous ledger. From 23c1a6f5dd3a3bf9e80cf7acf74a8e8546fd0176 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:12:55 +0100 Subject: [PATCH 15/16] fix(docs): spell it "XRPL epoch" in the data-collection reference The rename script rewrites "Ripple epoch" to "XRPL epoch", so the old spelling in a tracked .md makes the check-rename job fail on a dirty tree. The attribute keys are left alone: the script's pattern needs a space, and those keys are a cross-layer contract. --- OpenTelemetryPlan/09-data-collection-reference.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index 9507f1c7d8..9050e65aab 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -356,11 +356,11 @@ Join a transaction's work to its ledger with `{span.current_ledger_seq=}`. | `round_time_ms` | int64 | `consensus.accept`, `consensus.accept.apply` | Total consensus round duration in milliseconds | | `proposing` | boolean | `consensus.validation.send` | Whether this node was a proposer | | `consensus_state` | string | `consensus.accept.apply` | Consensus outcome: `"finished"` or `"moved_on"` | -| `close_time_ripple_epoch_s` | int64 | `consensus.accept.apply` | Agreed-upon ledger close time (Ripple epoch seconds) | +| `close_time_ripple_epoch_s` | int64 | `consensus.accept.apply` | Agreed-upon ledger close time (XRPL epoch seconds) | | `close_time_correct` | boolean | `consensus.accept.apply` | Whether validators reached agreement on close time | | `close_resolution_ms` | int64 | `consensus.accept.apply` | Close time rounding granularity in milliseconds | -| `parent_close_time_ripple_epoch_s` | int64 | `consensus.accept.apply` | Parent ledger's close time (Ripple epoch seconds) | -| `close_time_self_ripple_epoch_s` | int64 | `consensus.accept.apply` | This node's proposed close time (Ripple epoch seconds) | +| `parent_close_time_ripple_epoch_s` | int64 | `consensus.accept.apply` | Parent ledger's close time (XRPL epoch seconds) | +| `close_time_self_ripple_epoch_s` | int64 | `consensus.accept.apply` | This node's proposed close time (XRPL epoch seconds) | | `close_time_vote_bins` | string | `consensus.accept.apply` | Histogram of close time votes from validators | | `resolution_direction` | string | `consensus.accept.apply` | Resolution change: `"increased"`, `"decreased"`, or `"unchanged"` | | `converge_percent` | int64 | `consensus.establish` | Convergence percentage threshold | @@ -401,7 +401,7 @@ Join a transaction's work to its ledger with `{span.current_ledger_seq=}`. | Attribute | Type | Set On | Description | | --------------------------- | ------- | ------------------------------------------------------------- | ------------------------------------------------ | | `ledger_seq` | int64 | `ledger.build`, `ledger.validate`, `ledger.store`, `tx.apply` | Ledger sequence number | -| `close_time_ripple_epoch_s` | int64 | `ledger.build` | Ledger close time (Ripple epoch seconds) | +| `close_time_ripple_epoch_s` | int64 | `ledger.build` | Ledger close time (XRPL epoch seconds) | | `close_time_correct` | boolean | `ledger.build` | Whether close time was agreed upon by validators | | `close_resolution_ms` | int64 | `ledger.build` | Close time rounding granularity in milliseconds | | `tx_count` | int64 | `ledger.build`, `tx.apply` | Transactions in the ledger | From bfa9f1b1e09bb68bdac706b7f9e8009043075fdd Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:13:06 +0100 Subject: [PATCH 16/16] fix(test): rename the tlsPath namespace to tls_path readability-identifier-naming wants lower_case for a namespace, so clang-tidy failed on this file under warnings-as-errors. All seven use sites move with the declaration. --- .../libxrpl/telemetry/TraceExporterOptions.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp b/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp index 80f6f22205..8e7d1cb6ba 100644 --- a/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp +++ b/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp @@ -23,11 +23,11 @@ namespace { * certificate written into the key field, or a CA bundle written into either, * shows up as an inequality naming both paths rather than as a near-miss. */ -namespace tlsPath { +namespace tls_path { constexpr char const* ca = "/etc/xrpl/tls/collector-ca-bundle.pem"; constexpr char const* clientCert = "/etc/xrpl/tls/node-client-certificate.pem"; constexpr char const* clientKey = "/etc/xrpl/tls/node-client-private-key.pem"; -} // namespace tlsPath +} // namespace tls_path constexpr char const* kHttpsEndpoint = "https://collector.example:4318/v1/traces"; @@ -48,9 +48,9 @@ makeMtlsSetup(bool useTls) setup.enabled = true; setup.tracesEndpoint = kHttpsEndpoint; setup.useTls = useTls; - setup.tlsCertPath = tlsPath::ca; - setup.tlsClientCertPath = tlsPath::clientCert; - setup.tlsClientKeyPath = tlsPath::clientKey; + setup.tlsCertPath = tls_path::ca; + setup.tlsClientCertPath = tls_path::clientCert; + setup.tlsClientKeyPath = tls_path::clientKey; return setup; } @@ -84,9 +84,9 @@ TEST(TraceExporterOptions, mtls_paths_reach_the_matching_exporter_fields) auto const opts = telemetry::makeTraceExporterOptions(makeMtlsSetup(true)); EXPECT_EQ(opts.url, kHttpsEndpoint); - EXPECT_EQ(opts.ssl_ca_cert_path, tlsPath::ca); - EXPECT_EQ(opts.ssl_client_cert_path, tlsPath::clientCert); - EXPECT_EQ(opts.ssl_client_key_path, tlsPath::clientKey); + EXPECT_EQ(opts.ssl_ca_cert_path, tls_path::ca); + EXPECT_EQ(opts.ssl_client_cert_path, tls_path::clientCert); + EXPECT_EQ(opts.ssl_client_key_path, tls_path::clientKey); } TEST(TraceExporterOptions, one_way_tls_leaves_the_client_fields_empty) @@ -101,7 +101,7 @@ TEST(TraceExporterOptions, one_way_tls_leaves_the_client_fields_empty) auto const opts = telemetry::makeTraceExporterOptions(setup); EXPECT_EQ(opts.url, kHttpsEndpoint); - EXPECT_EQ(opts.ssl_ca_cert_path, tlsPath::ca); + EXPECT_EQ(opts.ssl_ca_cert_path, tls_path::ca); EXPECT_EQ(opts.ssl_client_cert_path, ""); EXPECT_EQ(opts.ssl_client_key_path, ""); }