From 00d3cee6ccc5dd87c6005c55ace5bf3ce064a8c5 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 16 Jan 2026 13:26:30 -0400 Subject: [PATCH 1/3] Improve ledger_entry lookups for fee, amendments, NUNL, and hashes (#5644) These "fixed location" objects can be found in multiple ways: 1. The lookup parameters use the same format as other ledger objects, but the only valid value is true or the valid index of the object: - Amendments: "amendments" : true - FeeSettings: "fee" : true - NegativeUNL: "nunl" : true - LedgerHashes: "hashes" : true (For the "short" list. See below.) 2. With RPC API >= 3, using special case values to "index", such as "index" : "amendments". Uses the same names as above. Note that for "hashes", this option will only return the recent ledger hashes / "short" skip list. 3. LedgerHashes has two types: "short", which stores recent ledger hashes, and "long", which stores the flag ledger hashes for a particular ledger range. - To find a "long" LedgerHashes object, request '"hashes" : '. must be a number that evaluates to an unsigned integer. - To find the "short" LedgerHashes object, request "hashes": true as with the other fixed objects. The following queries are all functionally equivalent: - "amendments" : true - "index" : "amendments" (API >=3 only) - "amendments" : "7DB0788C020F02780A673DC74757F23823FA3014C1866E72CC4CD8B226CD6EF4" - "index" : "7DB0788C020F02780A673DC74757F23823FA3014C1866E72CC4CD8B226CD6EF4" Finally, whether the object is found or not, if a valid index is computed, that index will be returned. This can be used to confirm the query was valid, or to save the index for future use. --- src/test/rpc/LedgerEntry_test.cpp | 469 ++++++++++++++++++++++++- src/xrpld/rpc/handlers/LedgerEntry.cpp | 237 ++++++++++--- 2 files changed, 649 insertions(+), 57 deletions(-) diff --git a/src/test/rpc/LedgerEntry_test.cpp b/src/test/rpc/LedgerEntry_test.cpp index 551e67dc5e..1b7079341c 100644 --- a/src/test/rpc/LedgerEntry_test.cpp +++ b/src/test/rpc/LedgerEntry_test.cpp @@ -5,6 +5,8 @@ #include #include +#include + #include #include #include @@ -30,6 +32,7 @@ enum class FieldType { CurrencyField, HashField, HashOrObjectField, + FixedHashField, IssueField, ObjectField, StringField, @@ -86,6 +89,7 @@ getTypeName(FieldType typeID) case FieldType::CurrencyField: return "Currency"; case FieldType::HashField: + case FieldType::FixedHashField: return "hex string"; case FieldType::HashOrObjectField: return "hex string or object"; @@ -202,6 +206,7 @@ class LedgerEntry_test : public beast::unit_test::suite static auto const& badBlobValues = remove({3, 7, 8, 16}); static auto const& badCurrencyValues = remove({14}); static auto const& badHashValues = remove({2, 3, 7, 8, 16}); + static auto const& badFixedHashValues = remove({1, 2, 3, 4, 7, 8, 16}); static auto const& badIndexValues = remove({12, 16, 18, 19}); static auto const& badUInt32Values = remove({2, 3}); static auto const& badUInt64Values = remove({2, 3}); @@ -222,6 +227,8 @@ class LedgerEntry_test : public beast::unit_test::suite return badHashValues; case FieldType::HashOrObjectField: return badIndexValues; + case FieldType::FixedHashField: + return badFixedHashValues; case FieldType::IssueField: return badIssueValues; case FieldType::UInt32Field: @@ -717,7 +724,12 @@ class LedgerEntry_test : public beast::unit_test::suite } // negative tests - runLedgerEntryTest(env, jss::amendments); + testMalformedField( + env, + Json::Value{}, + jss::amendments, + FieldType::FixedHashField, + "malformedRequest"); } void @@ -1538,7 +1550,12 @@ class LedgerEntry_test : public beast::unit_test::suite } // negative tests - runLedgerEntryTest(env, jss::fee); + testMalformedField( + env, + Json::Value{}, + jss::fee, + FieldType::FixedHashField, + "malformedRequest"); } void @@ -1561,7 +1578,12 @@ class LedgerEntry_test : public beast::unit_test::suite } // negative tests - runLedgerEntryTest(env, jss::hashes); + testMalformedField( + env, + Json::Value{}, + jss::hashes, + FieldType::FixedHashField, + "malformedRequest"); } void @@ -1686,7 +1708,12 @@ class LedgerEntry_test : public beast::unit_test::suite } // negative tests - runLedgerEntryTest(env, jss::nunl); + testMalformedField( + env, + Json::Value{}, + jss::nunl, + FieldType::FixedHashField, + "malformedRequest"); } void @@ -2343,6 +2370,438 @@ class LedgerEntry_test : public beast::unit_test::suite } } + /// Test the ledger entry types that don't take parameters + void + testFixed() + { + using namespace test::jtx; + + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, envconfig([](auto cfg) { + cfg->START_UP = Config::FRESH; + return cfg; + })}; + + env.close(); + + /** Verifies that the RPC result has the expected data + * + * @param good: Indicates that the request should have succeeded + * and returned a ledger object of `expectedType` type. + * @param jv: The RPC result Json value + * @param expectedType: The type that the ledger object should + * have if "good". + * @param expectedError: Optional. The expected error if not + * good. Defaults to "entryNotFound". + */ + auto checkResult = + [&](bool good, + Json::Value const& jv, + Json::StaticString const& expectedType, + std::optional const& expectedError = {}) { + if (good) + { + BEAST_EXPECTS( + jv.isObject() && jv.isMember(jss::result) && + !jv[jss::result].isMember(jss::error) && + jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::node].isMember( + sfLedgerEntryType.jsonName) && + jv[jss::result][jss::node] + [sfLedgerEntryType.jsonName] == expectedType, + to_string(jv)); + } + else + { + BEAST_EXPECTS( + jv.isObject() && jv.isMember(jss::result) && + jv[jss::result].isMember(jss::error) && + !jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::error] == + expectedError.value_or("entryNotFound"), + to_string(jv)); + } + }; + + /** Runs a series of tests for a given fixed-position ledger + * entry. + * + * @param field: The Json request field to use. + * @param expectedType: The type that the ledger object should + * have if "good". + * @param expectedKey: The keylet of the fixed object. + * @param good: Indicates whether the object is expected to + * exist. + */ + auto test = [&](Json::StaticString const& field, + Json::StaticString const& expectedType, + Keylet const& expectedKey, + bool good) { + testcase << expectedType.c_str() << (good ? "" : " not") + << " found"; + + auto const hexKey = strHex(expectedKey.key); + + { + // Test bad values + // "field":null + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[field] = Json::nullValue; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, expectedType, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // "field":"string" + params[jss::ledger_index] = jss::validated; + params[field] = "arbitrary string"; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, expectedType, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // "field":false + params[jss::ledger_index] = jss::validated; + params[field] = false; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, expectedType, "invalidParams"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + + // "field":[incorrect index hash] + auto const badKey = strHex(expectedKey.key + uint256{1}); + params[jss::ledger_index] = jss::validated; + params[field] = badKey; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, expectedType, "entryNotFound"); + BEAST_EXPECTS( + jv[jss::result][jss::index] == badKey, to_string(jv)); + } + + { + Json::Value params; + // "index":"field" using API 2 + params[jss::ledger_index] = jss::validated; + params[jss::index] = field; + params[jss::api_version] = 2; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, expectedType, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + std::string const pdIdx = [&]() { + { + Json::Value params; + // Test good values + // Use the "field":true notation + params[jss::ledger_index] = jss::validated; + params[field] = true; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + // Index will always be returned for valid parameters. + std::string const pdIdx = + jv[jss::result][jss::index].asString(); + BEAST_EXPECTS(hexKey == pdIdx, to_string(jv)); + checkResult(good, jv, expectedType); + + return pdIdx; + } + }(); + + { + Json::Value params; + // "field":"[index hash]" + params[jss::ledger_index] = jss::validated; + params[field] = hexKey; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(good, jv, expectedType); + BEAST_EXPECT(jv[jss::result][jss::index].asString() == hexKey); + } + + { + // Bad value + // Use the "index":"field" notation with API 2 + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::index] = field; + params[jss::api_version] = 2; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, expectedType, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // Use the "index":"field" notation with API 3 + params[jss::ledger_index] = jss::validated; + params[jss::index] = field; + params[jss::api_version] = 3; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + // Index is correct either way + BEAST_EXPECT(jv[jss::result][jss::index].asString() == hexKey); + checkResult(good, jv, expectedType); + } + + { + Json::Value params; + // Use the "index":"[index hash]" notation + params[jss::ledger_index] = jss::validated; + params[jss::index] = pdIdx; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + // Index is correct either way + BEAST_EXPECT(jv[jss::result][jss::index].asString() == hexKey); + checkResult(good, jv, expectedType); + } + }; + + test(jss::amendments, jss::Amendments, keylet::amendments(), true); + test(jss::fee, jss::FeeSettings, keylet::fees(), true); + // There won't be an nunl + test(jss::nunl, jss::NegativeUNL, keylet::negativeUNL(), false); + // Can only get the short skip list this way + test(jss::hashes, jss::LedgerHashes, keylet::skip(), true); + } + + void + testHashes() + { + using namespace test::jtx; + + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, envconfig([](auto cfg) { + cfg->START_UP = Config::FRESH; + return cfg; + })}; + + env.close(); + + /** Verifies that the RPC result has the expected data + * + * @param good: Indicates that the request should have succeeded + * and returned a ledger object of `expectedType` type. + * @param jv: The RPC result Json value + * @param expectedCount: The number of Hashes expected in the + * object if "good". + * @param expectedError: Optional. The expected error if not + * good. Defaults to "entryNotFound". + */ + auto checkResult = + [&](bool good, + Json::Value const& jv, + int expectedCount, + std::optional const& expectedError = {}) { + if (good) + { + BEAST_EXPECTS( + jv.isObject() && jv.isMember(jss::result) && + !jv[jss::result].isMember(jss::error) && + jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::node].isMember( + sfLedgerEntryType.jsonName) && + jv[jss::result][jss::node] + [sfLedgerEntryType.jsonName] == jss::LedgerHashes, + to_string(jv)); + BEAST_EXPECTS( + jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::node].isMember("Hashes") && + jv[jss::result][jss::node]["Hashes"].size() == + expectedCount, + to_string(jv[jss::result][jss::node]["Hashes"].size())); + } + else + { + BEAST_EXPECTS( + jv.isObject() && jv.isMember(jss::result) && + jv[jss::result].isMember(jss::error) && + !jv[jss::result].isMember(jss::node) && + jv[jss::result][jss::error] == + expectedError.value_or("entryNotFound"), + to_string(jv)); + } + }; + + /** Runs a series of tests for a given ledger index. + * + * @param ledger: The ledger index value of the "hashes" request + * parameter. May not necessarily be a number. + * @param expectedKey: The expected keylet of the object. + * @param good: Indicates whether the object is expected to + * exist. + * @param expectedCount: The number of Hashes expected in the + * object if "good". + */ + auto test = [&](Json::Value ledger, + Keylet const& expectedKey, + bool good, + int expectedCount = 0) { + testcase << "LedgerHashes: seq: " << env.current()->header().seq + << " \"hashes\":" << to_string(ledger) + << (good ? "" : " not") << " found"; + + auto const hexKey = strHex(expectedKey.key); + + { + // Test bad values + // "hashes":null + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = Json::nullValue; + auto jv = env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, 0, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // "hashes":"non-uint string" + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = "arbitrary string"; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, 0, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // "hashes":"uint string" is invalid, too + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = "10"; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, 0, "malformedRequest"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // "hashes":false + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = false; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, 0, "invalidParams"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + { + Json::Value params; + // "hashes":-1 + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = -1; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, 0, "internal"); + BEAST_EXPECT(!jv[jss::result].isMember(jss::index)); + } + + // "hashes":[incorrect index hash] + { + Json::Value params; + auto const badKey = strHex(expectedKey.key + uint256{1}); + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = badKey; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(false, jv, 0, "entryNotFound"); + BEAST_EXPECT(jv[jss::result][jss::index] == badKey); + } + + { + Json::Value params; + // Test good values + // Use the "hashes":ledger notation + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = ledger; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(good, jv, expectedCount); + // Index will always be returned for valid parameters. + std::string const pdIdx = + jv[jss::result][jss::index].asString(); + BEAST_EXPECTS(hexKey == pdIdx, strHex(pdIdx)); + } + + { + Json::Value params; + // "hashes":"[index hash]" + params[jss::ledger_index] = jss::validated; + params[jss::hashes] = hexKey; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(good, jv, expectedCount); + // Index is correct either way + BEAST_EXPECTS( + hexKey == jv[jss::result][jss::index].asString(), + strHex(jv[jss::result][jss::index].asString())); + } + + { + Json::Value params; + // Use the "index":"[index hash]" notation + params[jss::ledger_index] = jss::validated; + params[jss::index] = hexKey; + auto const jv = + env.rpc("json", "ledger_entry", to_string(params)); + checkResult(good, jv, expectedCount); + // Index is correct either way + BEAST_EXPECTS( + hexKey == jv[jss::result][jss::index].asString(), + strHex(jv[jss::result][jss::index].asString())); + } + }; + + // short skip list + test(true, keylet::skip(), true, 2); + // long skip list at index 0 + test(1, keylet::skip(1), false); + // long skip list at index 1 + test(1 << 17, keylet::skip(1 << 17), false); + + // Close more ledgers, but stop short of the flag ledger + for (auto i = env.current()->seq(); i <= 250; ++i) + env.close(); + + // short skip list + test(true, keylet::skip(), true, 249); + // long skip list at index 0 + test(1, keylet::skip(1), false); + // long skip list at index 1 + test(1 << 17, keylet::skip(1 << 17), false); + + // Close a flag ledger so the first "long" skip list is created + for (auto i = env.current()->seq(); i <= 260; ++i) + env.close(); + + // short skip list + test(true, keylet::skip(), true, 256); + // long skip list at index 0 + test(1, keylet::skip(1), true, 1); + // long skip list at index 1 + test(1 << 17, keylet::skip(1 << 17), false); + } + void testCLI() { @@ -2400,6 +2859,8 @@ public: testOracleLedgerEntry(); testMPT(); testPermissionedDomain(); + testFixed(); + testHashes(); testCLI(); } }; diff --git a/src/xrpld/rpc/handlers/LedgerEntry.cpp b/src/xrpld/rpc/handlers/LedgerEntry.cpp index d9ae357b1a..17d8e5e7bc 100644 --- a/src/xrpld/rpc/handlers/LedgerEntry.cpp +++ b/src/xrpld/rpc/handlers/LedgerEntry.cpp @@ -18,6 +18,32 @@ namespace xrpl { +using FunctionType = std::function( + Json::Value const&, + Json::StaticString const, + unsigned const apiVersion)>; + +static Expected +parseFixed( + Keylet const& keylet, + Json::Value const& params, + Json::StaticString const& fieldName, + unsigned const apiVersion); + +// Helper function to return FunctionType for objects that have a fixed +// location. That is, they don't take parameters to compute the index. +// e.g. amendments, fees, negative UNL, etc. +static FunctionType +fixed(Keylet const& keylet) +{ + return [keylet]( + Json::Value const& params, + Json::StaticString const fieldName, + unsigned const apiVersion) -> Expected { + return parseFixed(keylet, params, fieldName, apiVersion); + }; +} + static Expected parseObjectID( Json::Value const& params, @@ -33,13 +59,33 @@ parseObjectID( } static Expected -parseIndex(Json::Value const& params, Json::StaticString const fieldName) +parseIndex( + Json::Value const& params, + Json::StaticString const fieldName, + unsigned const apiVersion) { + if (apiVersion > 2u && params.isString()) + { + std::string const index = params.asString(); + if (index == jss::amendments.c_str()) + return keylet::amendments().key; + if (index == jss::fee.c_str()) + return keylet::fees().key; + if (index == jss::nunl) + return keylet::negativeUNL().key; + if (index == jss::hashes) + // Note this only finds the "short" skip list. Use "hashes":index to + // get the long list. + return keylet::skip().key; + } return parseObjectID(params, fieldName, "hex string"); } static Expected -parseAccountRoot(Json::Value const& params, Json::StaticString const fieldName) +parseAccountRoot( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (auto const account = LedgerEntryHelpers::parse(params)) { @@ -50,14 +96,13 @@ parseAccountRoot(Json::Value const& params, Json::StaticString const fieldName) "malformedAddress", fieldName, "AccountID"); } -static Expected -parseAmendments(Json::Value const& params, Json::StaticString const fieldName) -{ - return parseObjectID(params, fieldName, "hex string"); -} +auto const parseAmendments = fixed(keylet::amendments()); static Expected -parseAMM(Json::Value const& params, Json::StaticString const fieldName) +parseAMM( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -85,7 +130,10 @@ parseAMM(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parseBridge(Json::Value const& params, Json::StaticString const fieldName) +parseBridge( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isMember(jss::bridge)) { @@ -116,13 +164,19 @@ parseBridge(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parseCheck(Json::Value const& params, Json::StaticString const fieldName) +parseCheck( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { return parseObjectID(params, fieldName, "hex string"); } static Expected -parseCredential(Json::Value const& cred, Json::StaticString const fieldName) +parseCredential( + Json::Value const& cred, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!cred.isObject()) { @@ -153,7 +207,10 @@ parseCredential(Json::Value const& cred, Json::StaticString const fieldName) } static Expected -parseDelegate(Json::Value const& params, Json::StaticString const fieldName) +parseDelegate( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -244,7 +301,10 @@ parseAuthorizeCredentials(Json::Value const& jv) } static Expected -parseDepositPreauth(Json::Value const& dp, Json::StaticString const fieldName) +parseDepositPreauth( + Json::Value const& dp, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!dp.isObject()) { @@ -297,7 +357,10 @@ parseDepositPreauth(Json::Value const& dp, Json::StaticString const fieldName) } static Expected -parseDID(Json::Value const& params, Json::StaticString const fieldName) +parseDID( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { auto const account = LedgerEntryHelpers::parse(params); if (!account) @@ -312,7 +375,8 @@ parseDID(Json::Value const& params, Json::StaticString const fieldName) static Expected parseDirectoryNode( Json::Value const& params, - Json::StaticString const fieldName) + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -365,7 +429,10 @@ parseDirectoryNode( } static Expected -parseEscrow(Json::Value const& params, Json::StaticString const fieldName) +parseEscrow( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -384,20 +451,53 @@ parseEscrow(Json::Value const& params, Json::StaticString const fieldName) return keylet::escrow(*id, *seq).key; } +auto const parseFeeSettings = fixed(keylet::fees()); + static Expected -parseFeeSettings(Json::Value const& params, Json::StaticString const fieldName) +parseFixed( + Keylet const& keylet, + Json::Value const& params, + Json::StaticString const& fieldName, + [[maybe_unused]] unsigned const apiVersion) { - return parseObjectID(params, fieldName, "hex string"); + if (!params.isBool()) + { + return parseObjectID(params, fieldName, "hex string"); + } + if (!params.asBool()) + { + return LedgerEntryHelpers::invalidFieldError( + "invalidParams", fieldName, "true"); + } + + return keylet.key; } static Expected -parseLedgerHashes(Json::Value const& params, Json::StaticString const fieldName) +parseLedgerHashes( + Json::Value const& params, + Json::StaticString const fieldName, + unsigned const apiVersion) { - return parseObjectID(params, fieldName, "hex string"); + if (params.isUInt() || params.isInt()) + { + // If the index doesn't parse as a UInt, throw + auto const index = params.asUInt(); + + // Return the "long" skip list for the given ledger index. + auto const keylet = keylet::skip(index); + return keylet.key; + } + // Return the key in `params` or the "short" skip list, which contains + // hashes since the last flag ledger. + return parseFixed(keylet::skip(), params, fieldName, apiVersion); } static Expected -parseLoanBroker(Json::Value const& params, Json::StaticString const fieldName) +parseLoanBroker( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -417,7 +517,10 @@ parseLoanBroker(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parseLoan(Json::Value const& params, Json::StaticString const fieldName) +parseLoan( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -437,7 +540,10 @@ parseLoan(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parseMPToken(Json::Value const& params, Json::StaticString const fieldName) +parseMPToken( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -460,7 +566,8 @@ parseMPToken(Json::Value const& params, Json::StaticString const fieldName) static Expected parseMPTokenIssuance( Json::Value const& params, - Json::StaticString const fieldName) + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { auto const mptIssuanceID = LedgerEntryHelpers::parse(params); if (!mptIssuanceID) @@ -471,25 +578,30 @@ parseMPTokenIssuance( } static Expected -parseNFTokenOffer(Json::Value const& params, Json::StaticString const fieldName) +parseNFTokenOffer( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { return parseObjectID(params, fieldName, "hex string"); } static Expected -parseNFTokenPage(Json::Value const& params, Json::StaticString const fieldName) +parseNFTokenPage( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { return parseObjectID(params, fieldName, "hex string"); } -static Expected -parseNegativeUNL(Json::Value const& params, Json::StaticString const fieldName) -{ - return parseObjectID(params, fieldName, "hex string"); -} +auto const parseNegativeUNL = fixed(keylet::negativeUNL()); static Expected -parseOffer(Json::Value const& params, Json::StaticString const fieldName) +parseOffer( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -510,7 +622,10 @@ parseOffer(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parseOracle(Json::Value const& params, Json::StaticString const fieldName) +parseOracle( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -531,7 +646,10 @@ parseOracle(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parsePayChannel(Json::Value const& params, Json::StaticString const fieldName) +parsePayChannel( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { return parseObjectID(params, fieldName, "hex string"); } @@ -539,7 +657,8 @@ parsePayChannel(Json::Value const& params, Json::StaticString const fieldName) static Expected parsePermissionedDomain( Json::Value const& pd, - Json::StaticString const fieldName) + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (pd.isString()) { @@ -568,7 +687,8 @@ parsePermissionedDomain( static Expected parseRippleState( Json::Value const& jvRippleState, - Json::StaticString const fieldName) + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { Currency uCurrency; @@ -618,13 +738,19 @@ parseRippleState( } static Expected -parseSignerList(Json::Value const& params, Json::StaticString const fieldName) +parseSignerList( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { return parseObjectID(params, fieldName, "hex string"); } static Expected -parseTicket(Json::Value const& params, Json::StaticString const fieldName) +parseTicket( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -645,7 +771,10 @@ parseTicket(Json::Value const& params, Json::StaticString const fieldName) } static Expected -parseVault(Json::Value const& params, Json::StaticString const fieldName) +parseVault( + Json::Value const& params, + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!params.isObject()) { @@ -668,7 +797,8 @@ parseVault(Json::Value const& params, Json::StaticString const fieldName) static Expected parseXChainOwnedClaimID( Json::Value const& claim_id, - Json::StaticString const fieldName) + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!claim_id.isObject()) { @@ -693,7 +823,8 @@ parseXChainOwnedClaimID( static Expected parseXChainOwnedCreateAccountClaimID( Json::Value const& claim_id, - Json::StaticString const fieldName) + Json::StaticString const fieldName, + [[maybe_unused]] unsigned const apiVersion) { if (!claim_id.isObject()) { @@ -717,10 +848,6 @@ parseXChainOwnedCreateAccountClaimID( return keylet.key; } -using FunctionType = Expected (*)( - Json::Value const&, - Json::StaticString const); - struct LedgerEntry { Json::StaticString fieldName; @@ -753,7 +880,7 @@ doLedgerEntry(RPC::JsonContext& context) {jss::ripple_state, parseRippleState, ltRIPPLE_STATE}, }); - auto hasMoreThanOneMember = [&]() { + auto const hasMoreThanOneMember = [&]() { int count = 0; for (auto const& ledgerEntry : ledgerEntryParsers) @@ -797,8 +924,8 @@ doLedgerEntry(RPC::JsonContext& context) Json::Value const& params = ledgerEntry.fieldName == jss::bridge ? context.params : context.params[ledgerEntry.fieldName]; - auto const result = - ledgerEntry.parseFunction(params, ledgerEntry.fieldName); + auto const result = ledgerEntry.parseFunction( + params, ledgerEntry.fieldName, context.apiVersion); if (!result) return result.error(); @@ -829,9 +956,13 @@ doLedgerEntry(RPC::JsonContext& context) throw; } + // Return the computed index regardless of whether the node exists. + jvResult[jss::index] = to_string(uNodeIndex); + if (uNodeIndex.isZero()) { - return RPC::make_error(rpcENTRY_NOT_FOUND); + RPC::inject_error(rpcENTRY_NOT_FOUND, jvResult); + return jvResult; } auto const sleNode = lpLedger->read(keylet::unchecked(uNodeIndex)); @@ -843,12 +974,14 @@ doLedgerEntry(RPC::JsonContext& context) if (!sleNode) { // Not found. - return RPC::make_error(rpcENTRY_NOT_FOUND); + RPC::inject_error(rpcENTRY_NOT_FOUND, jvResult); + return jvResult; } if ((expectedType != ltANY) && (expectedType != sleNode->getType())) { - return RPC::make_error(rpcUNEXPECTED_LEDGER_TYPE); + RPC::inject_error(rpcUNEXPECTED_LEDGER_TYPE, jvResult); + return jvResult; } if (bNodeBinary) @@ -858,12 +991,10 @@ doLedgerEntry(RPC::JsonContext& context) sleNode->add(s); jvResult[jss::node_binary] = strHex(s.peekData()); - jvResult[jss::index] = to_string(uNodeIndex); } else { jvResult[jss::node] = sleNode->getJson(JsonOptions::none); - jvResult[jss::index] = to_string(uNodeIndex); } return jvResult; From 12c0d67ff62f141f4c1d25ece56800f22cb3861e Mon Sep 17 00:00:00 2001 From: Bart Date: Fri, 16 Jan 2026 15:01:53 -0500 Subject: [PATCH 2/3] ci: remove 'master' branch as a trigger (#6234) This change removes the `master` branch as a trigger for the CI pipelines, and updates comments accordingly. It also fixes the pre-commit workflow, so it will run on all release branches. --- .github/scripts/strategy-matrix/generate.py | 4 ++-- .github/workflows/on-pr.yml | 2 +- .github/workflows/on-trigger.yml | 20 +++++++++----------- .github/workflows/pre-commit.yml | 4 +++- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index bf5bf5d3ba..0e44b1be54 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -20,8 +20,8 @@ class Config: Generate a strategy matrix for GitHub Actions CI. On each PR commit we will build a selection of Debian, RHEL, Ubuntu, MacOS, and -Windows configurations, while upon merge into the develop, release, or master -branches, we will build all configurations, and test most of them. +Windows configurations, while upon merge into the develop or release branches, +we will build all configurations, and test most of them. We will further set additional CMake arguments as follows: - All builds will have the `tests`, `werr`, and `xrpld` options. diff --git a/.github/workflows/on-pr.yml b/.github/workflows/on-pr.yml index 3aa48ac070..dad211f94f 100644 --- a/.github/workflows/on-pr.yml +++ b/.github/workflows/on-pr.yml @@ -125,7 +125,7 @@ jobs: needs: - should-run - build-test - if: ${{ needs.should-run.outputs.go == 'true' && (startsWith(github.base_ref, 'release') || github.base_ref == 'master') }} + if: ${{ needs.should-run.outputs.go == 'true' && startsWith(github.ref, 'refs/heads/release') }} uses: ./.github/workflows/reusable-notify-clio.yml secrets: clio_notify_token: ${{ secrets.CLIO_NOTIFY_TOKEN }} diff --git a/.github/workflows/on-trigger.yml b/.github/workflows/on-trigger.yml index 2c63c2baa5..ef7bf41fa2 100644 --- a/.github/workflows/on-trigger.yml +++ b/.github/workflows/on-trigger.yml @@ -1,9 +1,8 @@ -# This workflow runs all workflows to build the dependencies required for the -# project on various Linux flavors, as well as on MacOS and Windows, on a -# scheduled basis, on merge into the 'develop', 'release', or 'master' branches, -# or manually. The missing commits check is only run when the code is merged -# into the 'develop' or 'release' branches, and the documentation is built when -# the code is merged into the 'develop' branch. +# This workflow runs all workflows to build and test the code on various Linux +# flavors, as well as on MacOS and Windows, on a scheduled basis, on merge into +# the 'develop' or 'release*' branches, or when requested manually. Upon +# successful completion, it also uploads the built libxrpl package to the Conan +# remote. name: Trigger on: @@ -11,7 +10,6 @@ on: branches: - "develop" - "release*" - - "master" paths: # These paths are unique to `on-trigger.yml`. - ".github/workflows/on-trigger.yml" @@ -70,10 +68,10 @@ jobs: with: # Enable ccache only for events targeting the XRPLF repository, since # other accounts will not have access to our remote cache storage. - # However, we do not enable ccache for events targeting the master or a - # release branch, to protect against the rare case that the output - # produced by ccache is not identical to a regular compilation. - ccache_enabled: ${{ github.repository_owner == 'XRPLF' && !(github.base_ref == 'master' || startsWith(github.base_ref, 'release')) }} + # However, we do not enable ccache for events targeting a release branch, + # to protect against the rare case that the output produced by ccache is + # not identical to a regular compilation. + ccache_enabled: ${{ github.repository_owner == 'XRPLF' && !startsWith(github.ref, 'refs/heads/release') }} os: ${{ matrix.os }} strategy_matrix: ${{ github.event_name == 'schedule' && 'all' || 'minimal' }} secrets: diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 00754e5eae..6b8fd9955e 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -3,7 +3,9 @@ name: Run pre-commit hooks on: pull_request: push: - branches: [develop, release, master] + branches: + - "develop" + - "release*" workflow_dispatch: jobs: From 5e808794d875b9ea3fe894d17995a6f348dea148 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 21 Jan 2026 14:19:53 +0000 Subject: [PATCH 3/3] Limit reply size on `TMGetObjectByHash` queries (#6110) `PeerImp` processes `TMGetObjectByHash` queries with an unbounded per-request loop, which performs a `NodeStore` fetch and then appends retrieved data to the reply for each queried object without a local count cap or reply-byte budget. However, the `Nodestore` fetches are expensive when high in numbers, which might slow down the process overall. Hence this code change adds an upper cap on the response size. --- .../scripts/levelization/results/ordering.txt | 1 + src/test/overlay/TMGetObjectByHash_test.cpp | 211 ++++++++++++++++++ src/xrpld/overlay/detail/PeerImp.cpp | 14 +- 3 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 src/test/overlay/TMGetObjectByHash_test.cpp diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index c9c65fb0dd..8d17e1167f 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -104,6 +104,7 @@ test.overlay > xrpl.basics test.overlay > xrpld.app test.overlay > xrpld.overlay test.overlay > xrpld.peerfinder +test.overlay > xrpl.nodestore test.overlay > xrpl.protocol test.overlay > xrpl.shamap test.peerfinder > test.beast diff --git a/src/test/overlay/TMGetObjectByHash_test.cpp b/src/test/overlay/TMGetObjectByHash_test.cpp new file mode 100644 index 0000000000..71a485416d --- /dev/null +++ b/src/test/overlay/TMGetObjectByHash_test.cpp @@ -0,0 +1,211 @@ +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace xrpl { +namespace test { + +using namespace jtx; + +/** + * Test for TMGetObjectByHash reply size limiting. + * + * This verifies the fix that limits TMGetObjectByHash replies to + * Tuning::hardMaxReplyNodes to prevent excessive memory usage and + * potential DoS attacks from peers requesting large numbers of objects. + */ +class TMGetObjectByHash_test : public beast::unit_test::suite +{ + using middle_type = boost::beast::tcp_stream; + using stream_type = boost::beast::ssl_stream; + using socket_type = boost::asio::ip::tcp::socket; + using shared_context = std::shared_ptr; + /** + * Test peer that captures sent messages for verification. + */ + class PeerTest : public PeerImp + { + public: + PeerTest( + Application& app, + std::shared_ptr const& slot, + http_request_type&& request, + PublicKey const& publicKey, + ProtocolVersion protocol, + Resource::Consumer consumer, + std::unique_ptr&& stream_ptr, + OverlayImpl& overlay) + : PeerImp( + app, + id_++, + slot, + std::move(request), + publicKey, + protocol, + consumer, + std::move(stream_ptr), + overlay) + { + } + + ~PeerTest() = default; + + void + run() override + { + } + + void + send(std::shared_ptr const& m) override + { + lastSentMessage_ = m; + } + + std::shared_ptr + getLastSentMessage() const + { + return lastSentMessage_; + } + + static void + resetId() + { + id_ = 0; + } + + private: + inline static Peer::id_t id_ = 0; + std::shared_ptr lastSentMessage_; + }; + + shared_context context_{make_SSLContext("")}; + ProtocolVersion protocolVersion_{1, 7}; + + std::shared_ptr + createPeer(jtx::Env& env) + { + auto& overlay = dynamic_cast(env.app().overlay()); + boost::beast::http::request request; + auto stream_ptr = std::make_unique( + socket_type(env.app().getIOContext()), *context_); + + beast::IP::Endpoint local( + boost::asio::ip::make_address("172.1.1.1"), 51235); + beast::IP::Endpoint remote( + boost::asio::ip::make_address("172.1.1.2"), 51235); + + PublicKey key(std::get<0>(randomKeyPair(KeyType::ed25519))); + auto consumer = overlay.resourceManager().newInboundEndpoint(remote); + auto [slot, _] = overlay.peerFinder().new_inbound_slot(local, remote); + + auto peer = std::make_shared( + env.app(), + slot, + std::move(request), + key, + protocolVersion_, + consumer, + std::move(stream_ptr), + overlay); + + overlay.add_active(peer); + return peer; + } + + std::shared_ptr + createRequest(size_t const numObjects, Env& env) + { + // Store objects in the NodeStore that will be found during the query + auto& nodeStore = env.app().getNodeStore(); + + // Create and store objects + std::vector hashes; + hashes.reserve(numObjects); + for (int i = 0; i < numObjects; ++i) + { + uint256 hash(xrpl::sha512Half(i)); + hashes.push_back(hash); + + Blob data(100, static_cast(i % 256)); + nodeStore.store( + hotLEDGER, + std::move(data), + hash, + nodeStore.earliestLedgerSeq()); + } + + // Create a request with more objects than hardMaxReplyNodes + auto request = std::make_shared(); + request->set_type(protocol::TMGetObjectByHash_ObjectType_otLEDGER); + request->set_query(true); + + for (int i = 0; i < numObjects; ++i) + { + auto object = request->add_objects(); + object->set_hash(hashes[i].data(), hashes[i].size()); + object->set_ledgerseq(i); + } + return request; + } + + /** + * Test that reply is limited to hardMaxReplyNodes when more objects + * are requested than the limit allows. + */ + void + testReplyLimit(size_t const numObjects, int const expectedReplySize) + { + testcase("Reply Limit"); + + Env env(*this); + PeerTest::resetId(); + + auto peer = createPeer(env); + + auto request = createRequest(numObjects, env); + // Call the onMessage handler + peer->onMessage(request); + + // Verify that a reply was sent + auto sentMessage = peer->getLastSentMessage(); + BEAST_EXPECT(sentMessage != nullptr); + + // Parse the reply message + auto const& buffer = + sentMessage->getBuffer(compression::Compressed::Off); + + BEAST_EXPECT(buffer.size() > 6); + // Skip the message header (6 bytes: 4 for size, 2 for type) + protocol::TMGetObjectByHash reply; + BEAST_EXPECT( + reply.ParseFromArray(buffer.data() + 6, buffer.size() - 6) == true); + + // Verify the reply is limited to expectedReplySize + BEAST_EXPECT(reply.objects_size() == expectedReplySize); + } + + void + run() override + { + int const limit = static_cast(Tuning::hardMaxReplyNodes); + testReplyLimit(limit + 1, limit); + testReplyLimit(limit, limit); + testReplyLimit(limit - 1, limit - 1); + } +}; + +BEAST_DEFINE_TESTSUITE(TMGetObjectByHash, overlay, xrpl); + +} // namespace test +} // namespace xrpl diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 53237ed3ae..b64227288c 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1351,8 +1351,8 @@ PeerImp::handleTransaction( { // If we've never been in synch, there's nothing we can do // with a transaction - JLOG(p_journal_.debug()) << "Ignoring incoming transaction: " - << "Need network ledger"; + JLOG(p_journal_.debug()) + << "Ignoring incoming transaction: Need network ledger"; return; } @@ -2618,6 +2618,16 @@ PeerImp::onMessage(std::shared_ptr const& m) newObj.set_ledgerseq(obj.ledgerseq()); // VFALCO NOTE "seq" in the message is obsolete + + // Check if by adding this object, reply has reached its + // limit + if (reply.objects_size() >= Tuning::hardMaxReplyNodes) + { + fee_.update( + Resource::feeModerateBurdenPeer, + " Reply limit reached. Truncating reply."); + break; + } } } }