From c29c4f0a629b79b58acc089a81576eb067cbb026 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:13:04 +0100 Subject: [PATCH 1/7] ci: [DEPENDABOT] bump docker/login-action from 4.5.1 to 4.6.0 in /.github/actions/build-docker-image (#3163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [docker/login-action](https://github.com/docker/login-action) from 4.5.1 to 4.6.0.
Release notes

Sourced from docker/login-action's releases.

v4.6.0

Full Changelog: https://github.com/docker/login-action/compare/v4.5.2...v4.6.0

v4.5.2

Full Changelog: https://github.com/docker/login-action/compare/v4.5.1...v4.5.2

Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=docker/login-action&package-manager=github_actions&previous-version=4.5.1&new-version=4.6.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/actions/build-docker-image/action.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/actions/build-docker-image/action.yml b/.github/actions/build-docker-image/action.yml index 4775486ef..69e4a6434 100644 --- a/.github/actions/build-docker-image/action.yml +++ b/.github/actions/build-docker-image/action.yml @@ -34,14 +34,14 @@ runs: steps: - name: Login to DockerHub if: ${{ inputs.push_image == 'true' && inputs.dockerhub_repo != '' }} - uses: docker/login-action@abd2ef45e78c5afb21d64d4ca52ee8550d9572c7 # v4.5.1 + uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: username: ${{ env.DOCKERHUB_USER }} password: ${{ env.DOCKERHUB_PW }} - name: Login to GitHub Container Registry if: ${{ inputs.push_image == 'true' }} - uses: docker/login-action@abd2ef45e78c5afb21d64d4ca52ee8550d9572c7 # v4.5.1 + uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: registry: ghcr.io username: ${{ github.repository_owner }} From d2d0f39614cf61e2a0f4a94c7433eb7f8787a016 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 4 Aug 2026 14:48:51 +0100 Subject: [PATCH 2/7] chore: Update pre-commit hooks (#3164) --- .hadolint.yml | 1 + .pre-commit-config.yaml | 6 +++--- docker/pre-commit/Dockerfile | 38 ------------------------------------ 3 files changed, 4 insertions(+), 41 deletions(-) delete mode 100644 docker/pre-commit/Dockerfile diff --git a/.hadolint.yml b/.hadolint.yml index 04d19ca1f..a5c519583 100644 --- a/.hadolint.yml +++ b/.hadolint.yml @@ -4,3 +4,4 @@ ignored: - DL3007 - DL3008 - DL3013 + - DL3066 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 118e35ee6..f32b28223 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -40,13 +40,13 @@ repos: exclude: LICENSE.md - repo: https://github.com/hadolint/hadolint - rev: 57e1618d78fd469a92c1e584e8c9313024656623 # frozen: v2.14.0 + rev: 2eece55955ced00200be9729e9728cb7dacca505 # frozen: v2.15.1 hooks: - id: hadolint-docker # hadolint-docker is a special hook that runs hadolint in a Docker container # Docker is not installed in the environment where pre-commit is run stages: [manual] - entry: hadolint/hadolint:v2.14.0 hadolint + entry: hadolint/hadolint:v2.15.1 hadolint - repo: https://github.com/codespell-project/codespell rev: 57b21406f092110c18776e39b0bda50d37c945c8 # frozen: v2.4.3 @@ -107,7 +107,7 @@ repos: types: [c++] - repo: https://github.com/BlankSpruce/gersemi-pre-commit - rev: e98930bdc210d3387007f9252d8c1694ea7e410f # frozen: 0.27.7 + rev: b9d6d13dc9b753c5ff24ed5f8470671189c3fcf0 # frozen: 0.28.0 hooks: - id: gersemi diff --git a/docker/pre-commit/Dockerfile b/docker/pre-commit/Dockerfile deleted file mode 100644 index 1d012bf0e..000000000 --- a/docker/pre-commit/Dockerfile +++ /dev/null @@ -1,38 +0,0 @@ -ARG GHCR_REPO=invalid -FROM ${GHCR_REPO}/clio-tools:latest AS clio-tools - -# We're using Ubuntu 24.04 to have a more recent version of Python -FROM ubuntu:24.04 - -ARG DEBIAN_FRONTEND=noninteractive - -SHELL ["/bin/bash", "-o", "pipefail", "-c"] - -# hadolint ignore=DL3002 -USER root -WORKDIR /root - -# Install common tools and dependencies -RUN apt-get update \ - && apt-get install -y --no-install-recommends --no-install-suggests \ - curl \ - git \ - libatomic1 \ - software-properties-common \ - && apt-get clean \ - && rm -rf /var/lib/apt/lists/* - -# Install Python tools -RUN apt-get update \ - && apt-get install -y --no-install-recommends --no-install-suggests \ - python3 \ - python3-pip \ - && apt-get clean \ - && rm -rf /var/lib/apt/lists/* - -RUN pip install -q --no-cache-dir --break-system-packages \ - pre-commit - -COPY --from=clio-tools \ - /usr/local/bin/doxygen \ - /usr/local/bin/ From 6ce668f49316e10291c23ffcfe578f7309932a85 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 4 Aug 2026 14:52:29 +0100 Subject: [PATCH 3/7] chore: Add clang-tidy rules for lower_case namespaces (#3165) --- .clang-tidy | 2 ++ .github/workflows/clang-tidy.yml | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index aa6a3c1db..4e5a13602 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -165,6 +165,8 @@ CheckOptions: readability-braces-around-statements.ShortStatementLines: 2 readability-identifier-naming.MacroDefinitionCase: UPPER_CASE + readability-identifier-naming.NamespaceCase: lower_case + readability-identifier-naming.InlineNamespaceCase: lower_case readability-identifier-naming.ClassCase: CamelCase readability-identifier-naming.StructCase: CamelCase readability-identifier-naming.UnionCase: CamelCase diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 2ec1644dd..809ebdcd0 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -17,7 +17,7 @@ on: - "tests/**" - "benchmarks/**" - - .clang_tidy + - .clang-tidy concurrency: # Only cancel in-progress jobs or runs for the current workflow - matches against branch & tags From 466b60f8eac94133e2eb5808303e8891843e08a6 Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Tue, 4 Aug 2026 12:22:43 -0400 Subject: [PATCH 4/7] feat: Support filtering txns by mpt_issuance_id (#3153) Adds an optional `mpt_issuance_id ` field to Account_tx so that users can filter Account transactions if the transaction is involved in a specific `mpt_issuance_id ` --- src/etl/MPTHelpers.cpp | 19 ++ src/etl/MPTHelpers.hpp | 20 ++ src/rpc/CMakeLists.txt | 2 +- src/rpc/handlers/AccountTx.cpp | 19 ++ src/rpc/handlers/AccountTx.hpp | 2 + src/rpc/handlers/MPTHolders.cpp | 1 + tests/unit/rpc/handlers/AccountTxTests.cpp | 303 +++++++++++++++++++++ 7 files changed, 365 insertions(+), 1 deletion(-) diff --git a/src/etl/MPTHelpers.cpp b/src/etl/MPTHelpers.cpp index 975dc6b7f..11471c75f 100644 --- a/src/etl/MPTHelpers.cpp +++ b/src/etl/MPTHelpers.cpp @@ -155,6 +155,25 @@ getMPTokenIssuanceTxsFromTx(xrpl::TxMeta const& txMeta, xrpl::STTx const& sttx) return result; } +bool +referencesMptIssuance( + xrpl::TxMeta const& txMeta, + xrpl::STTx const& sttx, + xrpl::uint192 const& mptIssuanceID +) +{ + if (txMeta.getResultTER() == xrpl::tesSUCCESS) { + for (auto const& node : txMeta.getNodes()) { + if (getMPTokenIssuanceIDFromNode(node) == mptIssuanceID) + return true; + } + } + + MPTokenIssuanceIDs issuanceIDs; + addMPTokenIssuanceIDsFromTx(issuanceIDs, sttx); + return issuanceIDs.contains(mptIssuanceID); +} + std::optional getMPTHolderFromObj(std::string const& key, std::string const& blob) { diff --git a/src/etl/MPTHelpers.hpp b/src/etl/MPTHelpers.hpp index cdc3405ad..69241df80 100644 --- a/src/etl/MPTHelpers.hpp +++ b/src/etl/MPTHelpers.hpp @@ -3,6 +3,7 @@ #include "data/DBHelpers.hpp" +#include #include #include @@ -51,4 +52,23 @@ getMPTHolderFromObj(std::string const& key, std::string const& blob); std::vector getMPTokenIssuanceTxsFromTx(xrpl::TxMeta const& txMeta, xrpl::STTx const& sttx); +/** + * @brief Check whether a transaction references a specific MPT issuance. + * + * @note Scans the same sources as getMPTokenIssuanceTxsFromTx (metadata's affected + * MPTokenIssuance/MPToken nodes, and the transaction's own MPTokenIssuanceID/MPT issue fields), but + * exits as soon as a match is found instead of collecting every distinct issuance touched. + * + * @param txMeta Transaction metadata. + * @param sttx The transaction. + * @param mptIssuanceID The MPT issuance to check for. + * @return true if the transaction references mptIssuanceID. + */ +bool +referencesMptIssuance( + xrpl::TxMeta const& txMeta, + xrpl::STTx const& sttx, + xrpl::uint192 const& mptIssuanceID +); + } // namespace etl diff --git a/src/rpc/CMakeLists.txt b/src/rpc/CMakeLists.txt index 9691de44d..b427817b1 100644 --- a/src/rpc/CMakeLists.txt +++ b/src/rpc/CMakeLists.txt @@ -58,4 +58,4 @@ target_sources( handlers/VaultInfo.cpp ) -target_link_libraries(clio_rpc PUBLIC clio_util clio_data) +target_link_libraries(clio_rpc PUBLIC clio_util clio_data clio_etl) diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index 720529438..154741822 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -1,6 +1,7 @@ #include "rpc/handlers/AccountTx.hpp" #include "data/Types.hpp" +#include "etl/MPTHelpers.hpp" #include "rpc/Errors.hpp" #include "rpc/JS.hpp" #include "rpc/RPCHelpers.hpp" @@ -16,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -128,6 +130,10 @@ AccountTxHandler::process(AccountTxHandler::Input const& input, Context const& c if (retCursor) response.marker = {.ledger = retCursor->ledgerSequence, .seq = retCursor->transactionIndex}; + std::optional mptIssuanceFilter; + if (input.mptIssuanceId) + mptIssuanceFilter = xrpl::uint192{input.mptIssuanceId->c_str()}; + for (auto const& txnPlusMeta : blobs) { // over the range if ((txnPlusMeta.ledgerSequence < minIndex && !input.forward) || @@ -142,6 +148,14 @@ AccountTxHandler::process(AccountTxHandler::Input const& input, Context const& c boost::json::object obj; + // Skip all Txns where the specified filter mpt_id doesn't match the query + if (mptIssuanceFilter) { + auto const [sttx, txMeta] = + deserializeTxPlusMeta(txnPlusMeta, txnPlusMeta.ledgerSequence); + if (!etl::referencesMptIssuance(*txMeta, *sttx, *mptIssuanceFilter)) + continue; + } + // if binary is false or transactionType is specified, we need to expand the transaction if (!input.binary || input.transactionTypeInLowercase.has_value()) { auto [txn, meta] = toExpandedJson(txnPlusMeta, ctx.apiVersion, NFTokenjson::ENABLE); @@ -298,6 +312,11 @@ tag_invoke(boost::json::value_to_tag, boost::json::valu boost::json::value_to(jsonObject.at("tx_type")); } + if (jsonObject.contains(JS(mpt_issuance_id))) { + input.mptIssuanceId = + boost::json::value_to(jsonObject.at(JS(mpt_issuance_id))); + } + return input; } diff --git a/src/rpc/handlers/AccountTx.hpp b/src/rpc/handlers/AccountTx.hpp index d21cf5814..3dcd6dc9e 100644 --- a/src/rpc/handlers/AccountTx.hpp +++ b/src/rpc/handlers/AccountTx.hpp @@ -85,6 +85,7 @@ public: std::optional limit; std::optional marker; std::optional transactionTypeInLowercase; + std::optional mptIssuanceId; }; using Result = HandlerReturnType; @@ -141,6 +142,7 @@ public: typesKeysInLowercase.cbegin(), typesKeysInLowercase.cend() ), }, + {JS(mpt_issuance_id), validation::CustomValidators::uint192HexStringValidator}, }; static auto const kRpcSpec = RpcSpec{ diff --git a/src/rpc/handlers/MPTHolders.cpp b/src/rpc/handlers/MPTHolders.cpp index ba67302ca..247f7a815 100644 --- a/src/rpc/handlers/MPTHolders.cpp +++ b/src/rpc/handlers/MPTHolders.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include diff --git a/tests/unit/rpc/handlers/AccountTxTests.cpp b/tests/unit/rpc/handlers/AccountTxTests.cpp index 7709d08ce..1c2b3522c 100644 --- a/tests/unit/rpc/handlers/AccountTxTests.cpp +++ b/tests/unit/rpc/handlers/AccountTxTests.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -35,6 +36,8 @@ constexpr auto kLedgerHash = "4BC50C9B0D8515D3EAAE1E74B29A95804346C491EE1A95BF25 constexpr auto kNftId = "05FB0EB4B899F056FA095537C5817163801F544BAFCEA39C995D76DB4D16F9DF"; constexpr auto kNftID2 = "05FB0EB4B899F056FA095537C5817163801F544BAFCEA39C995D76DB4D16F9DA"; constexpr auto kNftID3 = "15FB0EB4B899F056FA095537C5817163801F544BAFCEA39C995D76DB4D16F9DF"; +constexpr auto kMptIssuanceId = "000000014B4E9C06F24296074F7BC48F92A97916C6DC5EA9"; +constexpr auto kMptIssuanceId2 = "000000024B4E9C06F24296074F7BC48F92A97916C6DC5EA9"; constexpr auto kIndex = "E6DBAFC99223B42257915A63DFC6B0C032D4070F9A574B255AD97466726FC322"; } // namespace @@ -391,6 +394,24 @@ struct AccountTxParameterTest : public RPCAccountTxHandlerTest, })JSON", .expectedError = "invalidParams", .expectedErrorMessage = "Invalid field 'tx_type'." + }, + AccountTxParamTestCaseBundle{ + .testName = "MPTIssuanceIdMalformed", + .testJson = R"JSON({ + "account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn", + "mpt_issuance_id": "xxx" + })JSON", + .expectedError = "invalidParams", + .expectedErrorMessage = "mpt_issuance_idMalformed" + }, + AccountTxParamTestCaseBundle{ + .testName = "MPTIssuanceIdNotString", + .testJson = R"JSON({ + "account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn", + "mpt_issuance_id": 12 + })JSON", + .expectedError = "invalidParams", + .expectedErrorMessage = "mpt_issuance_idNotString" } }; }; @@ -1746,6 +1767,288 @@ TEST_F(RPCAccountTxHandlerTest, MPTTxs_API_v2) }); } +TEST_F(RPCAccountTxHandlerTest, MPTIssuanceIdFilterMatch) +{ + auto const out = fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": 10, + "ledger_index_max": 30, + "transactions": [ + {{ + "meta": {{ + "AffectedNodes": [ + {{ + "CreatedNode": {{ + "LedgerEntryType": "MPTokenIssuance", + "LedgerIndex": "0000000000000000000000000000000000000000000000000000000000000000", + "NewFields": {{ + "Flags": 0, + "Issuer": "{}", + "LedgerEntryType": "MPTokenIssuance", + "MPTokenMetadata": "746573742D6D657461", + "MaximumAmount": "0", + "OutstandingAmount": "0", + "OwnerNode": "0", + "PreviousTxnID": "0000000000000000000000000000000000000000000000000000000000000000", + "PreviousTxnLgrSeq": 0, + "Sequence": 1 + }} + }} + }} + ], + "TransactionIndex": 0, + "TransactionResult": "tesSUCCESS", + "mpt_issuance_id": "{}" + }}, + "hash": "A52221F4003C281D3C83F501F418B55A1F9DC1C6A129EF13E1A8F0E5C008DAE3", + "ledger_index": 11, + "ledger_hash": "{}", + "close_time_iso": "2000-01-01T00:00:00Z", + "tx_json": {{ + "Account": "{}", + "Fee": "50", + "Sequence": 1, + "SigningPubKey": "74657374", + "TransactionType": "MPTokenIssuanceCreate", + "ledger_index": 11, + "ctid": "C000000B00000000", + "date": 1 + }}, + "validated": true + }} + ], + "validated": true + }})JSON", + kAccount, + kAccount, + kMptIssuanceId, + kLedgerHash, + kAccount + ); + + auto mptTx = createMPTIssuanceCreateTxWithMetadata(kAccount, 50, 1); + mptTx.ledgerSequence = kMinSeq + 1; + mptTx.date = 1; + + auto transactions = std::vector{std::move(mptTx)}; + auto const transCursor = + TransactionsAndCursor{.txns = std::move(transactions), .cursor = std::nullopt}; + + EXPECT_CALL(*backend_, fetchAccountTransactions).WillOnce(Return(transCursor)); + + auto const ledgerHeader = createLedgerHeader(kLedgerHash, kMinSeq + 1); + EXPECT_CALL(*backend_, fetchLedgerBySequence(kMinSeq + 1, _)).WillOnce(Return(ledgerHeader)); + + runSpawn([&, this](auto yield) { + auto const handler = AnyHandler{AccountTxHandler{backend_, mockETLServicePtr_}}; + auto const input = boost::json::parse( + fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": {}, + "ledger_index_max": {}, + "mpt_issuance_id": "{}" + }})JSON", + kAccount, + kMinSeq, + kMaxSeq, + kMptIssuanceId + ) + ); + auto const output = handler.process(input, Context{.yield = yield, .apiVersion = 2u}); + ASSERT_TRUE(output); + EXPECT_EQ(*output.result, boost::json::parse(out)); + }); +} + +TEST_F(RPCAccountTxHandlerTest, MPTIssuanceIdFilterNoMatch) +{ + auto const out = fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": 10, + "ledger_index_max": 30, + "transactions": [], + "validated": true + }})JSON", + kAccount + ); + + auto mptTx = createMPTIssuanceCreateTxWithMetadata(kAccount, 50, 1); + mptTx.ledgerSequence = kMinSeq + 1; + mptTx.date = 1; + + auto transactions = std::vector{std::move(mptTx)}; + auto const transCursor = + TransactionsAndCursor{.txns = std::move(transactions), .cursor = std::nullopt}; + + EXPECT_CALL(*backend_, fetchAccountTransactions).WillOnce(Return(transCursor)); + // the tx is filtered out before the per-tx ledger_hash/close_time_iso enrichment step, so this + // must never be called + EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(0); + + runSpawn([&, this](auto yield) { + auto const handler = AnyHandler{AccountTxHandler{backend_, mockETLServicePtr_}}; + auto const input = boost::json::parse( + fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": {}, + "ledger_index_max": {}, + "mpt_issuance_id": "{}" + }})JSON", + kAccount, + kMinSeq, + kMaxSeq, + kMptIssuanceId2 + ) + ); + auto const output = handler.process(input, Context{.yield = yield, .apiVersion = 2u}); + ASSERT_TRUE(output); + EXPECT_EQ(*output.result, boost::json::parse(out)); + }); +} + +TEST_F(RPCAccountTxHandlerTest, MPTIssuanceIdFilterWithMatchingTxType) +{ + auto mptTx = createMPTIssuanceCreateTxWithMetadata(kAccount, 50, 1); + mptTx.ledgerSequence = kMinSeq + 1; + mptTx.date = 1; + + auto transactions = std::vector{std::move(mptTx)}; + auto const transCursor = + TransactionsAndCursor{.txns = std::move(transactions), .cursor = std::nullopt}; + + EXPECT_CALL(*backend_, fetchAccountTransactions).WillOnce(Return(transCursor)); + + auto const ledgerHeader = createLedgerHeader(kLedgerHash, kMinSeq + 1); + EXPECT_CALL(*backend_, fetchLedgerBySequence(kMinSeq + 1, _)).WillOnce(Return(ledgerHeader)); + + runSpawn([&, this](auto yield) { + auto const handler = AnyHandler{AccountTxHandler{backend_, mockETLServicePtr_}}; + auto const input = boost::json::parse( + fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": {}, + "ledger_index_max": {}, + "mpt_issuance_id": "{}", + "tx_type": "MPTokenIssuanceCreate" + }})JSON", + kAccount, + kMinSeq, + kMaxSeq, + kMptIssuanceId + ) + ); + auto const output = handler.process(input, Context{.yield = yield, .apiVersion = 2u}); + ASSERT_TRUE(output); + EXPECT_EQ(output.result->as_object().at("transactions").as_array().size(), 1); + }); +} + +TEST_F(RPCAccountTxHandlerTest, MPTIssuanceIdFilterWithMismatchingTxType) +{ + auto mptTx = createMPTIssuanceCreateTxWithMetadata(kAccount, 50, 1); + mptTx.ledgerSequence = kMinSeq + 1; + mptTx.date = 1; + + auto transactions = std::vector{std::move(mptTx)}; + auto const transCursor = + TransactionsAndCursor{.txns = std::move(transactions), .cursor = std::nullopt}; + + EXPECT_CALL(*backend_, fetchAccountTransactions).WillOnce(Return(transCursor)); + // tx_type mismatch causes the transaction to be skipped before the ledger_hash enrichment step + EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(0); + + runSpawn([&, this](auto yield) { + auto const handler = AnyHandler{AccountTxHandler{backend_, mockETLServicePtr_}}; + auto const input = boost::json::parse( + fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": {}, + "ledger_index_max": {}, + "mpt_issuance_id": "{}", + "tx_type": "Payment" + }})JSON", + kAccount, + kMinSeq, + kMaxSeq, + kMptIssuanceId + ) + ); + auto const output = handler.process(input, Context{.yield = yield, .apiVersion = 2u}); + ASSERT_TRUE(output); + EXPECT_EQ(output.result->as_object().at("transactions").as_array().size(), 0); + }); +} + +TEST_F(RPCAccountTxHandlerTest, MPTIssuanceIdFilterBinary) +{ + auto mptTx = createMPTIssuanceCreateTxWithMetadata(kAccount, 50, 1); + mptTx.ledgerSequence = kMinSeq + 1; + mptTx.date = 1; + + auto const expectedMetaBlob = xrpl::strHex(mptTx.metadata); + auto const expectedTxBlob = xrpl::strHex(mptTx.transaction); + + // non-matching transactions (no mpt_issuance_id reference) mixed in alongside the matching one, + // to prove the binary path actually filters rather than just passing everything through + auto transactions = genTransactions(kMinSeq + 2, kMinSeq + 3); + transactions.push_back(std::move(mptTx)); + auto const transCursor = + TransactionsAndCursor{.txns = std::move(transactions), .cursor = std::nullopt}; + + EXPECT_CALL(*backend_, fetchAccountTransactions).WillOnce(Return(transCursor)); + // the binary path never deserializes the tx to JSON, so no ledger_hash enrichment happens + EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(0); + + auto const out = fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": 10, + "ledger_index_max": 30, + "transactions": [ + {{ + "meta_blob": "{}", + "tx_blob": "{}", + "ledger_index": {}, + "validated": true + }} + ], + "validated": true + }})JSON", + kAccount, + expectedMetaBlob, + expectedTxBlob, + kMinSeq + 1 + ); + + runSpawn([&, this](auto yield) { + auto const handler = AnyHandler{AccountTxHandler{backend_, mockETLServicePtr_}}; + auto const input = boost::json::parse( + fmt::format( + R"JSON({{ + "account": "{}", + "ledger_index_min": {}, + "ledger_index_max": {}, + "mpt_issuance_id": "{}", + "binary": true + }})JSON", + kAccount, + kMinSeq, + kMaxSeq, + kMptIssuanceId + ) + ); + auto const output = handler.process(input, Context{.yield = yield, .apiVersion = 2u}); + ASSERT_TRUE(output); + EXPECT_EQ(*output.result, boost::json::parse(out)); + }); +} + struct AccountTxTransactionBundle { std::string testName; std::string testJson; From f490117cfb36485020eb09755e246daf449ad61d Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:57:31 -0400 Subject: [PATCH 5/7] fix: Remove clio_etl from rpc cmake (#3166) --- src/etl/CMakeLists.txt | 2 +- src/etl/MPTHelpers.cpp | 105 ++--------------- src/etl/MPTHelpers.hpp | 19 ---- src/rpc/CMakeLists.txt | 2 +- src/rpc/handlers/AccountTx.cpp | 4 +- src/util/CMakeLists.txt | 1 + src/util/MPTIssuanceUtils.cpp | 96 ++++++++++++++++ src/util/MPTIssuanceUtils.hpp | 57 ++++++++++ tests/unit/CMakeLists.txt | 1 + tests/unit/util/MPTIssuanceUtilsTests.cpp | 131 ++++++++++++++++++++++ 10 files changed, 298 insertions(+), 120 deletions(-) create mode 100644 src/util/MPTIssuanceUtils.cpp create mode 100644 src/util/MPTIssuanceUtils.hpp create mode 100644 tests/unit/util/MPTIssuanceUtilsTests.cpp diff --git a/src/etl/CMakeLists.txt b/src/etl/CMakeLists.txt index 2b0ce2e0d..17cabe312 100644 --- a/src/etl/CMakeLists.txt +++ b/src/etl/CMakeLists.txt @@ -29,4 +29,4 @@ target_sources( impl/ext/Successor.cpp ) -target_link_libraries(clio_etl PUBLIC clio_data) +target_link_libraries(clio_etl PUBLIC clio_data clio_util) diff --git a/src/etl/MPTHelpers.cpp b/src/etl/MPTHelpers.cpp index 11471c75f..03db7a167 100644 --- a/src/etl/MPTHelpers.cpp +++ b/src/etl/MPTHelpers.cpp @@ -1,15 +1,12 @@ +#include "etl/MPTHelpers.hpp" + #include "data/DBHelpers.hpp" #include "util/Assert.hpp" +#include "util/MPTIssuanceUtils.hpp" -#include #include -#include #include -#include #include -#include -#include -#include #include #include #include @@ -49,90 +46,23 @@ getMPTHolderFromTx(xrpl::TxMeta const& txMeta, xrpl::STTx const&) return holders; } -namespace { - -using MPTokenIssuanceIDs = boost::container::flat_set; - -/** - * @brief Derive the MPTokenIssuanceID from an affected node in transaction metadata. - * - * @param node An entry of the metadata's AffectedNodes array. - * @return The 192-bit issuance ID if the node is an MPTokenIssuance or MPToken object. - */ -std::optional -getMPTokenIssuanceIDFromNode(xrpl::STObject const& node) -{ - auto const entryType = node.getFieldU16(xrpl::sfLedgerEntryType); - if (entryType != xrpl::ltMPTOKEN && entryType != xrpl::ltMPTOKEN_ISSUANCE) - return std::nullopt; - - auto const& fieldsName = - node.getFName() == xrpl::sfCreatedNode ? xrpl::sfNewFields : xrpl::sfFinalFields; - if (not node.isFieldPresent(fieldsName)) - return std::nullopt; - - auto const& fields = node.peekAtField(fieldsName).downcast(); - - if (entryType == xrpl::ltMPTOKEN) { - if (not fields.isFieldPresent(xrpl::sfMPTokenIssuanceID)) - return std::nullopt; - - return fields[xrpl::sfMPTokenIssuanceID]; - } - - // MPTokenIssuance objects carry no sfMPTokenIssuanceID, and the node's ledger key is a - // one-way hash that does not embed the ID, so reconstruct it from sfSequence and sfIssuer - if (not fields.isFieldPresent(xrpl::sfSequence) || not fields.isFieldPresent(xrpl::sfIssuer)) - return std::nullopt; - - return xrpl::makeMptID( - fields.getFieldU32(xrpl::sfSequence), fields.getAccountID(xrpl::sfIssuer) - ); -} - -void -addMPTokenIssuanceIDsFromTx(MPTokenIssuanceIDs& issuanceIDs, xrpl::STTx const& sttx) -{ - if (sttx.isFieldPresent(xrpl::sfMPTokenIssuanceID)) - issuanceIDs.insert(sttx.getFieldH192(xrpl::sfMPTokenIssuanceID)); - - for (xrpl::STBase const& field : sttx) { - switch (field.getSType()) { - case xrpl::STI_AMOUNT: { - auto const& amount = field.downcast(); - if (amount.holds()) - issuanceIDs.insert(amount.get().getMptID()); - break; - } - case xrpl::STI_ISSUE: { - auto const& issue = field.downcast(); - if (issue.holds()) - issuanceIDs.insert(issue.value().get().getMptID()); - break; - } - default: - break; - } - } -} - -} // namespace - std::vector getMPTokenIssuanceTxsFromTx(xrpl::TxMeta const& txMeta, xrpl::STTx const& sttx) { // Collect each distinct issuance only once per transaction; the same set of affected accounts // is attached to every record produced below. - MPTokenIssuanceIDs issuanceIDs; + util::MPTokenIssuanceIDs issuanceIDs; if (txMeta.getResultTER() == xrpl::tesSUCCESS) { for (auto const& node : txMeta.getNodes()) { - if (auto const issuanceID = getMPTokenIssuanceIDFromNode(node); issuanceID.has_value()) + if (auto const issuanceID = util::getMPTokenIssuanceIDFromNode(node); + issuanceID.has_value()) { issuanceIDs.insert(*issuanceID); + } } } - addMPTokenIssuanceIDsFromTx(issuanceIDs, sttx); + util::addMPTokenIssuanceIDsFromTx(issuanceIDs, sttx); if (issuanceIDs.empty()) return {}; @@ -155,25 +85,6 @@ getMPTokenIssuanceTxsFromTx(xrpl::TxMeta const& txMeta, xrpl::STTx const& sttx) return result; } -bool -referencesMptIssuance( - xrpl::TxMeta const& txMeta, - xrpl::STTx const& sttx, - xrpl::uint192 const& mptIssuanceID -) -{ - if (txMeta.getResultTER() == xrpl::tesSUCCESS) { - for (auto const& node : txMeta.getNodes()) { - if (getMPTokenIssuanceIDFromNode(node) == mptIssuanceID) - return true; - } - } - - MPTokenIssuanceIDs issuanceIDs; - addMPTokenIssuanceIDsFromTx(issuanceIDs, sttx); - return issuanceIDs.contains(mptIssuanceID); -} - std::optional getMPTHolderFromObj(std::string const& key, std::string const& blob) { diff --git a/src/etl/MPTHelpers.hpp b/src/etl/MPTHelpers.hpp index 69241df80..ba69b704c 100644 --- a/src/etl/MPTHelpers.hpp +++ b/src/etl/MPTHelpers.hpp @@ -52,23 +52,4 @@ getMPTHolderFromObj(std::string const& key, std::string const& blob); std::vector getMPTokenIssuanceTxsFromTx(xrpl::TxMeta const& txMeta, xrpl::STTx const& sttx); -/** - * @brief Check whether a transaction references a specific MPT issuance. - * - * @note Scans the same sources as getMPTokenIssuanceTxsFromTx (metadata's affected - * MPTokenIssuance/MPToken nodes, and the transaction's own MPTokenIssuanceID/MPT issue fields), but - * exits as soon as a match is found instead of collecting every distinct issuance touched. - * - * @param txMeta Transaction metadata. - * @param sttx The transaction. - * @param mptIssuanceID The MPT issuance to check for. - * @return true if the transaction references mptIssuanceID. - */ -bool -referencesMptIssuance( - xrpl::TxMeta const& txMeta, - xrpl::STTx const& sttx, - xrpl::uint192 const& mptIssuanceID -); - } // namespace etl diff --git a/src/rpc/CMakeLists.txt b/src/rpc/CMakeLists.txt index b427817b1..9691de44d 100644 --- a/src/rpc/CMakeLists.txt +++ b/src/rpc/CMakeLists.txt @@ -58,4 +58,4 @@ target_sources( handlers/VaultInfo.cpp ) -target_link_libraries(clio_rpc PUBLIC clio_util clio_data clio_etl) +target_link_libraries(clio_rpc PUBLIC clio_util clio_data) diff --git a/src/rpc/handlers/AccountTx.cpp b/src/rpc/handlers/AccountTx.cpp index 154741822..f15bfdb60 100644 --- a/src/rpc/handlers/AccountTx.cpp +++ b/src/rpc/handlers/AccountTx.cpp @@ -1,7 +1,6 @@ #include "rpc/handlers/AccountTx.hpp" #include "data/Types.hpp" -#include "etl/MPTHelpers.hpp" #include "rpc/Errors.hpp" #include "rpc/JS.hpp" #include "rpc/RPCHelpers.hpp" @@ -9,6 +8,7 @@ #include "rpc/common/Types.hpp" #include "util/Assert.hpp" #include "util/JsonUtils.hpp" +#include "util/MPTIssuanceUtils.hpp" #include "util/Profiler.hpp" #include "util/log/Logger.hpp" @@ -152,7 +152,7 @@ AccountTxHandler::process(AccountTxHandler::Input const& input, Context const& c if (mptIssuanceFilter) { auto const [sttx, txMeta] = deserializeTxPlusMeta(txnPlusMeta, txnPlusMeta.ledgerSequence); - if (!etl::referencesMptIssuance(*txMeta, *sttx, *mptIssuanceFilter)) + if (!util::referencesMptIssuance(*txMeta, *sttx, *mptIssuanceFilter)) continue; } diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 8e08507b7..b5e4e5a0b 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -35,6 +35,7 @@ target_sources( TimeUtils.cpp TxUtils.cpp LedgerUtils.cpp + MPTIssuanceUtils.cpp config/Array.cpp config/ArrayView.cpp config/ConfigConstraints.cpp diff --git a/src/util/MPTIssuanceUtils.cpp b/src/util/MPTIssuanceUtils.cpp new file mode 100644 index 000000000..5c112597e --- /dev/null +++ b/src/util/MPTIssuanceUtils.cpp @@ -0,0 +1,96 @@ +#include "util/MPTIssuanceUtils.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace util { + +std::optional +getMPTokenIssuanceIDFromNode(xrpl::STObject const& node) +{ + auto const entryType = node.getFieldU16(xrpl::sfLedgerEntryType); + if (entryType != xrpl::ltMPTOKEN && entryType != xrpl::ltMPTOKEN_ISSUANCE) + return std::nullopt; + + auto const& fieldsName = + node.getFName() == xrpl::sfCreatedNode ? xrpl::sfNewFields : xrpl::sfFinalFields; + if (not node.isFieldPresent(fieldsName)) + return std::nullopt; + + auto const& fields = node.peekAtField(fieldsName).downcast(); + + if (entryType == xrpl::ltMPTOKEN) { + if (not fields.isFieldPresent(xrpl::sfMPTokenIssuanceID)) + return std::nullopt; + + return fields[xrpl::sfMPTokenIssuanceID]; + } + + // MPTokenIssuance objects carry no sfMPTokenIssuanceID, and the node's ledger key is a + // one-way hash that does not embed the ID, so reconstruct it from sfSequence and sfIssuer + if (not fields.isFieldPresent(xrpl::sfSequence) || not fields.isFieldPresent(xrpl::sfIssuer)) + return std::nullopt; + + return xrpl::makeMptID( + fields.getFieldU32(xrpl::sfSequence), fields.getAccountID(xrpl::sfIssuer) + ); +} + +void +addMPTokenIssuanceIDsFromTx(MPTokenIssuanceIDs& issuanceIDs, xrpl::STTx const& sttx) +{ + if (sttx.isFieldPresent(xrpl::sfMPTokenIssuanceID)) + issuanceIDs.insert(sttx.getFieldH192(xrpl::sfMPTokenIssuanceID)); + + for (xrpl::STBase const& field : sttx) { + switch (field.getSType()) { + case xrpl::STI_AMOUNT: { + auto const& amount = field.downcast(); + if (amount.holds()) + issuanceIDs.insert(amount.get().getMptID()); + break; + } + case xrpl::STI_ISSUE: { + auto const& issue = field.downcast(); + if (issue.holds()) + issuanceIDs.insert(issue.value().get().getMptID()); + break; + } + default: + break; + } + } +} + +bool +referencesMptIssuance( + xrpl::TxMeta const& txMeta, + xrpl::STTx const& sttx, + xrpl::uint192 const& mptIssuanceID +) +{ + if (txMeta.getResultTER() == xrpl::tesSUCCESS) { + for (auto const& node : txMeta.getNodes()) { + if (getMPTokenIssuanceIDFromNode(node) == mptIssuanceID) + return true; + } + } + + MPTokenIssuanceIDs issuanceIDs; + addMPTokenIssuanceIDsFromTx(issuanceIDs, sttx); + return issuanceIDs.contains(mptIssuanceID); +} + +} // namespace util diff --git a/src/util/MPTIssuanceUtils.hpp b/src/util/MPTIssuanceUtils.hpp new file mode 100644 index 000000000..60f444313 --- /dev/null +++ b/src/util/MPTIssuanceUtils.hpp @@ -0,0 +1,57 @@ +#pragma once + +#include +#include +#include +#include +#include + +#include + +namespace util { + +/** + * @brief A set of distinct MPTokenIssuanceIDs. + */ +using MPTokenIssuanceIDs = boost::container::flat_set; + +/** + * @brief Derive the MPTokenIssuanceID from an affected node in transaction metadata. + * + * @param node An entry of the metadata's AffectedNodes array. + * @return The 192-bit issuance ID if the node is an MPTokenIssuance or MPToken object. + */ +std::optional +getMPTokenIssuanceIDFromNode(xrpl::STObject const& node); + +/** + * @brief Collect every MPTokenIssuanceID referenced by a transaction's own fields. + * + * @note Checks the top-level sfMPTokenIssuanceID field, plus any STI_AMOUNT/STI_ISSUE field holding + * an xrpl::MPTIssue (e.g. Payment's sfAmount, AMM's sfAsset/sfAsset2). + * + * @param [out] issuanceIDs Set to insert each found issuance ID into. + * @param sttx The transaction. + */ +void +addMPTokenIssuanceIDsFromTx(MPTokenIssuanceIDs& issuanceIDs, xrpl::STTx const& sttx); + +/** + * @brief Check whether a transaction references a specific MPT issuance. + * + * @note Scans the transaction's metadata for affected MPTokenIssuance/MPToken nodes, and the + * transaction's own MPTokenIssuanceID/MPT issue fields, exiting as soon as a match is found. + * + * @param txMeta Transaction metadata. + * @param sttx The transaction. + * @param mptIssuanceID The MPT issuance to check for. + * @return true if the transaction references mptIssuanceID. + */ +bool +referencesMptIssuance( + xrpl::TxMeta const& txMeta, + xrpl::STTx const& sttx, + xrpl::uint192 const& mptIssuanceID +); + +} // namespace util diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index a5a35a406..dc805b193 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -181,6 +181,7 @@ target_sources( util/ChannelTests.cpp util/CoroutineTest.cpp util/MoveTrackerTests.cpp + util/MPTIssuanceUtilsTests.cpp util/ObservableValueTest.cpp util/ObservableValueAtomicTest.cpp util/RandomTests.cpp diff --git a/tests/unit/util/MPTIssuanceUtilsTests.cpp b/tests/unit/util/MPTIssuanceUtilsTests.cpp new file mode 100644 index 000000000..2590dea65 --- /dev/null +++ b/tests/unit/util/MPTIssuanceUtilsTests.cpp @@ -0,0 +1,131 @@ +#include "util/MPTIssuanceUtils.hpp" +#include "util/MPTokenTestObjects.hpp" +#include "util/TestObject.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace { + +constexpr auto kAccount = "rM2AGCCCRb373FRuD8wHyUwUsh2dV4BW5Q"; +constexpr auto kAccount2 = "rnd1nHuzceyQDqnLH8urWNr4QBKt4v7WVk"; +constexpr auto kIssuer = "rK1EX542EgA9m948JrJRaEzwLVEhqWvnr9"; +constexpr auto kTX = "13F1A95D7AAB7108D5CE7EEAF504B2894B8C674E6D68499076441C4837282BF8"; +constexpr std::uint32_t kIssuanceSeq = 7; +constexpr std::uint32_t kLedgerSeq = 99; +constexpr std::uint32_t kTxIndex = 4; + +xrpl::Slice const kSlice("test", 4); + +xrpl::uint192 +defaultIssuanceID() +{ + return xrpl::makeMptID(kIssuanceSeq, getAccountIdWithString(kIssuer)); +} + +xrpl::TxMeta +createTxMeta(std::vector nodes, int result = xrpl::tesSUCCESS) +{ + xrpl::STObject metaObj(xrpl::sfTransactionMetaData); + metaObj.setFieldU8(xrpl::sfTransactionResult, result); + metaObj.setFieldU32(xrpl::sfTransactionIndex, kTxIndex); + + xrpl::STArray affectedNodes(xrpl::sfAffectedNodes); + for (auto& node : nodes) + affectedNodes.push_back(std::move(node)); + metaObj.setFieldArray(xrpl::sfAffectedNodes, affectedNodes); + + return xrpl::TxMeta{xrpl::uint256(kTX), kLedgerSeq, metaObj.getSerializer().peekData()}; +} + +xrpl::STTx +createPaymentTx() +{ + xrpl::STObject obj(xrpl::sfTransaction); + obj.setFieldU16(xrpl::sfTransactionType, xrpl::ttPAYMENT); + obj.setAccountID(xrpl::sfAccount, getAccountIdWithString(kAccount)); + obj.setFieldAmount(xrpl::sfAmount, xrpl::STAmount(100, false)); + obj.setAccountID(xrpl::sfDestination, getAccountIdWithString(kAccount2)); + obj.setFieldAmount(xrpl::sfFee, xrpl::STAmount(10, false)); + obj.setFieldU32(xrpl::sfSequence, 1); + obj.setFieldVL(xrpl::sfSigningPubKey, kSlice); + + auto const serialized = obj.getSerializer(); + return xrpl::STTx{xrpl::SerialIter{serialized.slice()}}; +} + +xrpl::STTx +createMptIssuanceSetTx() +{ + xrpl::STObject obj(xrpl::sfTransaction); + obj.setFieldU16(xrpl::sfTransactionType, xrpl::ttMPTOKEN_ISSUANCE_SET); + obj.setAccountID(xrpl::sfAccount, getAccountIdWithString(kAccount)); + obj.setFieldAmount(xrpl::sfFee, xrpl::STAmount(10, false)); + obj.setFieldU32(xrpl::sfSequence, 1); + obj.setFieldVL(xrpl::sfSigningPubKey, kSlice); + obj[xrpl::sfMPTokenIssuanceID] = defaultIssuanceID(); + + auto const serialized = obj.getSerializer(); + return xrpl::STTx{xrpl::SerialIter{serialized.slice()}}; +} + +} // namespace + +TEST(MPTIssuanceUtilsTest, ReferencesMptIssuance_MatchesFromAffectedNode) +{ + std::vector nodes; + nodes.push_back(util::createMPTokenNode(xrpl::sfCreatedNode, defaultIssuanceID(), kAccount)); + auto const txMeta = createTxMeta(std::move(nodes)); + + EXPECT_TRUE(util::referencesMptIssuance(txMeta, createPaymentTx(), defaultIssuanceID())); +} + +TEST(MPTIssuanceUtilsTest, ReferencesMptIssuance_NoMatchWhenIssuanceDiffers) +{ + auto const otherIssuance = xrpl::makeMptID(kIssuanceSeq + 1, getAccountIdWithString(kIssuer)); + std::vector nodes; + nodes.push_back(util::createMPTokenNode(xrpl::sfCreatedNode, otherIssuance, kAccount)); + auto const txMeta = createTxMeta(std::move(nodes)); + + EXPECT_FALSE(util::referencesMptIssuance(txMeta, createPaymentTx(), defaultIssuanceID())); +} + +TEST(MPTIssuanceUtilsTest, ReferencesMptIssuance_FailedTxIgnoresAffectedNodes) +{ + std::vector nodes; + nodes.push_back(util::createMPTokenNode(xrpl::sfCreatedNode, defaultIssuanceID(), kAccount)); + auto const txMeta = createTxMeta(std::move(nodes), xrpl::tecINCOMPLETE); + + EXPECT_FALSE(util::referencesMptIssuance(txMeta, createPaymentTx(), defaultIssuanceID())); +} + +TEST(MPTIssuanceUtilsTest, ReferencesMptIssuance_FailedTxStillMatchesViaOwnFields) +{ + // A failed transaction has no meaningful affected nodes, but its own fields (e.g. + // sfMPTokenIssuanceID) are scanned regardless of the transaction result. + auto const txMeta = createTxMeta({}, xrpl::tecINCOMPLETE); + + EXPECT_TRUE(util::referencesMptIssuance(txMeta, createMptIssuanceSetTx(), defaultIssuanceID())); +} + +TEST(MPTIssuanceUtilsTest, ReferencesMptIssuance_NoReferenceReturnsFalse) +{ + auto const txMeta = createTxMeta({}); + + EXPECT_FALSE(util::referencesMptIssuance(txMeta, createPaymentTx(), defaultIssuanceID())); +} From dbdbe4ea1f49b3cb0399c1bc459b004c192e3c16 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 10 Aug 2026 14:39:15 +0100 Subject: [PATCH 6/7] fix: Introduce retry for transient errors from DB (#3167) --- docs/config-description.md | 22 ++- src/data/BackendInterface.hpp | 130 ++++++++++++++++-- src/data/cassandra/CassandraBackendFamily.hpp | 18 +++ src/data/cassandra/Error.hpp | 11 -- src/data/cassandra/SettingsProvider.cpp | 13 +- src/data/cassandra/SettingsProvider.hpp | 21 +++ src/data/cassandra/impl/ExecutionStrategy.hpp | 28 ++-- src/data/cassandra/impl/RetryPolicy.hpp | 3 +- src/etl/impl/CacheLoader.hpp | 14 +- src/etl/impl/SubscriptionSource.cpp | 4 +- src/etl/impl/SubscriptionSource.hpp | 2 +- src/rpc/Errors.hpp | 4 +- src/rpc/RPCEngine.hpp | 8 +- src/util/Retry.cpp | 38 +++-- src/util/Retry.hpp | 57 ++++++-- src/util/config/ConfigDefinition.cpp | 10 +- src/util/config/ConfigDescription.hpp | 10 +- .../integration/data/BackendFactoryTests.cpp | 6 +- .../data/cassandra/BackendTests.cpp | 6 +- .../CassandraMigrationManagerTests.cpp | 6 +- tests/unit/CMakeLists.txt | 1 + tests/unit/data/BackendInterfaceTests.cpp | 127 +++++++++++++++++ tests/unit/data/cassandra/ErrorTests.cpp | 43 ++++++ .../data/cassandra/ExecutionStrategyTests.cpp | 45 +++++- .../data/cassandra/SettingsProviderTests.cpp | 75 +++++++++- tests/unit/rpc/RPCEngineTests.cpp | 2 +- tests/unit/util/RetryTests.cpp | 49 ++++++- 27 files changed, 665 insertions(+), 88 deletions(-) create mode 100644 tests/unit/data/cassandra/ErrorTests.cpp diff --git a/docs/config-description.md b/docs/config-description.md index be20dbdd6..c41957a2a 100644 --- a/docs/config-description.md +++ b/docs/config-description.md @@ -132,10 +132,26 @@ This document provides a list of all available Clio configuration properties in ### database.cassandra.request_timeout - **Required**: False -- **Type**: int +- **Type**: double - **Default value**: None -- **Constraints**: The minimum value is `1`. The maximum value is `4294967295`. -- **Description**: The maximum amount of time in seconds that the system waits for a request to be fetched from the database. +- **Constraints**: The value must be a positive double number. +- **Description**: The maximum amount of time in seconds that the system waits for a request to be fetched from the database. Should be set higher than the server side read timeout. If omitted, no request timeout is applied. + +### database.cassandra.initial_request_retry_delay + +- **Required**: True +- **Type**: double +- **Default value**: `0.5` +- **Constraints**: The value must be a positive double number. +- **Description**: How long in seconds to wait before the first retry of a database request that failed with a transient error. + +### database.cassandra.max_request_retry_delay + +- **Required**: True +- **Type**: double +- **Default value**: `5` +- **Constraints**: The value must be a positive double number. +- **Description**: Upper bound in seconds for the exponential backoff between retries of a database request. ### database.cassandra.username diff --git a/src/data/BackendInterface.hpp b/src/data/BackendInterface.hpp index 7d32f1220..6e1e70f69 100644 --- a/src/data/BackendInterface.hpp +++ b/src/data/BackendInterface.hpp @@ -4,6 +4,7 @@ #include "data/LedgerCacheInterface.hpp" #include "data/Types.hpp" #include "etl/CorruptionDetector.hpp" +#include "util/Retry.hpp" #include "util/Spawn.hpp" #include "util/log/Logger.hpp" @@ -35,41 +36,118 @@ namespace data { /** - * @brief Represents a database timeout error. + * @brief Represents a transient database error that the caller should retry. */ -class DatabaseTimeout : public std::exception { +class DatabaseError : public std::exception { + std::string message_{"Transient database error. Please retry the request"}; + public: + DatabaseError() = default; + + /** + * @brief Construct with a description of the underlying failure. + * + * @param message What actually went wrong. + */ + explicit DatabaseError(std::string message) : message_{std::move(message)} + { + } + /** * @return The error message as a C string */ [[nodiscard]] char const* - what() const throw() override + what() const noexcept override { - return "Database read timed out. Please retry the request"; + return message_.c_str(); } }; -static constexpr std::size_t kDefaultWaitBetweenRetry = 500; /** - * @brief A helper function that catches DatabaseTimeout exceptions and retries indefinitely. + * @brief Delay before the first retry in @ref retryOnTimeout(). + */ +static constexpr std::chrono::milliseconds kDefaultWaitBetweenRetry{500}; + +/** + * @brief Default upper bound for the exponential backoff in @ref retryOnTimeout(). + */ +static constexpr std::chrono::milliseconds kMaxWaitBetweenRetry{5'000}; + +/** + * @brief Default delays for @ref retryOnTimeout(). + */ +static constexpr util::Retry::Delays kDefaultRetryDelays{ + .initial = kDefaultWaitBetweenRetry, + .max = kDefaultWaitBetweenRetry +}; + +/** + * @brief Retry `func` while it throws DatabaseError, suspending the calling coroutine in between. * * @tparam FnType The type of function object to execute * @param func The function object to execute - * @param waitMs Delay between retry attempts + * @param yield The coroutine to suspend between attempts + * @param delays The delays to use between attempts * @return The same as the return type of func */ template auto -retryOnTimeout(FnType func, size_t waitMs = kDefaultWaitBetweenRetry) +retryOnTimeout( + FnType func, + boost::asio::yield_context yield, + util::Retry::Delays delays = kDefaultRetryDelays +) { static util::Logger const log{"Backend"}; // NOLINT(readability-identifier-naming) + auto retry = util::makeRetryExponentialBackoff(delays, yield.get_executor()); + while (true) { try { return func(); - } catch (DatabaseTimeout const&) { - LOG(log.error()) << "Database request timed out. Sleeping and retrying ... "; - std::this_thread::sleep_for(std::chrono::milliseconds(waitMs)); + } catch (DatabaseError const& e) { + auto const delayMs = + std::chrono::duration_cast(retry.delayValue()).count(); + LOG(log.error()) << e.what() << " (attempt " << retry.attemptNumber() + 1 + << "). Retrying in " << delayMs << "ms ..."; + + retry.wait(yield); + } + } +} + +/** + * @brief Retry `func` while it throws DatabaseError, blocking the calling thread in between. + * + * @warning Blocks the calling thread; from a coroutine use the `yield_context` overload instead. + * + * @tparam FnType The type of function object to execute + * @param func The function object to execute + * @param delays The delays to use between attempts + * @return The same as the return type of func + */ +template +auto +retryOnTimeout(FnType func, util::Retry::Delays delays = kDefaultRetryDelays) +{ + static util::Logger const log{"Backend"}; // NOLINT(readability-identifier-naming) + + util::ExponentialBackoffStrategy backoff{delays}; + std::size_t attempt = 1; + + while (true) { + try { + return func(); + } catch (DatabaseError const& e) { + auto const delay = backoff.getDelay(); + LOG(log.error()) << e.what() << " (attempt " << attempt << "). Retrying in " + << std::chrono::duration_cast(delay).count() + << "ms ..."; + + ++attempt; + + std::this_thread::sleep_for(delay); + backoff.increaseDelay(); } } } @@ -105,18 +183,21 @@ synchronous(FnType&& func) } /** - * @brief Synchronously execute the given function object and retry until no DatabaseTimeout is + * @brief Synchronously execute the given function object and retry until no DatabaseError is * thrown. * + * @warning Blocks the calling thread while backing off. + * * @tparam FnType The type of function object to execute * @param func The function object to execute + * @param delays The delays to use between attempts * @return The same as the return type of func */ template auto -synchronousAndRetryOnTimeout(FnType&& func) +synchronousAndRetryOnTimeout(FnType&& func, util::Retry::Delays delays = kDefaultRetryDelays) { - return retryOnTimeout([&]() { return synchronous(func); }); + return retryOnTimeout([&]() { return synchronous(func); }, delays); } /** @@ -139,8 +220,27 @@ public: BackendInterface(LedgerCacheInterface& cache) : cache_{cache} { } + virtual ~BackendInterface() = default; + /** + * @return Delay before the first retry of a request against this backend + */ + [[nodiscard]] virtual std::chrono::milliseconds + initialRetryDelay() const + { + return kDefaultWaitBetweenRetry; + } + + /** + * @return Upper bound for the retry backoff; equal to @ref initialRetryDelay() means flat + */ + [[nodiscard]] virtual std::chrono::milliseconds + maxRetryDelay() const + { + return kMaxWaitBetweenRetry; + } + // TODO https://github.com/XRPLF/clio/issues/1956: Remove this hack once old ETL is removed. // Cache should not be exposed thru BackendInterface @@ -705,7 +805,7 @@ public: hardFetchLedgerRange(boost::asio::yield_context yield) const = 0; /** - * @brief Fetches the ledger range from DB retrying until no DatabaseTimeout is thrown. + * @brief Fetches the ledger range from DB retrying until no DatabaseError is thrown. * * @return The ledger range if available; nullopt otherwise */ diff --git a/src/data/cassandra/CassandraBackendFamily.hpp b/src/data/cassandra/CassandraBackendFamily.hpp index 3d0aa01f3..953daedff 100644 --- a/src/data/cassandra/CassandraBackendFamily.hpp +++ b/src/data/cassandra/CassandraBackendFamily.hpp @@ -139,6 +139,24 @@ public: */ CassandraBackendFamily(CassandraBackendFamily&&) = delete; + /** + * @return The configured delay before the first retry + */ + [[nodiscard]] std::chrono::milliseconds + initialRetryDelay() const override + { + return settingsProvider_.getInitialRetryDelay(); + } + + /** + * @return The configured upper bound for the retry backoff + */ + [[nodiscard]] std::chrono::milliseconds + maxRetryDelay() const override + { + return settingsProvider_.getMaxRetryDelay(); + } + TransactionsAndCursor fetchAccountTransactions( xrpl::AccountID const& account, diff --git a/src/data/cassandra/Error.hpp b/src/data/cassandra/Error.hpp index 88709ef47..58033ae20 100644 --- a/src/data/cassandra/Error.hpp +++ b/src/data/cassandra/Error.hpp @@ -80,17 +80,6 @@ public: return code_; } - /** - * @return true if the wrapped error is considered a timeout; false otherwise - */ - [[nodiscard]] bool - isTimeout() const - { - return code_ == CASS_ERROR_LIB_NO_HOSTS_AVAILABLE or - code_ == CASS_ERROR_LIB_REQUEST_TIMED_OUT or code_ == CASS_ERROR_SERVER_UNAVAILABLE or - code_ == CASS_ERROR_SERVER_OVERLOADED or code_ == CASS_ERROR_SERVER_READ_TIMEOUT; - } - /** * @return true if the wrapped error is an invalid query; false otherwise */ diff --git a/src/data/cassandra/SettingsProvider.cpp b/src/data/cassandra/SettingsProvider.cpp index d0f57eaa3..1284c8e24 100644 --- a/src/data/cassandra/SettingsProvider.cpp +++ b/src/data/cassandra/SettingsProvider.cpp @@ -3,6 +3,7 @@ #include "data/cassandra/Types.hpp" #include "data/cassandra/impl/Cluster.hpp" #include "util/Constants.hpp" +#include "util/config/ConfigDefinition.hpp" #include "util/config/ObjectView.hpp" #include @@ -25,6 +26,12 @@ SettingsProvider::SettingsProvider(util::config::ObjectView const& cfg) , keyspace_{cfg.get("keyspace")} , tablePrefix_{cfg.maybeValue("table_prefix")} , replicationFactor_{cfg.get("replication_factor")} + , initialRetryDelay_{util::config::ClioConfigDefinition::toMilliseconds( + cfg.get("initial_request_retry_delay") + )} + , maxRetryDelay_{util::config::ClioConfigDefinition::toMilliseconds( + cfg.get("max_request_retry_delay") + )} , settings_{parseSettings()} { } @@ -94,9 +101,9 @@ SettingsProvider::parseSettings() const } if (config_.getValueView("request_timeout").hasValue()) { - auto const requestTimeoutSecond = config_.get("request_timeout"); - settings.requestTimeout = - std::chrono::milliseconds{requestTimeoutSecond * util::kMillisecondsPerSecond}; + settings.requestTimeout = util::config::ClioConfigDefinition::toMilliseconds( + config_.get("request_timeout") + ); } settings.certificate = parseOptionalCertificate(); diff --git a/src/data/cassandra/SettingsProvider.hpp b/src/data/cassandra/SettingsProvider.hpp index 631f0401e..34625280e 100644 --- a/src/data/cassandra/SettingsProvider.hpp +++ b/src/data/cassandra/SettingsProvider.hpp @@ -4,6 +4,7 @@ #include "data/cassandra/impl/Cluster.hpp" #include "util/config/ObjectView.hpp" +#include #include #include #include @@ -19,6 +20,8 @@ class SettingsProvider { std::string keyspace_; std::optional tablePrefix_; uint16_t replicationFactor_; + std::chrono::milliseconds initialRetryDelay_; + std::chrono::milliseconds maxRetryDelay_; Settings settings_; public: @@ -62,6 +65,24 @@ public: return replicationFactor_; } + /** + * @return Delay before the first retry of a failed request + */ + [[nodiscard]] std::chrono::milliseconds + getInitialRetryDelay() const + { + return initialRetryDelay_; + } + + /** + * @return Upper bound for the retry backoff + */ + [[nodiscard]] std::chrono::milliseconds + getMaxRetryDelay() const + { + return maxRetryDelay_; + } + private: [[nodiscard]] std::optional parseOptionalCertificate() const; diff --git a/src/data/cassandra/impl/ExecutionStrategy.hpp b/src/data/cassandra/impl/ExecutionStrategy.hpp index e8907a5f9..6496fcb5e 100644 --- a/src/data/cassandra/impl/ExecutionStrategy.hpp +++ b/src/data/cassandra/impl/ExecutionStrategy.hpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -27,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -169,7 +171,7 @@ public: * * @param preparedStatement Statement to prepare and execute * @param args Args to bind to the prepared statement - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error */ template void @@ -185,7 +187,7 @@ public: * Retries forever with retry policy specified by @ref AsyncExecutor * * @param statement Statement to execute - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error */ void write(StatementType&& statement) @@ -215,7 +217,7 @@ public: * Retries forever with retry policy specified by @ref AsyncExecutor. * * @param statements Vector of statements to execute as a batch - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error */ void write(std::vector&& statements) @@ -254,7 +256,7 @@ public: * Retries forever with retry policy specified by @ref AsyncExecutor. * * @param statements Vector of statements to execute - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error */ void writeEach(std::vector&& statements) @@ -272,7 +274,7 @@ public: * @param token Completion token (yield_context) * @param preparedStatement Statement to prepare and execute * @param args Args to bind to the prepared statement - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error * @return ResultType or error wrapped in Expected */ template @@ -289,7 +291,7 @@ public: * * @param token Completion token (yield_context) * @param statements Statements to execute in a batch - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error * @return ResultType or error wrapped in Expected */ [[maybe_unused]] ResultOrErrorType @@ -346,7 +348,7 @@ public: * * @param token Completion token (yield_context) * @param statement Statement to execute - * @throws DatabaseTimeout on timeout + * @throws DatabaseError on a database error * @return ResultType or error wrapped in Expected */ [[maybe_unused]] ResultOrErrorType @@ -402,7 +404,7 @@ public: * * @param token Completion token (yield_context) * @param statements Statements to execute - * @throws DatabaseTimeout on db error + * @throws DatabaseError on a database error * @return Vector of results */ std::vector @@ -457,7 +459,7 @@ public: ); counters_->registerReadError(errorsCount); counters_->registerReadFinished(startTime, statements.size() - errorsCount); - throw DatabaseTimeout{}; + throw DatabaseError{}; } counters_->registerReadFinished(startTime, statements.size()); @@ -551,11 +553,13 @@ private: void throwErrorIfNeeded(CassandraError err) const { - if (err.isTimeout()) - throw DatabaseTimeout(); - + // NOTE: etl::impl::Loader and etl::impl::Extractor treat std::runtime_error as + // "amendment blocked", so only genuinely permanent failures may be thrown as one. if (err.isInvalidQuery()) throw std::runtime_error("Invalid query"); + + // anything else, including unclassified codes, is transient and gets retried + throw DatabaseError{fmt::format("Database error [{}]: {}", err.code(), err.message())}; } }; diff --git a/src/data/cassandra/impl/RetryPolicy.hpp b/src/data/cassandra/impl/RetryPolicy.hpp index 50b664772..6d31581da 100644 --- a/src/data/cassandra/impl/RetryPolicy.hpp +++ b/src/data/cassandra/impl/RetryPolicy.hpp @@ -28,8 +28,7 @@ public: ExponentialBackoffRetryPolicy(boost::asio::io_context& ioc) : retry_( util::makeRetryExponentialBackoff( - std::chrono::milliseconds(1), - std::chrono::seconds(1), + {.initial = std::chrono::milliseconds(1), .max = std::chrono::seconds(1)}, boost::asio::make_strand(ioc) ) ) diff --git a/src/etl/impl/CacheLoader.hpp b/src/etl/impl/CacheLoader.hpp index de36ac570..1db69775d 100644 --- a/src/etl/impl/CacheLoader.hpp +++ b/src/etl/impl/CacheLoader.hpp @@ -145,9 +145,17 @@ private: LOG(log_.debug()) << "Starting a cursor: " << xrpl::strHex(start); while (not token.isStopRequested() and not cache_.get().isDisabled()) { - auto res = data::retryOnTimeout([this, seq, cachePageFetchSize, &start, token]() { - return backend_->fetchLedgerPage(start, seq, cachePageFetchSize, false, token); - }); + auto res = data::retryOnTimeout( + [this, seq, cachePageFetchSize, &start, token]() { + return backend_->fetchLedgerPage( + start, seq, cachePageFetchSize, false, token + ); + }, + token, + util::Retry::Delays{ + .initial = backend_->initialRetryDelay(), .max = backend_->maxRetryDelay() + } + ); cache_.get().update(res.objects, seq, true); diff --git a/src/etl/impl/SubscriptionSource.cpp b/src/etl/impl/SubscriptionSource.cpp index 833d3ac04..56fc3be6b 100644 --- a/src/etl/impl/SubscriptionSource.cpp +++ b/src/etl/impl/SubscriptionSource.cpp @@ -58,7 +58,9 @@ SubscriptionSource::SubscriptionSource( , subscriptions_(std::move(subscriptions)) , strand_(boost::asio::make_strand(ioContext)) , wsTimeout_(wsTimeout) - , retry_(util::makeRetryExponentialBackoff(retryDelay, kRetryMaxDelay, strand_)) + , retry_( + util::makeRetryExponentialBackoff({.initial = retryDelay, .max = kMaxRetryDelay}, strand_) + ) , onConnect_(std::move(onConnect)) , onDisconnect_(std::move(onDisconnect)) , onLedgerClosed_(std::move(onLedgerClosed)) diff --git a/src/etl/impl/SubscriptionSource.hpp b/src/etl/impl/SubscriptionSource.hpp index 1e35181fe..08cea70f0 100644 --- a/src/etl/impl/SubscriptionSource.hpp +++ b/src/etl/impl/SubscriptionSource.hpp @@ -75,7 +75,7 @@ private: util::StopHelper stopHelper_; static constexpr std::chrono::seconds kWsTimeout{30}; - static constexpr std::chrono::seconds kRetryMaxDelay{30}; + static constexpr std::chrono::seconds kMaxRetryDelay{30}; static constexpr std::chrono::seconds kRetryDelay{1}; public: diff --git a/src/rpc/Errors.hpp b/src/rpc/Errors.hpp index 8f69d8157..5b06c365a 100644 --- a/src/rpc/Errors.hpp +++ b/src/rpc/Errors.hpp @@ -239,7 +239,7 @@ public: * @return The error message */ [[nodiscard]] char const* - what() const throw() override + what() const noexcept override { return msg_.c_str(); } @@ -267,7 +267,7 @@ public: * @return The error message */ [[nodiscard]] char const* - what() const throw() override + what() const noexcept override { return account_.c_str(); } diff --git a/src/rpc/RPCEngine.hpp b/src/rpc/RPCEngine.hpp index c6380ba9f..a44f33707 100644 --- a/src/rpc/RPCEngine.hpp +++ b/src/rpc/RPCEngine.hpp @@ -178,8 +178,8 @@ public: } return Result{std::move(v)}; - } catch (data::DatabaseTimeout const& t) { - LOG(log_.error()) << "Database timeout"; + } catch (data::DatabaseError const& t) { + LOG(log_.error()) << "Database error: " << t.what(); notifyTooBusy(); return Result{Status{RippledError::RpcTooBusy}}; @@ -361,8 +361,8 @@ private: } return Result{std::move(v)}; - } catch (data::DatabaseTimeout const& t) { - LOG(log_.error()) << "Database timeout"; + } catch (data::DatabaseError const& t) { + LOG(log_.error()) << "Database error: " << t.what(); notifyTooBusy(); return Result{Status{RippledError::RpcTooBusy}}; diff --git a/src/util/Retry.cpp b/src/util/Retry.cpp index 29d2fdba8..d45a1f2d2 100644 --- a/src/util/Retry.cpp +++ b/src/util/Retry.cpp @@ -1,6 +1,8 @@ #include "util/Retry.hpp" +#include #include +#include #include #include @@ -42,6 +44,11 @@ Retry::Retry( { } +Retry::Retry(RetryStrategyPtr strategy, boost::asio::any_io_executor executor) + : strategy_(std::move(strategy)), timer_(executor) +{ +} + Retry::~Retry() { *canceled_ = true; @@ -73,11 +80,8 @@ Retry::reset() (*strategy_).reset(); } -ExponentialBackoffStrategy::ExponentialBackoffStrategy( - std::chrono::steady_clock::duration delay, - std::chrono::steady_clock::duration maxDelay -) - : RetryStrategy(delay), maxDelay_(maxDelay) +ExponentialBackoffStrategy::ExponentialBackoffStrategy(Retry::Delays delays) + : RetryStrategy(delays.initial), maxDelay_(delays.max) { } @@ -88,14 +92,32 @@ ExponentialBackoffStrategy::nextDelay() const return std::min(next, maxDelay_); } +void +Retry::wait(boost::asio::yield_context yield) +{ + *canceled_ = false; + timer_.expires_after(strategy_->getDelay()); + strategy_->increaseDelay(); + ++attemptNumber_; + + // error ignored on purpose: a cancelled timer just means the caller retries sooner + boost::system::error_code ec; + timer_.async_wait(yield[ec]); +} + Retry makeRetryExponentialBackoff( - std::chrono::steady_clock::duration delay, - std::chrono::steady_clock::duration maxDelay, + Retry::Delays delays, boost::asio::strand strand ) { - return Retry(std::make_unique(delay, maxDelay), std::move(strand)); + return Retry(std::make_unique(delays), std::move(strand)); +} + +Retry +makeRetryExponentialBackoff(Retry::Delays delays, boost::asio::any_io_executor executor) +{ + return Retry(std::make_unique(delays), std::move(executor)); } } // namespace util diff --git a/src/util/Retry.hpp b/src/util/Retry.hpp index 3064423e2..b58ec06b2 100644 --- a/src/util/Retry.hpp +++ b/src/util/Retry.hpp @@ -1,7 +1,9 @@ #pragma once +#include #include #include +#include #include #include @@ -65,6 +67,16 @@ class Retry { std::shared_ptr canceled_{std::make_shared(false)}; public: + /** + * @brief The delays to use between retry attempts + * + * Equal `initial` and `max` mean a flat delay with no backoff. + */ + struct Delays { + std::chrono::steady_clock::duration initial; + std::chrono::steady_clock::duration max; + }; + /** * @brief Construct a new Retry object * @@ -76,6 +88,16 @@ public: boost::asio::strand strand ); + /** + * @brief Construct a new Retry object from any I/O executor + * + * For coroutines, pass `yield.get_executor()` and drive it with @ref wait(). + * + * @param strategy The retry strategy to use + * @param executor The executor to run the retry timer on + */ + Retry(RetryStrategyPtr strategy, boost::asio::any_io_executor executor); + /** * @brief Destroy the Retry object */ @@ -105,6 +127,17 @@ public: }); } + /** + * @brief Wait out the current delay by suspending the calling coroutine, then back off. + * + * Unlike @ref retry() this returns once the delay elapsed instead of scheduling a callback, so + * the caller can keep its own loop. Advances the delay and attempt number like @ref retry(). + * + * @param yield The coroutine to suspend + */ + void + wait(boost::asio::yield_context yield); + /** * @brief Cancel scheduled retry if any */ @@ -140,13 +173,9 @@ public: /** * @brief Construct a new Exponential Backoff Strategy object * - * @param delay The initial delay value - * @param maxDelay The maximum delay value + * @param delays The delays to use between attempts */ - ExponentialBackoffStrategy( - std::chrono::steady_clock::duration delay, - std::chrono::steady_clock::duration maxDelay - ); + explicit ExponentialBackoffStrategy(Retry::Delays delays); private: [[nodiscard]] std::chrono::steady_clock::duration @@ -156,16 +185,24 @@ private: /** * @brief Create a retry mechanism with exponential backoff strategy * - * @param delay The initial delay value - * @param maxDelay The maximum delay value + * @param delays The delays to use between attempts * @param strand The strand to use for async operations * @return The retry object */ Retry makeRetryExponentialBackoff( - std::chrono::steady_clock::duration delay, - std::chrono::steady_clock::duration maxDelay, + Retry::Delays delays, boost::asio::strand strand ); +/** + * @brief Create a retry mechanism with exponential backoff strategy on any I/O executor + * + * @param delays The delays to use between attempts + * @param executor The executor to run the retry timer on + * @return The retry object + */ +Retry +makeRetryExponentialBackoff(Retry::Delays delays, boost::asio::any_io_executor executor); + } // namespace util diff --git a/src/util/config/ConfigDefinition.cpp b/src/util/config/ConfigDefinition.cpp index 7166067fe..0fef3d0a0 100644 --- a/src/util/config/ConfigDefinition.cpp +++ b/src/util/config/ConfigDefinition.cpp @@ -278,7 +278,15 @@ getClioConfig() {"database.cassandra.connect_timeout", ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateUint32)}, {"database.cassandra.request_timeout", - ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateUint32)}, + ConfigValue{ConfigType::Double}.optional().withConstraint(gValidatePositiveDouble)}, + {"database.cassandra.initial_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(0.5).withConstraint( + gValidatePositiveDouble + )}, + {"database.cassandra.max_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(5.0).withConstraint( + gValidatePositiveDouble + )}, {"database.cassandra.username", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.password", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.certfile", ConfigValue{ConfigType::String}.optional()}, diff --git a/src/util/config/ConfigDescription.hpp b/src/util/config/ConfigDescription.hpp index b0fee6c03..eb6fcd666 100644 --- a/src/util/config/ConfigDescription.hpp +++ b/src/util/config/ConfigDescription.hpp @@ -182,8 +182,14 @@ This document provides a list of all available Clio configuration properties in "established."}, KV{.key = "database.cassandra.request_timeout", .value = "The maximum amount of time in seconds that the system waits for a request to " - "be fetched from the " - "database."}, + "be fetched from the database. Should be set higher than the server side read " + "timeout. If omitted, no request timeout is applied."}, + KV{.key = "database.cassandra.initial_request_retry_delay", + .value = "How long in seconds to wait before the first retry of a database request " + "that failed with a transient error."}, + KV{.key = "database.cassandra.max_request_retry_delay", + .value = "Upper bound in seconds for the exponential backoff between retries of a " + "database request."}, KV{.key = "database.cassandra.username", .value = "The username used for authenticating with the database."}, KV{.key = "database.cassandra.password", diff --git a/tests/integration/data/BackendFactoryTests.cpp b/tests/integration/data/BackendFactoryTests.cpp index 6dd6cc875..cadb5f82b 100644 --- a/tests/integration/data/BackendFactoryTests.cpp +++ b/tests/integration/data/BackendFactoryTests.cpp @@ -52,7 +52,11 @@ protected: {"database.cassandra.write_batch_size", ConfigValue{ConfigType::Integer}.defaultValue(20)}, {"database.cassandra.connect_timeout", ConfigValue{ConfigType::Integer}.defaultValue(1).optional()}, - {"database.cassandra.request_timeout", ConfigValue{ConfigType::Integer}.optional()}, + {"database.cassandra.initial_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(0.5)}, + {"database.cassandra.max_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(5.0)}, + {"database.cassandra.request_timeout", ConfigValue{ConfigType::Double}.optional()}, {"database.cassandra.username", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.password", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.certfile", ConfigValue{ConfigType::String}.optional()}, diff --git a/tests/integration/data/cassandra/BackendTests.cpp b/tests/integration/data/cassandra/BackendTests.cpp index 3a925e714..d31b160da 100644 --- a/tests/integration/data/cassandra/BackendTests.cpp +++ b/tests/integration/data/cassandra/BackendTests.cpp @@ -96,8 +96,12 @@ protected: {"database.cassandra.write_batch_size", ConfigValue{ConfigType::Integer}.defaultValue(20)}, {"database.cassandra.connect_timeout", ConfigValue{ConfigType::Integer}.defaultValue(10).optional()}, + {"database.cassandra.initial_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(0.5)}, + {"database.cassandra.max_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(5.0)}, {"database.cassandra.request_timeout", - ConfigValue{ConfigType::Integer}.defaultValue(10).optional()}, + ConfigValue{ConfigType::Double}.defaultValue(10.0).optional()}, {"database.cassandra.username", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.password", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.certfile", ConfigValue{ConfigType::String}.optional()}, diff --git a/tests/integration/migration/cassandra/CassandraMigrationManagerTests.cpp b/tests/integration/migration/cassandra/CassandraMigrationManagerTests.cpp index 9aa8ad5f3..fbb25a8d4 100644 --- a/tests/integration/migration/cassandra/CassandraMigrationManagerTests.cpp +++ b/tests/integration/migration/cassandra/CassandraMigrationManagerTests.cpp @@ -117,8 +117,12 @@ protected: ConfigValue{ConfigType::Integer}.defaultValue(20).withConstraint(gValidateUint16)}, {"database.cassandra.connect_timeout", ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateUint32)}, + {"database.cassandra.initial_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(0.5)}, + {"database.cassandra.max_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(5.0)}, {"database.cassandra.request_timeout", - ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateUint32)}, + ConfigValue{ConfigType::Double}.optional().withConstraint(gValidatePositiveDouble)}, {"database.cassandra.username", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.password", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.certfile", ConfigValue{ConfigType::String}.optional()}, diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index dc805b193..e67d37663 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -15,6 +15,7 @@ target_sources( data/LedgerCacheLoadingStateTests.cpp data/LedgerCacheSaverTests.cpp data/cassandra/AsyncExecutorTests.cpp + data/cassandra/ErrorTests.cpp data/cassandra/ExecutionStrategyTests.cpp data/cassandra/LedgerHeaderCacheTests.cpp data/cassandra/RetryPolicyTests.cpp diff --git a/tests/unit/data/BackendInterfaceTests.cpp b/tests/unit/data/BackendInterfaceTests.cpp index 19e3267a6..c69dd7b18 100644 --- a/tests/unit/data/BackendInterfaceTests.cpp +++ b/tests/unit/data/BackendInterfaceTests.cpp @@ -1,10 +1,13 @@ +#include "data/BackendInterface.hpp" #include "etl/CorruptionDetector.hpp" #include "etl/SystemState.hpp" #include "util/AsioContextTestFixture.hpp" #include "util/MockBackendTestFixture.hpp" #include "util/MockPrometheus.hpp" +#include "util/Retry.hpp" #include "util/TestObject.hpp" +#include #include #include #include @@ -12,7 +15,12 @@ #include #include +#include +#include +#include #include +#include +#include #include using namespace data; @@ -160,3 +168,122 @@ TEST_F( }); EXPECT_FALSE(backend_->cache().isDisabled()); } + +// Loader and Extractor catch std::runtime_error to decide the server must amendment-block, so +// DatabaseError has to stay outside that hierarchy. +TEST(BackendInterfaceRetryTest, DatabaseErrorIsNotARuntimeError) +{ + static_assert(std::is_base_of_v); + static_assert(not std::is_base_of_v); + + try { + throw DatabaseError{"transient"}; + } catch (std::runtime_error const&) { + FAIL() << "DatabaseError must not be caught as std::runtime_error - doing so would " + "amendment-block the server on a transient database error"; + } catch (std::exception const& e) { + EXPECT_STREQ(e.what(), "transient"); + } +} + +TEST(BackendInterfaceRetryTest, DatabaseErrorKeepsDefaultMessage) +{ + EXPECT_STREQ(DatabaseError{}.what(), "Transient database error. Please retry the request"); +} + +TEST(BackendInterfaceRetryTest, RetryOnTimeoutBlockingRetriesUntilSuccess) +{ + std::size_t calls = 0; + + auto const result = retryOnTimeout( + [&calls]() -> int { + if (++calls < 3) + throw DatabaseError{}; + return 42; + }, + util::Retry::Delays{ + .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{2} + } + ); + + EXPECT_EQ(result, 42); + EXPECT_EQ(calls, 3); +} + +TEST(BackendInterfaceRetryTest, RetryOnTimeoutBlockingDoesNotSwallowOtherExceptions) +{ + EXPECT_THROW( + retryOnTimeout( + []() -> int { throw std::runtime_error{"permanent"}; }, + util::Retry::Delays{ + .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{1} + } + ), + std::runtime_error + ); +} + +struct BackendInterfaceRetryCoroTest : SyncAsioContextTest {}; + +TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroRetriesUntilSuccess) +{ + std::size_t calls = 0; + + runSpawn([&calls](auto yield) { + auto const result = retryOnTimeout( + [&calls]() -> int { + if (++calls < 3) + throw DatabaseError{}; + return 42; + }, + yield, + util::Retry::Delays{ + .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{2} + } + ); + + EXPECT_EQ(result, 42); + }); + + EXPECT_EQ(calls, 3); +} + +TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotSwallowOtherExceptions) +{ + runSpawn([](auto yield) { + EXPECT_THROW( + retryOnTimeout( + []() -> int { throw std::runtime_error{"permanent"}; }, + yield, + util::Retry::Delays{ + .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{1} + } + ), + std::runtime_error + ); + }); +} + +TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotBlockItsThread) +{ + bool ran = false; + + runSpawn([&ran, this](auto yield) { + boost::asio::post(ctx_, [&ran]() { ran = true; }); + + std::size_t calls = 0; + retryOnTimeout( + [&calls]() -> int { + if (++calls < 2) + throw DatabaseError{}; + return 0; + }, + yield, + util::Retry::Delays{ + .initial = std::chrono::milliseconds{20}, .max = std::chrono::milliseconds{20} + } + ); + + EXPECT_TRUE(ran); + }); +} diff --git a/tests/unit/data/cassandra/ErrorTests.cpp b/tests/unit/data/cassandra/ErrorTests.cpp new file mode 100644 index 000000000..5739a156e --- /dev/null +++ b/tests/unit/data/cassandra/ErrorTests.cpp @@ -0,0 +1,43 @@ +#include "data/cassandra/Error.hpp" + +#include +#include + +#include + +using namespace data::cassandra; + +namespace { + +CassandraError +makeError(uint32_t const code) +{ + return CassandraError{"some error", code}; +} + +} // namespace + +// isInvalidQuery is load bearing: DefaultExecutionStrategy::throwErrorIfNeeded treats it as the +// only permanent failure and retries everything else, so anything wrongly reported here would be +// retried forever (if false) or surfaced as a fatal std::runtime_error (if true). +TEST(BackendCassandraErrorTest, IsInvalidQueryOnlyForInvalidQuery) +{ + EXPECT_TRUE(makeError(CASS_ERROR_SERVER_INVALID_QUERY).isInvalidQuery()); + + EXPECT_FALSE(makeError(CASS_ERROR_SERVER_READ_FAILURE).isInvalidQuery()); + EXPECT_FALSE(makeError(CASS_ERROR_SERVER_WRITE_FAILURE).isInvalidQuery()); + EXPECT_FALSE(makeError(CASS_ERROR_SERVER_READ_TIMEOUT).isInvalidQuery()); + EXPECT_FALSE(makeError(CASS_ERROR_SERVER_UNAVAILABLE).isInvalidQuery()); + EXPECT_FALSE(makeError(CASS_ERROR_SERVER_SYNTAX_ERROR).isInvalidQuery()); + EXPECT_FALSE(makeError(CASS_OK).isInvalidQuery()); +} + +TEST(BackendCassandraErrorTest, MessageAndCodeArePreserved) +{ + // throwErrorIfNeeded puts message() into the DatabaseError it throws, so that treating an + // unclassified error as a timeout still reports what actually failed + auto const err = CassandraError{"received 1 responses and 1 failures", 0x1300}; + + EXPECT_EQ(err.message(), "received 1 responses and 1 failures"); + EXPECT_EQ(err.code(), 0x1300u); +} diff --git a/tests/unit/data/cassandra/ExecutionStrategyTests.cpp b/tests/unit/data/cassandra/ExecutionStrategyTests.cpp index 7a8efc633..a16cab462 100644 --- a/tests/unit/data/cassandra/ExecutionStrategyTests.cpp +++ b/tests/unit/data/cassandra/ExecutionStrategyTests.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -154,7 +155,45 @@ TEST_F(BackendCassandraExecutionStrategyTest, ReadOneInCoroutineThrowsOnTimeoutF runSpawn([&strat](boost::asio::yield_context yield) { auto statement = FakeStatement{}; - EXPECT_THROW(strat.read(yield, statement), data::DatabaseTimeout); + EXPECT_THROW(strat.read(yield, statement), data::DatabaseError); + }); +} + +// A CL=QUORUM read failure is CASS_ERROR_SERVER_READ_FAILURE, which used to not throw at all, +// leaving read() spinning; Times(1) pins that down. +TEST_F(BackendCassandraExecutionStrategyTest, ReadOneInCoroutineThrowsOnQuorumReadFailure) +{ + auto strat = makeStrategy(); + + ON_CALL( + handle_, + asyncExecute(A(), A&&>()) + ) + .WillByDefault([](auto const&, auto&& cb) { + auto res = FakeResultOrError{CassandraError{ + "received 1 responses and 1 failures", CASS_ERROR_SERVER_READ_FAILURE + }}; + cb(res); // notify that item is ready + return FakeFutureWithCallback{res}; + }); + EXPECT_CALL( + handle_, + asyncExecute(A(), A&&>()) + ) + .Times(1); + EXPECT_CALL(*counters_, registerReadStartedImpl(1)); + EXPECT_CALL(*counters_, registerReadErrorImpl(1)); + + runSpawn([&strat](boost::asio::yield_context yield) { + auto statement = FakeStatement{}; + try { + strat.read(yield, statement); + FAIL() << "expected DatabaseError"; + } catch (data::DatabaseError const& e) { + EXPECT_THAT( + std::string{e.what()}, testing::HasSubstr("received 1 responses and 1 failures") + ); + } }); } @@ -246,7 +285,7 @@ TEST_F(BackendCassandraExecutionStrategyTest, ReadBatchInCoroutineThrowsOnTimeou runSpawn([&strat](boost::asio::yield_context yield) { auto statements = std::vector(kNumStatements); - EXPECT_THROW(strat.read(yield, statements), data::DatabaseTimeout); + EXPECT_THROW(strat.read(yield, statements), data::DatabaseError); }); } @@ -384,7 +423,7 @@ TEST_F(BackendCassandraExecutionStrategyTest, ReadEachInCoroutineThrowsOnFailure runSpawn([&strat](boost::asio::yield_context yield) { auto statements = std::vector(kNumStatements); - EXPECT_THROW(strat.readEach(yield, statements), data::DatabaseTimeout); + EXPECT_THROW(strat.readEach(yield, statements), data::DatabaseError); }); } diff --git a/tests/unit/data/cassandra/SettingsProviderTests.cpp b/tests/unit/data/cassandra/SettingsProviderTests.cpp index 51919611b..ea27d6401 100644 --- a/tests/unit/data/cassandra/SettingsProviderTests.cpp +++ b/tests/unit/data/cassandra/SettingsProviderTests.cpp @@ -47,7 +47,11 @@ getParseSettingsConfig(boost::json::value val) {"database.cassandra.write_batch_size", ConfigValue{ConfigType::Integer}.defaultValue(20)}, {"database.cassandra.connect_timeout", ConfigValue{ConfigType::Integer}.optional()}, {"database.cassandra.certfile", ConfigValue{ConfigType::String}.optional()}, - {"database.cassandra.request_timeout", ConfigValue{ConfigType::Integer}.defaultValue(0)}, + {"database.cassandra.initial_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(0.5)}, + {"database.cassandra.max_request_retry_delay", + ConfigValue{ConfigType::Double}.defaultValue(5.0)}, + {"database.cassandra.request_timeout", ConfigValue{ConfigType::Double}.optional()}, {"database.cassandra.secure_connect_bundle", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.username", ConfigValue{ConfigType::String}.optional()}, {"database.cassandra.password", ConfigValue{ConfigType::String}.optional()}, @@ -163,3 +167,72 @@ TEST_F(SettingsProviderTest, CertificateConfig) auto const settings = provider.getSettings(); EXPECT_EQ(settings.certificate, "certificateData"); } + +TEST_F(SettingsProviderTest, RequestTimeoutAcceptsFractionalSeconds) +{ + auto const cfg = getParseSettingsConfig( + boost::json::parse( + R"JSON({"database.cassandra.contact_points": "127.0.0.1", + "database.cassandra.request_timeout": 2.5})JSON" + ) + ); + SettingsProvider const provider{cfg.getObject("database.cassandra")}; + + EXPECT_EQ(provider.getSettings().requestTimeout, std::chrono::milliseconds{2500}); +} + +TEST_F(SettingsProviderTest, RequestTimeoutStillAcceptsWholeSeconds) +{ + // a JSON integer must still be accepted + auto const cfg = getParseSettingsConfig( + boost::json::parse( + R"JSON({"database.cassandra.contact_points": "127.0.0.1", + "database.cassandra.request_timeout": 3})JSON" + ) + ); + SettingsProvider const provider{cfg.getObject("database.cassandra")}; + + EXPECT_EQ(provider.getSettings().requestTimeout, std::chrono::milliseconds{3000}); +} + +TEST_F(SettingsProviderTest, RetryDelaysDefaults) +{ + auto const cfg = getParseSettingsConfig( + boost::json::parse(R"JSON({"database.cassandra.contact_points": "127.0.0.1"})JSON") + ); + SettingsProvider const provider{cfg.getObject("database.cassandra")}; + + EXPECT_EQ(provider.getInitialRetryDelay(), std::chrono::milliseconds{500}); + EXPECT_EQ(provider.getMaxRetryDelay(), std::chrono::milliseconds{5000}); +} + +TEST_F(SettingsProviderTest, RetryDelaysAreIndependentOfRequestTimeout) +{ + auto const cfg = getParseSettingsConfig( + boost::json::parse( + R"JSON({"database.cassandra.contact_points": "127.0.0.1", + "database.cassandra.request_timeout": 2.5, + "database.cassandra.initial_request_retry_delay": 0.25, + "database.cassandra.max_request_retry_delay": 1.5})JSON" + ) + ); + SettingsProvider const provider{cfg.getObject("database.cassandra")}; + + EXPECT_EQ(provider.getSettings().requestTimeout, std::chrono::milliseconds{2500}); + EXPECT_EQ(provider.getInitialRetryDelay(), std::chrono::milliseconds{250}); + EXPECT_EQ(provider.getMaxRetryDelay(), std::chrono::milliseconds{1500}); +} + +TEST_F(SettingsProviderTest, EqualRetryDelaysDisableBackoff) +{ + auto const cfg = getParseSettingsConfig( + boost::json::parse( + R"JSON({"database.cassandra.contact_points": "127.0.0.1", + "database.cassandra.initial_request_retry_delay": 0.5, + "database.cassandra.max_request_retry_delay": 0.5})JSON" + ) + ); + SettingsProvider const provider{cfg.getObject("database.cassandra")}; + + EXPECT_EQ(provider.getInitialRetryDelay(), provider.getMaxRetryDelay()); +} diff --git a/tests/unit/rpc/RPCEngineTests.cpp b/tests/unit/rpc/RPCEngineTests.cpp index 57ffaa0d8..48820e48b 100644 --- a/tests/unit/rpc/RPCEngineTests.cpp +++ b/tests/unit/rpc/RPCEngineTests.cpp @@ -262,7 +262,7 @@ TEST_F(RPCEngineTest, ThrowDatabaseError) EXPECT_CALL(*backend_, isTooBusy).WillOnce(Return(false)); EXPECT_CALL(*handlerProvider, getHandler(method)) .WillOnce(Return(AnyHandler{tests::common::FailingHandlerFake{}})); - EXPECT_CALL(*mockCountersPtr_, rpcErrored(method)).WillOnce(Throw(data::DatabaseTimeout{})); + EXPECT_CALL(*mockCountersPtr_, rpcErrored(method)).WillOnce(Throw(data::DatabaseError{})); EXPECT_CALL(*handlerProvider, contains(method)).WillOnce(Return(true)); EXPECT_CALL(*mockCountersPtr_, onTooBusy()); diff --git a/tests/unit/util/RetryTests.cpp b/tests/unit/util/RetryTests.cpp index 3a2d786e4..f6cbf799c 100644 --- a/tests/unit/util/RetryTests.cpp +++ b/tests/unit/util/RetryTests.cpp @@ -1,6 +1,7 @@ #include "util/AsioContextTestFixture.hpp" #include "util/Retry.hpp" +#include #include #include #include @@ -18,7 +19,7 @@ protected: TEST_F(RetryTests, ExponentialBackoffStrategy) { - ExponentialBackoffStrategy strategy{delay_, maxDelay_}; + ExponentialBackoffStrategy strategy{{.initial = delay_, .max = maxDelay_}}; EXPECT_EQ(strategy.getDelay(), delay_); @@ -46,7 +47,10 @@ struct RetryWithExponentialBackoffStrategyTests : SyncAsioContextTest, RetryTest } protected: - Retry retry_ = makeRetryExponentialBackoff(delay_, maxDelay_, boost::asio::make_strand(ctx_)); + Retry retry_ = makeRetryExponentialBackoff( + {.initial = delay_, .max = maxDelay_}, + boost::asio::make_strand(ctx_) + ); testing::MockFunction mockCallback_; }; @@ -88,3 +92,44 @@ TEST_F(RetryWithExponentialBackoffStrategyTests, Reset) EXPECT_EQ(retry_.attemptNumber(), 0); EXPECT_EQ(retry_.delayValue(), delay_); } + +struct RetryWaitTests : SyncAsioContextTest, RetryTests {}; + +TEST_F(RetryWaitTests, WaitOnCoroutineAdvancesAttemptAndDelay) +{ + runSpawn([this](auto yield) { + auto retry = makeRetryExponentialBackoff( + {.initial = delay_, .max = maxDelay_}, yield.get_executor() + ); + + EXPECT_EQ(retry.attemptNumber(), 0); + EXPECT_EQ(retry.delayValue(), delay_); + + retry.wait(yield); + EXPECT_EQ(retry.attemptNumber(), 1); + EXPECT_EQ(retry.delayValue(), delay_ * 2); + + retry.wait(yield); + EXPECT_EQ(retry.attemptNumber(), 2); + }); +} + +TEST_F(RetryWaitTests, WaitDoesNotBlockItsThread) +{ + bool ran = false; + bool ranBeforeWaitReturned = false; + + runSpawn([this, &ran, &ranBeforeWaitReturned](auto yield) { + boost::asio::post(ctx_, [&ran]() { ran = true; }); + + auto retry = makeRetryExponentialBackoff( + {.initial = std::chrono::milliseconds{20}, .max = std::chrono::milliseconds{20}}, + yield.get_executor() + ); + retry.wait(yield); + + ranBeforeWaitReturned = ran; + }); + + EXPECT_TRUE(ranBeforeWaitReturned); +} From 62428a2d9140a20922a28a9005e5c680f64a569c Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Wed, 12 Aug 2026 09:57:52 +0100 Subject: [PATCH 7/7] chore: Update libxrpl to 3.3.0 (#3170) --- conan.lock | 4 +- conanfile.py | 2 +- src/rpc/handlers/AccountMPTokenIssuances.cpp | 47 ++++--- src/rpc/handlers/AccountMPTokenIssuances.hpp | 17 +-- tests/common/util/TestObject.cpp | 6 +- tests/common/util/TestObject.hpp | 2 +- .../handlers/AccountMPTokenIssuancesTests.cpp | 115 +++++++++--------- 7 files changed, 101 insertions(+), 92 deletions(-) diff --git a/conan.lock b/conan.lock index 3d063ec20..75cc25874 100644 --- a/conan.lock +++ b/conan.lock @@ -3,7 +3,7 @@ "requires": [ "zlib/1.3.2#1cb806da49011867778ffb6ac7190fcb%1782392402.122708", "xxhash/0.8.3#681d36a0a6111fc56e5e45ea182c19cc%1782392402.420688", - "xrpl/3.3.0-rc1-custom#cdfb12fc2671eaea72d5920bea800fd2%1785426802.141277", + "xrpl/3.3.0#5e356a24ae1f0d6da6bd617b926f92e6%1786467262.262007", "sqlite3/3.53.0#324ada52333108388a9a6108bfa96734%1782392403.185447", "spdlog/1.17.0#bcbaaf7147bda6ad24ffbd1ac3d7142c%1782736610.443882", "soci/4.0.3#e726491a03468795453f7c83fc924a96%1782392402.679521", @@ -15,7 +15,7 @@ "protobuf/6.33.5#ff253ead763bd8d9904a52979cd21e81%1782392410.233933", "openssl/3.6.3#f806de8933e3bf6f01016c6a888cee2e%1783945160.863288", "nudb/2.0.9#11149c73f8f2baff9a0198fe25971fc7%1782392402.297166", - "mpt-crypto/0.4.0-rc4#ffdba12f2332357f0d8b0ae944cfff52%1784138702.932355", + "mpt-crypto/1.0.2#b313cef0c1a493eb970ad185b2e9bab7%1784285108.866483", "minizip/1.2.13#64dfec2ee447ab6c0c7eab967815a762%1782736605.272739", "lz4/1.10.0#982d9b673900f665a1da109e09c17cab%1782392402.164188", "libuv/1.46.0#e1a592bf7c0f37802889ca2c795fb26c%1782736605.776567", diff --git a/conanfile.py b/conanfile.py index a4139086a..d638113c9 100644 --- a/conanfile.py +++ b/conanfile.py @@ -17,7 +17,7 @@ class ClioConan(ConanFile): "fmt/12.1.0", "libbacktrace/cci.20210118", "spdlog/1.17.0", - "xrpl/3.3.0-rc1-custom", + "xrpl/3.3.0", ] default_options = { diff --git a/src/rpc/handlers/AccountMPTokenIssuances.cpp b/src/rpc/handlers/AccountMPTokenIssuances.cpp index b5acc7de9..5870adf67 100644 --- a/src/rpc/handlers/AccountMPTokenIssuances.cpp +++ b/src/rpc/handlers/AccountMPTokenIssuances.cpp @@ -57,22 +57,25 @@ AccountMPTokenIssuancesHandler::addMPTokenIssuance( setFlag(issuance.mptCanClawback, xrpl::lsfMPTCanClawback); setFlag(issuance.mptCanHoldConfidentialBalance, xrpl::lsfMPTCanHoldConfidentialBalance); - if (sle.isFieldPresent(xrpl::sfMutableFlags)) { - auto const mutableFlags = sle.getFieldU32(xrpl::sfMutableFlags); + if (sle.isFieldPresent(xrpl::sfImmutableFlags)) { + auto const immutableFlags = sle.getFieldU32(xrpl::sfImmutableFlags); - auto const setMutableFlag = [&](std::optional& field, std::uint32_t mask) { - if ((mutableFlags & mask) != 0u) + auto const setImmutableFlag = [&](std::optional& field, std::uint32_t mask) { + if ((immutableFlags & mask) != 0u) field = true; }; - setMutableFlag(issuance.mptCanMutateCanLock, xrpl::lsmfMPTCanEnableCanLock); - setMutableFlag(issuance.mptCanMutateRequireAuth, xrpl::lsmfMPTCanEnableRequireAuth); - setMutableFlag(issuance.mptCanMutateCanEscrow, xrpl::lsmfMPTCanEnableCanEscrow); - setMutableFlag(issuance.mptCanMutateCanTrade, xrpl::lsmfMPTCanEnableCanTrade); - setMutableFlag(issuance.mptCanMutateCanTransfer, xrpl::lsmfMPTCanEnableCanTransfer); - setMutableFlag(issuance.mptCanMutateCanClawback, xrpl::lsmfMPTCanEnableCanClawback); - setMutableFlag(issuance.mptCanMutateMetadata, xrpl::lsmfMPTCanMutateMetadata); - setMutableFlag(issuance.mptCanMutateTransferFee, xrpl::lsmfMPTCanMutateTransferFee); + setImmutableFlag(issuance.mptImmutableCanLock, xrpl::lsifMPTCanLock); + setImmutableFlag(issuance.mptImmutableRequireAuth, xrpl::lsifMPTRequireAuth); + setImmutableFlag(issuance.mptImmutableCanEscrow, xrpl::lsifMPTCanEscrow); + setImmutableFlag(issuance.mptImmutableCanTrade, xrpl::lsifMPTCanTrade); + setImmutableFlag(issuance.mptImmutableCanTransfer, xrpl::lsifMPTCanTransfer); + setImmutableFlag(issuance.mptImmutableCanClawback, xrpl::lsifMPTCanClawback); + setImmutableFlag( + issuance.mptImmutableCanHoldConfidentialBalance, xrpl::lsifMPTCanHoldConfidentialBalance + ); + setImmutableFlag(issuance.mptImmutableMetadata, xrpl::lsifMPTMetadata); + setImmutableFlag(issuance.mptImmutableTransferFee, xrpl::lsifMPTTransferFee); } if (sle.isFieldPresent(xrpl::sfTransferFee)) @@ -277,14 +280,18 @@ tag_invoke( setIfPresent("mpt_can_transfer", issuance.mptCanTransfer); setIfPresent("mpt_can_clawback", issuance.mptCanClawback); - setIfPresent("mpt_can_mutate_can_lock", issuance.mptCanMutateCanLock); - setIfPresent("mpt_can_mutate_require_auth", issuance.mptCanMutateRequireAuth); - setIfPresent("mpt_can_mutate_can_escrow", issuance.mptCanMutateCanEscrow); - setIfPresent("mpt_can_mutate_can_trade", issuance.mptCanMutateCanTrade); - setIfPresent("mpt_can_mutate_can_transfer", issuance.mptCanMutateCanTransfer); - setIfPresent("mpt_can_mutate_can_clawback", issuance.mptCanMutateCanClawback); - setIfPresent("mpt_can_mutate_metadata", issuance.mptCanMutateMetadata); - setIfPresent("mpt_can_mutate_transfer_fee", issuance.mptCanMutateTransferFee); + setIfPresent("mpt_immutable_can_lock", issuance.mptImmutableCanLock); + setIfPresent("mpt_immutable_require_auth", issuance.mptImmutableRequireAuth); + setIfPresent("mpt_immutable_can_escrow", issuance.mptImmutableCanEscrow); + setIfPresent("mpt_immutable_can_trade", issuance.mptImmutableCanTrade); + setIfPresent("mpt_immutable_can_transfer", issuance.mptImmutableCanTransfer); + setIfPresent("mpt_immutable_can_clawback", issuance.mptImmutableCanClawback); + setIfPresent( + "mpt_immutable_can_hold_confidential_balance", + issuance.mptImmutableCanHoldConfidentialBalance + ); + setIfPresent("mpt_immutable_metadata", issuance.mptImmutableMetadata); + setIfPresent("mpt_immutable_transfer_fee", issuance.mptImmutableTransferFee); setIfPresent("mpt_can_hold_confidential_balance", issuance.mptCanHoldConfidentialBalance); setUint64IfPresent( diff --git a/src/rpc/handlers/AccountMPTokenIssuances.hpp b/src/rpc/handlers/AccountMPTokenIssuances.hpp index e26fba853..1cd679569 100644 --- a/src/rpc/handlers/AccountMPTokenIssuances.hpp +++ b/src/rpc/handlers/AccountMPTokenIssuances.hpp @@ -63,14 +63,15 @@ public: std::optional mptCanTransfer; std::optional mptCanClawback; - std::optional mptCanMutateCanLock; - std::optional mptCanMutateRequireAuth; - std::optional mptCanMutateCanEscrow; - std::optional mptCanMutateCanTrade; - std::optional mptCanMutateCanTransfer; - std::optional mptCanMutateCanClawback; - std::optional mptCanMutateMetadata; - std::optional mptCanMutateTransferFee; + std::optional mptImmutableCanLock; + std::optional mptImmutableRequireAuth; + std::optional mptImmutableCanEscrow; + std::optional mptImmutableCanTrade; + std::optional mptImmutableCanTransfer; + std::optional mptImmutableCanClawback; + std::optional mptImmutableCanHoldConfidentialBalance; + std::optional mptImmutableMetadata; + std::optional mptImmutableTransferFee; std::optional mptCanHoldConfidentialBalance; std::optional confidentialOutstandingAmount; diff --git a/tests/common/util/TestObject.cpp b/tests/common/util/TestObject.cpp index 19fe2d95e..0f5034fac 100644 --- a/tests/common/util/TestObject.cpp +++ b/tests/common/util/TestObject.cpp @@ -1493,7 +1493,7 @@ createMptIssuanceObject( std::optional maxAmount, std::optional lockedAmount, std::optional domainId, - std::optional mutableFlags, + std::optional immutableFlags, std::optional issuerEncryptionKey, std::optional auditorEncryptionKey, std::optional confidentialOutstandingAmount @@ -1523,8 +1523,8 @@ createMptIssuanceObject( } if (domainId.has_value()) mptIssuance.setFieldH256(xrpl::sfDomainID, xrpl::uint256{*domainId}); - if (mutableFlags.has_value()) - mptIssuance.setFieldU32(xrpl::sfMutableFlags, *mutableFlags); + if (immutableFlags.has_value()) + mptIssuance.setFieldU32(xrpl::sfImmutableFlags, *immutableFlags); if (issuerEncryptionKey.has_value()) { xrpl::Slice const slice(issuerEncryptionKey->data(), issuerEncryptionKey->size()); mptIssuance.setFieldVL(xrpl::sfIssuerEncryptionKey, slice); diff --git a/tests/common/util/TestObject.hpp b/tests/common/util/TestObject.hpp index 280f8798c..e6c4e629d 100644 --- a/tests/common/util/TestObject.hpp +++ b/tests/common/util/TestObject.hpp @@ -466,7 +466,7 @@ createMptIssuanceObject( std::optional maxAmount = std::nullopt, std::optional lockedAmount = std::nullopt, std::optional domainId = std::nullopt, - std::optional mutableFlags = std::nullopt, + std::optional immutableFlags = std::nullopt, std::optional issuerEncryptionKey = std::nullopt, std::optional auditorEncryptionKey = std::nullopt, std::optional confidentialOutstandingAmount = std::nullopt diff --git a/tests/unit/rpc/handlers/AccountMPTokenIssuancesTests.cpp b/tests/unit/rpc/handlers/AccountMPTokenIssuancesTests.cpp index 638dbc99d..a472c5d05 100644 --- a/tests/unit/rpc/handlers/AccountMPTokenIssuancesTests.cpp +++ b/tests/unit/rpc/handlers/AccountMPTokenIssuancesTests.cpp @@ -956,15 +956,13 @@ TEST_P(AccountMPTokenIssuancesAmountSerializationTest, SerializedAsStrings) }); } -TEST_F(RPCAccountMPTokenIssuancesHandlerTest, MutableFlags) +TEST_F(RPCAccountMPTokenIssuancesHandlerTest, ImmutableFlags) { - uint32_t const mutableFlags1 = xrpl::lsmfMPTCanEnableCanLock | - xrpl::lsmfMPTCanEnableRequireAuth | xrpl::lsmfMPTCanEnableCanEscrow | - xrpl::lsmfMPTCanEnableCanTrade; + uint32_t const immutableFlags1 = xrpl::lsifMPTCanLock | xrpl::lsifMPTRequireAuth | + xrpl::lsifMPTCanEscrow | xrpl::lsifMPTCanTrade; - uint32_t const mutableFlags2 = xrpl::lsmfMPTCanEnableCanTransfer | - xrpl::lsmfMPTCanEnableCanClawback | xrpl::lsmfMPTCanMutateMetadata | - xrpl::lsmfMPTCanMutateTransferFee; + uint32_t const immutableFlags2 = xrpl::lsifMPTCanTransfer | xrpl::lsifMPTCanClawback | + xrpl::lsifMPTMetadata | xrpl::lsifMPTTransferFee; auto const ledgerHeader = createLedgerHeader(kLedgerHash, 30); EXPECT_CALL(*backend_, fetchLedgerBySequence).WillOnce(Return(ledgerHeader)); @@ -993,7 +991,7 @@ TEST_F(RPCAccountMPTokenIssuancesHandlerTest, MutableFlags) std::nullopt, std::nullopt, std::nullopt, - mutableFlags1 + immutableFlags1 ) .getSerializer() .peekData(), @@ -1009,7 +1007,7 @@ TEST_F(RPCAccountMPTokenIssuancesHandlerTest, MutableFlags) std::nullopt, std::nullopt, std::nullopt, - mutableFlags2 + immutableFlags2 ) .getSerializer() .peekData() @@ -1042,10 +1040,10 @@ TEST_F(RPCAccountMPTokenIssuancesHandlerTest, MutableFlags) "outstanding_amount": "{}", "transfer_fee": {}, "mpt_can_transfer": true, - "mpt_can_mutate_can_lock": true, - "mpt_can_mutate_require_auth": true, - "mpt_can_mutate_can_escrow": true, - "mpt_can_mutate_can_trade": true + "mpt_immutable_can_lock": true, + "mpt_immutable_require_auth": true, + "mpt_immutable_can_escrow": true, + "mpt_immutable_can_trade": true }}, {{ "mpt_issuance_id": "{}", @@ -1055,10 +1053,10 @@ TEST_F(RPCAccountMPTokenIssuancesHandlerTest, MutableFlags) "transfer_fee": {}, "mptoken_metadata": "{}", "mpt_can_transfer": true, - "mpt_can_mutate_can_transfer": true, - "mpt_can_mutate_can_clawback": true, - "mpt_can_mutate_metadata": true, - "mpt_can_mutate_transfer_fee": true + "mpt_immutable_can_transfer": true, + "mpt_immutable_can_clawback": true, + "mpt_immutable_metadata": true, + "mpt_immutable_transfer_fee": true }} ] }})JSON", @@ -1174,8 +1172,8 @@ struct SingleFlagTest { std::string expectedJsonKey; }; -struct AccountMPTokenIssuancesImmutableFlagsTest : RPCAccountMPTokenIssuancesHandlerTest, - WithParamInterface {}; +struct AccountMPTokenIssuancesLedgerFlagsTest : RPCAccountMPTokenIssuancesHandlerTest, + WithParamInterface {}; static auto generateSingleFlagTests() @@ -1200,13 +1198,13 @@ generateSingleFlagTests() } INSTANTIATE_TEST_SUITE_P( - RPCAccountMPTokenIssuancesImmutableFlagsGroup, - AccountMPTokenIssuancesImmutableFlagsTest, + RPCAccountMPTokenIssuancesLedgerFlagsGroup, + AccountMPTokenIssuancesLedgerFlagsTest, ValuesIn(generateSingleFlagTests()), tests::util::kNameGenerator ); -TEST_P(AccountMPTokenIssuancesImmutableFlagsTest, SingleFlag) +TEST_P(AccountMPTokenIssuancesLedgerFlagsTest, SingleFlag) { auto const testParams = GetParam(); @@ -1254,54 +1252,57 @@ TEST_P(AccountMPTokenIssuancesImmutableFlagsTest, SingleFlag) }); } -struct SingleMutableFlagTest { +struct SingleImmutableFlagTest { std::string testName; - uint32_t mutableFlag; + uint32_t immutableFlag; std::string expectedJsonKey; }; -struct AccountMPTokenIssuancesMutableFlagsTest : RPCAccountMPTokenIssuancesHandlerTest, - WithParamInterface {}; +struct AccountMPTokenIssuancesImmutableFlagsTest : RPCAccountMPTokenIssuancesHandlerTest, + WithParamInterface {}; static auto -generateSingleMutableFlagTests() +generateSingleImmutableFlagTests() { - return std::vector{ - {.testName = "CanMutateCanLock", - .mutableFlag = xrpl::lsmfMPTCanEnableCanLock, - .expectedJsonKey = "mpt_can_mutate_can_lock"}, - {.testName = "CanMutateRequireAuth", - .mutableFlag = xrpl::lsmfMPTCanEnableRequireAuth, - .expectedJsonKey = "mpt_can_mutate_require_auth"}, - {.testName = "CanMutateCanEscrow", - .mutableFlag = xrpl::lsmfMPTCanEnableCanEscrow, - .expectedJsonKey = "mpt_can_mutate_can_escrow"}, - {.testName = "CanMutateCanTrade", - .mutableFlag = xrpl::lsmfMPTCanEnableCanTrade, - .expectedJsonKey = "mpt_can_mutate_can_trade"}, - {.testName = "CanMutateCanTransfer", - .mutableFlag = xrpl::lsmfMPTCanEnableCanTransfer, - .expectedJsonKey = "mpt_can_mutate_can_transfer"}, - {.testName = "CanMutateCanClawback", - .mutableFlag = xrpl::lsmfMPTCanEnableCanClawback, - .expectedJsonKey = "mpt_can_mutate_can_clawback"}, - {.testName = "CanMutateMetadata", - .mutableFlag = xrpl::lsmfMPTCanMutateMetadata, - .expectedJsonKey = "mpt_can_mutate_metadata"}, - {.testName = "CanMutateTransferFee", - .mutableFlag = xrpl::lsmfMPTCanMutateTransferFee, - .expectedJsonKey = "mpt_can_mutate_transfer_fee"}, + return std::vector{ + {.testName = "ImmutableCanLock", + .immutableFlag = xrpl::lsifMPTCanLock, + .expectedJsonKey = "mpt_immutable_can_lock"}, + {.testName = "ImmutableRequireAuth", + .immutableFlag = xrpl::lsifMPTRequireAuth, + .expectedJsonKey = "mpt_immutable_require_auth"}, + {.testName = "ImmutableCanEscrow", + .immutableFlag = xrpl::lsifMPTCanEscrow, + .expectedJsonKey = "mpt_immutable_can_escrow"}, + {.testName = "ImmutableCanTrade", + .immutableFlag = xrpl::lsifMPTCanTrade, + .expectedJsonKey = "mpt_immutable_can_trade"}, + {.testName = "ImmutableCanTransfer", + .immutableFlag = xrpl::lsifMPTCanTransfer, + .expectedJsonKey = "mpt_immutable_can_transfer"}, + {.testName = "ImmutableCanClawback", + .immutableFlag = xrpl::lsifMPTCanClawback, + .expectedJsonKey = "mpt_immutable_can_clawback"}, + {.testName = "ImmutableCanHoldConfidentialBalance", + .immutableFlag = xrpl::lsifMPTCanHoldConfidentialBalance, + .expectedJsonKey = "mpt_immutable_can_hold_confidential_balance"}, + {.testName = "ImmutableMetadata", + .immutableFlag = xrpl::lsifMPTMetadata, + .expectedJsonKey = "mpt_immutable_metadata"}, + {.testName = "ImmutableTransferFee", + .immutableFlag = xrpl::lsifMPTTransferFee, + .expectedJsonKey = "mpt_immutable_transfer_fee"}, }; } INSTANTIATE_TEST_SUITE_P( - RPCAccountMPTokenIssuancesMutableFlagsGroup, - AccountMPTokenIssuancesMutableFlagsTest, - ValuesIn(generateSingleMutableFlagTests()), + RPCAccountMPTokenIssuancesImmutableFlagsGroup, + AccountMPTokenIssuancesImmutableFlagsTest, + ValuesIn(generateSingleImmutableFlagTests()), tests::util::kNameGenerator ); -TEST_P(AccountMPTokenIssuancesMutableFlagsTest, SingleMutableFlag) +TEST_P(AccountMPTokenIssuancesImmutableFlagsTest, SingleImmutableFlag) { auto const testParams = GetParam(); @@ -1330,7 +1331,7 @@ TEST_P(AccountMPTokenIssuancesMutableFlagsTest, SingleMutableFlag) std::nullopt, std::nullopt, std::nullopt, - testParams.mutableFlag + testParams.immutableFlag ) .getSerializer() .peekData()};