From 29b0076fa89189f227fe19c00033f0ee4abbf533 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 28 Feb 2026 13:54:00 -0500 Subject: [PATCH] Use new protocol version instead of amendment, add tests --- include/xrpl/proto/xrpl.proto | 4 +- include/xrpl/protocol/detail/features.macro | 1 - src/test/app/LedgerNodeHelpers_test.cpp | 525 ++++++++++-------- src/xrpld/app/ledger/detail/InboundLedger.cpp | 6 +- .../app/ledger/detail/InboundLedgers.cpp | 2 +- .../app/ledger/detail/InboundTransactions.cpp | 4 +- .../app/ledger/detail/LedgerNodeHelpers.cpp | 41 +- .../app/ledger/detail/LedgerNodeHelpers.h | 81 ++- src/xrpld/overlay/Peer.h | 1 + src/xrpld/overlay/detail/PeerImp.cpp | 10 +- src/xrpld/overlay/detail/ProtocolVersion.cpp | 3 +- 11 files changed, 366 insertions(+), 312 deletions(-) diff --git a/include/xrpl/proto/xrpl.proto b/include/xrpl/proto/xrpl.proto index 5b417fe48c..d17ac28714 100644 --- a/include/xrpl/proto/xrpl.proto +++ b/include/xrpl/proto/xrpl.proto @@ -245,10 +245,10 @@ message TMGetObjectByHash { message TMLedgerNode { required bytes nodedata = 1; - // Used when fixLedgerNodeID is disabled. + // Used when protocol version <2.3. optional bytes nodeid = 2; // missing for ledger base data - // Used when fixLedgerNodeID is enabled. Neither value is set for ledger base data. + // Used when protocol version >=2.3. Neither value is set for ledger base data. oneof reference { bytes id = 3; // Set for inner nodes. uint32 depth = 4; // Set for leaf nodes. diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index c0c1c387db..bc781afaac 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,7 +15,6 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. -XRPL_FIX (LedgerNodeID, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::no, VoteBehavior::DefaultNo) diff --git a/src/test/app/LedgerNodeHelpers_test.cpp b/src/test/app/LedgerNodeHelpers_test.cpp index 540e48395b..a1ecd60873 100644 --- a/src/test/app/LedgerNodeHelpers_test.cpp +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -1,24 +1,24 @@ #include #include #include -#include #include -#include #include -#include +#include #include +#include +#include #include -#include +#include namespace xrpl { namespace tests { class LedgerNodeHelpers_test : public beast::unit_test::suite { - // Helper to create a simple SHAMapItem for testing - boost::intrusive_ptr + // Helper function to create a simple SHAMapItem for testing. + static boost::intrusive_ptr makeTestItem(std::uint32_t seed) { Serializer s; @@ -28,152 +28,181 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite return make_shamapitem(s.getSHA512Half(), s.slice()); } - // Helper to serialize a tree node to wire format - std::string + // Helper function to serialize a tree node to wire format. + static std::string serializeNode(intr_ptr::SharedPtr const& node) { Serializer s; - node->serializeWithPrefix(s); + node->serializeForWire(s); auto const slice = s.slice(); - return std::string(reinterpret_cast(slice.data()), slice.size()); + return std::string(std::bit_cast(slice.data()), slice.size()); } void testValidateLedgerNode() { + // In the tests below the validity of the content of the node data and ID fields is not + // checked - only that the fields have values when expected. The content of the fields is + // verified in the other tests in this field. testcase("validateLedgerNode"); - using namespace test::jtx; - - /*// Test with amendment disabled. + // Invalid: missing all fields. { - Env env{*this, testable_amendments() - fixLedgerNodeID}; - auto& app = env.app(); - - // Valid node with nodeid. - protocol::TMLedgerNode node1; - node1.set_nodedata("test_data"); - node1.set_nodeid("test_nodeid"); - BEAST_EXPECT(validateLedgerNode(app, node1)); - - // Invalid: missing nodedata. - protocol::TMLedgerNode node2; - node2.set_nodeid("test_nodeid"); - BEAST_EXPECT(!validateLedgerNode(app, node2)); - - // Invalid: has new field (id). - protocol::TMLedgerNode node3; - node3.set_nodedata("test_data"); - node3.set_nodeid("test_nodeid"); - node3.set_id("test_id"); - BEAST_EXPECT(!validateLedgerNode(app, node3)); - - // Invalid: has new field (depth). - protocol::TMLedgerNode node4; - node4.set_nodedata("test_data"); - node4.set_nodeid("test_nodeid"); - node4.set_depth(5); - BEAST_EXPECT(!validateLedgerNode(app, node4)); + protocol::TMLedgerNode node; + BEAST_EXPECT(!validateLedgerNode(node)); } - // Test with amendment enabled. + // Invalid: missing `nodedata` field. { - Env env{*this, testable_amendments() | fixLedgerNodeID}; - auto& app = env.app(); + protocol::TMLedgerNode node; + node.set_nodeid("test_nodeid"); + BEAST_EXPECT(!validateLedgerNode(node)); + } - // Valid inner node with id. - protocol::TMLedgerNode node1; - node1.set_nodedata("test_data"); - node1.set_id("test_id"); - BEAST_EXPECT(validateLedgerNode(app, node1)); + // Invalid: missing `nodedata` field. + { + protocol::TMLedgerNode node; + node.set_id("test_nodeid"); + BEAST_EXPECT(!validateLedgerNode(node)); + } - // Valid leaf node with depth. - protocol::TMLedgerNode node2; - node2.set_nodedata("test_data"); - node2.set_depth(5); - BEAST_EXPECT(validateLedgerNode(app, node2)); + // Invalid: missing `nodedata` field. + { + protocol::TMLedgerNode node; + node.set_depth(1); + BEAST_EXPECT(!validateLedgerNode(node)); + } - // Valid leaf node at max depth. - protocol::TMLedgerNode node3; - node3.set_nodedata("test_data"); - node3.set_depth(SHAMap::leafDepth); - BEAST_EXPECT(validateLedgerNode(app, node3)); + // Valid: legacy `nodeid` field. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_nodeid("test_nodeid"); + BEAST_EXPECT(validateLedgerNode(node)); + } - // Invalid: depth exceeds max depth. - protocol::TMLedgerNode node4; - node4.set_nodedata("test_data"); - node4.set_depth(SHAMap::leafDepth + 1); - BEAST_EXPECT(!validateLedgerNode(app, node4)); + // Invalid: has both legacy `nodeid` and new `id` fields. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_nodeid("test_nodeid"); + node.set_id("test_nodeid"); + BEAST_EXPECT(!validateLedgerNode(node)); + } - // Invalid: has legacy field (nodeid). - protocol::TMLedgerNode node5; - node5.set_nodedata("test_data"); - node5.set_nodeid("test_nodeid"); - BEAST_EXPECT(!validateLedgerNode(app, node5)); + // Invalid: has both legacy `nodeid` and new `depth` fields. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_nodeid("test_nodeid"); + node.set_depth(5); + BEAST_EXPECT(!validateLedgerNode(node)); + } - // Invalid: missing both id and depth. - protocol::TMLedgerNode node6; - node6.set_nodedata("test_data"); - BEAST_EXPECT(!validateLedgerNode(app, node6)); + // Valid: new `id` feld. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_id("test_id"); + BEAST_EXPECT(validateLedgerNode(node)); + } - // Invalid: missing nodedata. - protocol::TMLedgerNode node7; - node7.set_id("test_id"); - BEAST_EXPECT(!validateLedgerNode(app, node7)); - }*/ + // Valid: new `depth` field. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_depth(5); + BEAST_EXPECT(validateLedgerNode(node)); + } + + // Invalid: `depth` is less than minimum depth. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_depth(0); + BEAST_EXPECT(!validateLedgerNode(node)); + } + + // Valid: `depth` at minimum depth. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_depth(1); + BEAST_EXPECT(validateLedgerNode(node)); + } + + // Valid: `depth` at arbitrary depth between minimum and maximum. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_depth(10); + BEAST_EXPECT(validateLedgerNode(node)); + } + + // Valid: `depth` at maximum depth. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_depth(SHAMap::leafDepth); + BEAST_EXPECT(validateLedgerNode(node)); + } + + // Invalid: `depth` is greater than maximum depth. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_depth(SHAMap::leafDepth + 1); + BEAST_EXPECT(!validateLedgerNode(node)); + } } - /*void + void testGetTreeNode() { testcase("getTreeNode"); - using namespace test::jtx; - test::SuiteJournal journal("LedgerNodeHelpers_test", *this); - TestNodeFamily f(journal); - - // Test with valid inner node + // Valid: inner node. { - auto innerNode = std::make_shared(1); - auto serialized = serializeNode(innerNode); - auto result = getTreeNode(serialized); + auto const rootNode = intr_ptr::make_shared(1); + auto const rootData = serializeNode(rootNode); + auto const result = getTreeNode(rootData); BEAST_EXPECT(result.has_value()); BEAST_EXPECT((*result)->isInner()); } - // Test with valid leaf node + // Valid: leaf node. { - auto item = makeTestItem(12345); - auto leafNode = - std::make_shared(item, SHAMapNodeType::tnACCOUNT_STATE); - auto serialized = serializeNode(leafNode); - auto result = getTreeNode(serialized); + auto const leafItem = makeTestItem(12345); + auto const leafNode = + intr_ptr::make_shared(std::move(leafItem), 1); + auto const leafData = serializeNode(leafNode); + auto result = getTreeNode(leafData); BEAST_EXPECT(result.has_value()); BEAST_EXPECT((*result)->isLeaf()); } - // Test with invalid data - empty string + // Invalid: empty data. { - auto result = getTreeNode(""); + auto const result = getTreeNode(""); BEAST_EXPECT(!result.has_value()); } - // Test with invalid data - garbage data + // Invalid: garbage data. { - std::string garbage = "This is not a valid serialized node!"; - auto result = getTreeNode(garbage); + auto const result = getTreeNode("invalid"); BEAST_EXPECT(!result.has_value()); } - // Test with malformed data - truncated + // Invalid: truncated data. { - auto item = makeTestItem(54321); - auto leafNode = - std::make_shared(item, SHAMapNodeType::tnACCOUNT_STATE); - auto serialized = serializeNode(leafNode); - // Truncate the data - serialized = serialized.substr(0, 5); - auto result = getTreeNode(serialized); + auto const leafItem = makeTestItem(54321); + auto const leafNode = + intr_ptr::make_shared(std::move(leafItem), 1); + // Truncate the data to trigger an exception in SHAMapTreeNode::makeAccountState when + // the data is used to deserialize the node. + uint256 tag; + auto const leafData = serializeNode(leafNode).substr(0, tag.bytes - 1); + auto const result = getTreeNode(leafData); BEAST_EXPECT(!result.has_value()); } } @@ -183,166 +212,206 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite { testcase("getSHAMapNodeID"); - using namespace test::jtx; - test::SuiteJournal journal("LedgerNodeHelpers_test", *this); - TestNodeFamily f(journal); - - auto item = makeTestItem(99999); - auto leafNode = - std::make_shared(item, SHAMapNodeType::tnACCOUNT_STATE); - auto innerNode = std::make_shared(1); - - // Test with amendment disabled { - Env env{*this}; - auto& app = env.app(); + // Tests using a root node (=inner node at depth 0). + auto const rootNode = intr_ptr::make_shared(1); + auto const rootData = serializeNode(rootNode); + auto const rootNodeID = SHAMapNodeID{}; - // Test with leaf node - valid nodeid + // Valid: legacy `nodeid` field. { - auto nodeID = SHAMapNodeID::createID(5, item->key()); - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_nodeid(nodeID.getRawString()); - - auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + protocol::TMLedgerNode node; + node.set_nodedata(rootData); + node.set_nodeid(rootNodeID.getRawString()); + auto const result = getSHAMapNodeID(node, rootNode); BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == nodeID); + BEAST_EXPECT(*result == rootNodeID); } - // Test with leaf node - invalid nodeid (wrong depth) + // Valid: new `id` field. { - auto nodeID = SHAMapNodeID::createID(7, item->key()); - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_nodeid(nodeID.getRawString()); + protocol::TMLedgerNode node; + node.set_nodedata(rootData); + node.set_id(rootNodeID.getRawString()); + auto const result = getSHAMapNodeID(node, rootNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == rootNodeID); + } - auto wrongNodeID = SHAMapNodeID::createID(5, item->key()); - ledgerNode.set_nodeid(wrongNodeID.getRawString()); - - auto result = getSHAMapNodeID(app, ledgerNode, leafNode); - // Should fail because nodeid doesn't match the leaf's key at the - // given depth + // Invalid: Missing `nodeid` or `id` field. + { + protocol::TMLedgerNode node; + node.set_nodedata(rootData); + auto const result = getSHAMapNodeID(node, rootNode); BEAST_EXPECT(!result.has_value()); } - // Test with inner node + // Invalid: `depth` field is present but should not be used for inner nodes. { - auto nodeID = SHAMapNodeID::createID(3, uint256{}); - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_nodeid(nodeID.getRawString()); - - auto result = getSHAMapNodeID(app, ledgerNode, innerNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == nodeID); - } - - // Test missing nodeid - { - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - - auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + protocol::TMLedgerNode node; + node.set_nodedata(rootData); + node.set_depth(0); + auto const result = getSHAMapNodeID(node, rootNode); BEAST_EXPECT(!result.has_value()); } } - // Test with amendment enabled { - Env env{*this, testable_amendments() | fixLedgerNodeID}; - auto& app = env.app(); + // Tests using an inner node at arbitrary depth. + auto const innerNode = intr_ptr::make_shared(1); + auto const innerData = serializeNode(innerNode); + auto const innerDepth = 3; + auto const innerNodeID = SHAMapNodeID::createID(innerDepth, uint256{}); - // Test with leaf node - valid depth + // Valid: legacy `nodeid` field. { - std::uint32_t depth = 5; - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_depth(depth); - - auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + protocol::TMLedgerNode node; + node.set_nodedata(innerData); + node.set_nodeid(innerNodeID.getRawString()); + auto const result = getSHAMapNodeID(node, innerNode); BEAST_EXPECT(result.has_value()); - auto expectedID = SHAMapNodeID::createID(depth, item->key()); - BEAST_EXPECT(*result == expectedID); + BEAST_EXPECT(*result == innerNodeID); } - // Test with leaf node - missing depth + // Valid: new `id` field. { - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); + protocol::TMLedgerNode node; + node.set_nodedata(innerData); + node.set_id(innerNodeID.getRawString()); + auto const result = getSHAMapNodeID(node, innerNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == innerNodeID); + } - auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + // Invalid: Missing `nodeid` or `id` field. + { + protocol::TMLedgerNode node; + node.set_nodedata(innerData); + auto const result = getSHAMapNodeID(node, innerNode); BEAST_EXPECT(!result.has_value()); } - // Test with inner node - valid id + // Invalid: new `depth` field should not be used for inner nodes. { - auto nodeID = SHAMapNodeID::createID(3, uint256{}); - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_id(nodeID.getRawString()); - - auto result = getSHAMapNodeID(app, ledgerNode, innerNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == nodeID); - } - - // Test with inner node - missing id - { - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - - auto result = getSHAMapNodeID(app, ledgerNode, innerNode); + protocol::TMLedgerNode node; + node.set_nodedata(innerData); + node.set_depth(innerDepth); + auto const result = getSHAMapNodeID(node, innerNode); BEAST_EXPECT(!result.has_value()); } - - // Test that nodeid is rejected when amendment is enabled - { - auto nodeID = SHAMapNodeID::createID(5, item->key()); - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_nodeid(nodeID.getRawString()); - - // Should fail validation before getSHAMapNodeID is called, - // but let's verify getSHAMapNodeID behavior - // Note: validateLedgerNode should be called first in practice - BEAST_EXPECT(!validateLedgerNode(app, ledgerNode)); - } - - // Test with root node (inner node at depth 0) - { - auto rootNode = std::make_shared(1); - auto nodeID = SHAMapNodeID{}; // root node ID - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_id(nodeID.getRawString()); - - auto result = getSHAMapNodeID(app, ledgerNode, rootNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == nodeID); - } - - // Test with leaf at maximum depth - { - std::uint32_t depth = SHAMap::leafDepth; - protocol::TMLedgerNode ledgerNode; - ledgerNode.set_nodedata("test_data"); - ledgerNode.set_depth(depth); - - auto result = getSHAMapNodeID(app, ledgerNode, leafNode); - BEAST_EXPECT(result.has_value()); - auto expectedID = SHAMapNodeID::createID(depth, item->key()); - BEAST_EXPECT(*result == expectedID); - } } - }*/ + + { + // Tests using leaf nodes at various depths, but with the same key. + auto const leafItem = makeTestItem(12345); + auto const leafNode = intr_ptr::make_shared(leafItem, 1); + auto const leafData = serializeNode(leafNode); + auto const leafKey = leafItem->key(); + + // Valid: legacy `nodeid` field at arbitrary depth. + { + auto const leafDepth = 5; + auto const leafID = SHAMapNodeID::createID(leafDepth, leafKey); + + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(leafData); + ledgerNode.set_nodeid(leafID.getRawString()); + auto result = getSHAMapNodeID(ledgerNode, leafNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == leafID); + } + + // Invalid: new `id` field should not be used for leaf nodes. + { + auto const leafDepth = 5; + auto const leafID = SHAMapNodeID::createID(leafDepth, leafKey); + + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(leafData); + ledgerNode.set_id(leafID.getRawString()); + auto result = getSHAMapNodeID(ledgerNode, leafNode); + BEAST_EXPECT(!result.has_value()); + } + + // Valid: new `depth` field at minimum depth. + // Note that we do not test a depth less than the minimum depth, because the proto + // message is assumed to have been validated by the time the getSHAMapNodeID function is + // called. + { + auto const leafDepth = 1; + auto const leafID = SHAMapNodeID::createID(leafDepth, leafKey); + + protocol::TMLedgerNode node; + node.set_nodedata(leafData); + node.set_depth(leafDepth); + auto result = getSHAMapNodeID(node, leafNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == leafID); + } + + // Valid: new `depth` field at arbitrary depth between minimum and maximum. + { + auto const leafDepth = 10; + auto const leafID = SHAMapNodeID::createID(leafDepth, leafKey); + + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(leafData); + ledgerNode.set_depth(leafDepth); + auto result = getSHAMapNodeID(ledgerNode, leafNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == leafID); + } + + // Valid: new `depth` field at maximum depth. + // Note that we do not test a depth greater than the maximum depth, because the proto + // message is assumed to have been validated by the time the getSHAMapNodeID function is + // called. + { + auto const leafDepth = SHAMap::leafDepth; + auto const leafID = SHAMapNodeID::createID(leafDepth, leafKey); + + protocol::TMLedgerNode node; + node.set_nodedata(leafData); + node.set_depth(leafDepth); + auto result = getSHAMapNodeID(node, leafNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == leafID); + } + + // Invalid: Missing `depth` field. + { + protocol::TMLedgerNode node; + node.set_nodedata(leafData); + auto const result = getSHAMapNodeID(node, leafNode); + BEAST_EXPECT(!result.has_value()); + } + + // Invalid: legacy `nodeid` field where the node ID is inconsistent with the key. + { + auto const otherItem = makeTestItem(54321); + auto const otherNode = + intr_ptr::make_shared(otherItem, 1); + auto const otherData = serializeNode(otherNode); + auto const otherKey = otherItem->key(); + auto const otherDepth = 1; + auto const otherID = SHAMapNodeID::createID(otherDepth, otherKey); + + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(otherData); + ledgerNode.set_nodeid(otherID.getRawString()); + auto result = getSHAMapNodeID(ledgerNode, leafNode); + BEAST_EXPECT(!result.has_value()); + } + } + } public: void run() override { testValidateLedgerNode(); - // testGetTreeNode(); - // testGetSHAMapNodeID(); + testGetTreeNode(); + testGetSHAMapNodeID(); } }; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 2b4eab0496..8f5b177da1 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -840,7 +840,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) for (auto const& ledger_node : packet.nodes()) { - if (!validateLedgerNode(app_, ledger_node)) + if (!validateLedgerNode(ledger_node)) { JLOG(journal_.warn()) << "Got malformed ledger node"; san.incInvalid(); @@ -855,7 +855,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) return; } - auto const nodeID = getSHAMapNodeID(app_, ledger_node, *treeNode); + auto const nodeID = getSHAMapNodeID(ledger_node, *treeNode); if (!nodeID) { JLOG(journal_.warn()) << "Got invalid node id"; @@ -1092,7 +1092,7 @@ InboundLedger::processData(std::shared_ptr peer, protocol::TMLedgerData& p // Verify nodes are complete for (auto const& ledger_node : packet.nodes()) { - if (!validateLedgerNode(app_, ledger_node)) + if (!validateLedgerNode(ledger_node)) { JLOG(journal_.warn()) << "Got malformed ledger node"; peer->charge(Resource::feeMalformedRequest, "ledger_node"); diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 349aacbcb5..e0b91fc116 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -223,7 +223,7 @@ public: Serializer s; for (auto const& ledger_node : packet_ptr->nodes()) { - if (!validateLedgerNode(app_, ledger_node)) + if (!validateLedgerNode(ledger_node)) return; auto const treeNode = getTreeNode(ledger_node.nodedata()); diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index b40ea8616b..f8c5239644 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -138,7 +138,7 @@ public: for (auto const& ledger_node : packet.nodes()) { - if (!validateLedgerNode(app_, ledger_node)) + if (!validateLedgerNode(ledger_node)) { JLOG(j_.warn()) << "Got malformed ledger node"; peer->charge(Resource::feeMalformedRequest, "ledger_node"); @@ -153,7 +153,7 @@ public: return; } - auto const nodeID = getSHAMapNodeID(app_, ledger_node, *treeNode); + auto const nodeID = getSHAMapNodeID(ledger_node, *treeNode); if (!nodeID) { JLOG(j_.warn()) << "Got invalid node id"; diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index 1a852390fb..c01cf6ee85 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -1,36 +1,31 @@ #include -#include #include +#include #include -#include +#include +#include #include #include #include +#include +#include + namespace xrpl { bool -validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node) +validateLedgerNode(protocol::TMLedgerNode const& ledger_node) { if (!ledger_node.has_nodedata()) return false; - if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) - { - // Note that we cannot confirm here whether the node is actually an - // inner or leaf node, and will need to perform additional checks - // separately. - if (ledger_node.has_nodeid()) - return false; - if (ledger_node.has_id()) - return true; - return ledger_node.has_depth() && ledger_node.depth() <= SHAMap::leafDepth; - } + if (ledger_node.has_nodeid()) + return !ledger_node.has_id() && !ledger_node.has_depth(); - if (ledger_node.has_id() || ledger_node.has_depth()) - return false; - return ledger_node.has_nodeid(); + return ledger_node.has_id() || + (ledger_node.has_depth() && ledger_node.depth() >= 1 && + ledger_node.depth() <= SHAMap::leafDepth); } std::optional> @@ -39,7 +34,10 @@ getTreeNode(std::string const& data) auto const slice = makeSlice(data); try { - return SHAMapTreeNode::makeFromWire(slice); + auto treeNode = SHAMapTreeNode::makeFromWire(slice); + if (!treeNode) + return std::nullopt; + return treeNode; } catch (std::exception const&) { @@ -49,12 +47,10 @@ getTreeNode(std::string const& data) std::optional getSHAMapNodeID( - Application& app, protocol::TMLedgerNode const& ledger_node, intr_ptr::SharedPtr const& treeNode) { - // When the amendment is enabled and a node depth is present, we can calculate the node ID. - if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) + if (ledger_node.has_id() || ledger_node.has_depth()) { if (treeNode->isInner()) { @@ -79,9 +75,6 @@ getSHAMapNodeID( return std::nullopt; } - // When the amendment is disabled, we expect the node ID to always be present. For leaf nodes - // we perform an extra check to ensure the node's position in the tree is consistent with its - // content. XRPL_ASSERT(ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is present"); if (!ledger_node.has_nodeid()) return std::nullopt; diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h index 7dd0dd3511..8dea158a52 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h @@ -1,82 +1,71 @@ #pragma once -#include - #include -#include #include #include +#include + +namespace protocol { +class TMLedgerNode; +} // namespace protocol + namespace xrpl { /** - * @brief Validates a ledger node based on the fixLedgerNodeID amendment status. + * @brief Validates a ledger node proto message. * - * This function checks whether a ledger node has the expected fields based on - * whether the fixLedgerNodeID amendment is enabled. The validation rules differ - * depending on the amendment state: - * - * When the amendment is enabled: + * This function checks whether a ledger node has the expected fields: * - The node must have `nodedata`. - * - The legacy `nodeid` field must NOT be present. - * - For inner nodes: the `id` field must be present. - * - For leaf nodes: the `depth` field must be present and <= SHAMap::leafDepth. + * - If the legacy `nodeid` field is present then the new `id` and `depth` fields must not be + * present. + * - If the new `id` or `depth` fields are present (it is a oneof field, so only one of the two can + * be set) then the legacy `nodeid` must not be present. + * - If the `depth` field is present then it must be between 1 and SHAMap::leafDepth (inclusive). * - * When the amendment is disabled: - * - The node must have `nodedata`. - * - The `nodeid` field must be present. - * - The new `id` and `depth` fields must NOT be present. - * - * @param app The application instance used to check amendment status. * @param ledger_node The ledger node to validate. * @return true if the ledger node has the expected fields, false otherwise. */ bool -validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node); +validateLedgerNode(protocol::TMLedgerNode const& ledger_node); /** * @brief Deserializes a SHAMapTreeNode from wire format data. * - * This function attempts to create a SHAMapTreeNode from the provided data - * string. If the data is malformed or deserialization fails, the function - * returns std::nullopt instead of throwing an exception. + * This function attempts to create a SHAMapTreeNode from the provided data string. If the data is + * malformed or deserialization fails, the function returns std::nullopt instead of throwing an + * exception. * * @param data The serialized node data in wire format. - * @return An optional containing the deserialized tree node if successful, or - * std::nullopt if deserialization fails. + * @return An optional containing the deserialized tree node if successful, or std::nullopt if + * deserialization fails. */ -std::optional> +[[nodiscard]] std::optional> getTreeNode(std::string const& data); /** - * @brief Extracts or reconstructs the SHAMapNodeID from a ledger node. + * @brief Extracts or reconstructs the SHAMapNodeID from a ledger node proto message. * - * This function retrieves the SHAMapNodeID for a tree node, with behavior that - * depends on the fixLedgerNodeID amendment status and the node type (inner vs. - * leaf). + * This function retrieves the SHAMapNodeID for a tree node, with behavior that depends on which + * field is set and the node type (inner vs. leaf). * - * When the fixLedgerNodeID amendment is enabled: - * - For inner nodes: Deserializes the node ID from the `ledger_node.id` field. - * Note that root nodes are also inner nodes. - * - For leaf nodes: Reconstructs the node ID using both the depth from the - * `ledger_node.depth` field and the key from the leaf node's item. + * When the legacy `nodeid` field is set in the message: + * - For all nodes: Deserializes the node ID from the field. + * - For leaf nodes: Validates that the node ID is consistent with the leaf's key. * - * When the amendment is disabled: - * - For all nodes: Deserializes the node ID from the `ledger_node.nodeid` - * field. - * - For leaf nodes: Validates that the node ID is consistent with the leaf's - * key. + * When the new `id` or `depth` field is set in the message: + * - For inner nodes: Deserializes the node ID from the `id` field. Note that root nodes are also + * inner nodes. + * - For leaf nodes: Reconstructs the node ID using both the depth from the `depth` field and the + * key from the leaf node's item. * - * @param app The application instance used to check amendment status. - * @param ledger_node The protocol message containing the ledger node data. + * @param ledger_node The validated protocol message containing the ledger node data. * @param treeNode The deserialized tree node (inner or leaf node). - * @return An optional containing the node ID if extraction/reconstruction - * succeeds, or std::nullopt if the required fields are missing or - * validation fails. + * @return An optional containing the node ID if extraction/reconstruction succeeds, or std::nullopt + * if the required fields are missing or validation fails. */ -std::optional +[[nodiscard]] std::optional getSHAMapNodeID( - Application& app, protocol::TMLedgerNode const& ledger_node, intr_ptr::SharedPtr const& treeNode); diff --git a/src/xrpld/overlay/Peer.h b/src/xrpld/overlay/Peer.h index a0e4c040fd..84aa241f63 100644 --- a/src/xrpld/overlay/Peer.h +++ b/src/xrpld/overlay/Peer.h @@ -17,6 +17,7 @@ enum class ProtocolFeature { ValidatorListPropagation, ValidatorList2Propagation, LedgerReplay, + LedgerNodeDepth, }; /** Represents a peer connection in the overlay. */ diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 48a9ea1c5b..33892e2be1 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -472,6 +472,8 @@ PeerImp::supportsFeature(ProtocolFeature f) const return protocol_ >= make_protocol(2, 1); case ProtocolFeature::ValidatorList2Propagation: return protocol_ >= make_protocol(2, 2); + case ProtocolFeature::LedgerNodeDepth: + return protocol_ >= make_protocol(2, 3); case ProtocolFeature::LedgerReplay: return ledgerReplayEnabled_; } @@ -3266,11 +3268,11 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) auto const& node_data = std::get<1>(d); node->set_nodedata(node_data.data(), node_data.size()); - // When the amendment is disabled, we always set the node ID. However, when - // the amendment is enabled, we only set it for inner nodes, while for leaf - // nodes we set the node depth instead. + // When the LedgerNode protocol feature is not supported by the peer, we + // always set the node ID. However, when it is supported then we only set it + // for inner nodes, while for leaf nodes we set the node depth instead. auto const& nodeID = std::get<0>(d); - if (!app_.getAmendmentTable().isEnabled(fixLedgerNodeID)) + if (!supportsFeature(ProtocolFeature::LedgerNodeDepth)) node->set_nodeid(nodeID.getRawString()); else if (std::get<2>(d)) node->set_depth(nodeID.getDepth()); diff --git a/src/xrpld/overlay/detail/ProtocolVersion.cpp b/src/xrpld/overlay/detail/ProtocolVersion.cpp index 3163ba55e0..35225fc63b 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.cpp +++ b/src/xrpld/overlay/detail/ProtocolVersion.cpp @@ -21,7 +21,8 @@ namespace xrpl { constexpr ProtocolVersion const supportedProtocolList[] { {2, 1}, - {2, 2} + {2, 2}, + {2, 3}, }; // clang-format on