From 7912d112e752fb59833adc3b91560c551e2182b9 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sun, 3 May 2026 17:48:25 -0400 Subject: [PATCH] Address AI feedback --- src/libxrpl/shamap/SHAMapSync.cpp | 17 +-------- src/test/app/LedgerNodeHelpers_test.cpp | 35 +++++++++++++------ src/xrpld/app/ledger/detail/InboundLedger.cpp | 2 +- .../app/ledger/detail/InboundTransactions.cpp | 2 +- .../app/ledger/detail/LedgerNodeHelpers.cpp | 9 +++-- .../app/ledger/detail/LedgerNodeHelpers.h | 5 +-- .../app/ledger/detail/TransactionAcquire.cpp | 2 +- 7 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 93e9139ea7..ae422db940 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -512,12 +512,8 @@ SHAMap::addRootNode( SHAMapTreeNodePtr rootNode, SHAMapSyncFilter const* filter) { + XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid"); XRPL_ASSERT(rootNode, "xrpl::SHAMap::addRootNode : non-null root node"); - if (!rootNode) - { - JLOG(journal_.error()) << "Null node received"; - return SHAMapAddNode::invalid(); - } // we already have a root_ node if (root_->getHash().isNonZero()) @@ -527,7 +523,6 @@ SHAMap::addRootNode( return SHAMapAddNode::duplicate(); } - XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid"); if (rootNode->getHash() != hash) { JLOG(journal_.warn()) << "Corrupt node received"; @@ -560,17 +555,7 @@ SHAMap::addKnownNode( SHAMapSyncFilter const* filter) { XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node input"); - if (nodeID.isRoot()) - { - JLOG(journal_.error()) << "Root node received"; - return SHAMapAddNode::invalid(); - } XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node"); - if (!treeNode) - { - JLOG(journal_.error()) << "Null node received"; - return SHAMapAddNode::invalid(); - } if (!isSynching()) { diff --git a/src/test/app/LedgerNodeHelpers_test.cpp b/src/test/app/LedgerNodeHelpers_test.cpp index bcae354f55..0e614d1ad5 100644 --- a/src/test/app/LedgerNodeHelpers_test.cpp +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -44,11 +44,10 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite 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 file. testcase("validateLedgerNode"); + auto const validID = SHAMapNodeID::createID(3, uint256{}).getRawString(); + // Invalid: missing all fields. { protocol::TMLedgerNode const node; @@ -58,14 +57,14 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite // Invalid: missing `nodedata` field. { protocol::TMLedgerNode node; - node.set_nodeid("test_nodeid"); + node.set_nodeid(validID); BEAST_EXPECT(!validateLedgerNode(node)); } // Invalid: missing `nodedata` field. { protocol::TMLedgerNode node; - node.set_id("test_nodeid"); + node.set_id(validID); BEAST_EXPECT(!validateLedgerNode(node)); } @@ -80,16 +79,24 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite { protocol::TMLedgerNode node; node.set_nodedata("test_data"); - node.set_nodeid("test_nodeid"); + node.set_nodeid(validID); BEAST_EXPECT(validateLedgerNode(node)); } + // Invalid: legacy `nodeid` field with garbage content. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_nodeid("garbage"); + BEAST_EXPECT(!validateLedgerNode(node)); + } + // 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"); + node.set_nodeid(validID); + node.set_id(validID); BEAST_EXPECT(!validateLedgerNode(node)); } @@ -97,7 +104,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite { protocol::TMLedgerNode node; node.set_nodedata("test_data"); - node.set_nodeid("test_nodeid"); + node.set_nodeid(validID); node.set_depth(5); BEAST_EXPECT(!validateLedgerNode(node)); } @@ -106,10 +113,18 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite { protocol::TMLedgerNode node; node.set_nodedata("test_data"); - node.set_id("test_id"); + node.set_id(validID); BEAST_EXPECT(validateLedgerNode(node)); } + // Invalid: new `id` field with garbage content. + { + protocol::TMLedgerNode node; + node.set_nodedata("test_data"); + node.set_id("garbage"); + BEAST_EXPECT(!validateLedgerNode(node)); + } + // Valid: new `depth` field. { protocol::TMLedgerNode node; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 3174a9396f..01bf547dc0 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -1150,7 +1150,7 @@ InboundLedger::processData(std::shared_ptr peer, protocol::TMLedgerData& p if (!validateLedgerNode(ledgerNode)) { JLOG(journal_.warn()) << "Got malformed ledger node"; - peer->charge(Resource::kFEE_MALFORMED_REQUEST, "ledgerNode"); + peer->charge(Resource::kFEE_MALFORMED_REQUEST, "ledger_node"); return -1; } } diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index 9071097847..e29c6e1b4a 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -155,7 +155,7 @@ public: if (!validateLedgerNode(ledgerNode)) { JLOG(j_.warn()) << "Got malformed ledger node"; - peer->charge(Resource::kFEE_MALFORMED_REQUEST, "ledgerNode"); + peer->charge(Resource::kFEE_MALFORMED_REQUEST, "ledger_node"); return; } diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index 06feed2716..d00227a0cc 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -23,10 +23,13 @@ validateLedgerNode(protocol::TMLedgerNode const& ledgerNode) return false; if (ledgerNode.has_nodeid()) - return !ledgerNode.has_id() && !ledgerNode.has_depth(); + return !ledgerNode.has_id() && !ledgerNode.has_depth() && + deserializeSHAMapNodeID(ledgerNode.nodeid()).has_value(); - return ledgerNode.has_id() || - (ledgerNode.has_depth() && ledgerNode.depth() <= SHAMap::kLEAF_DEPTH); + if (ledgerNode.has_id()) + return deserializeSHAMapNodeID(ledgerNode.id()).has_value(); + + return ledgerNode.has_depth() && ledgerNode.depth() <= SHAMap::kLEAF_DEPTH; } SHAMapTreeNodePtr diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h index f727e059f7..7a89b09543 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h @@ -19,10 +19,11 @@ namespace xrpl { * This function checks whether a ledger node has the expected fields (for non-ledger base data): * - The node must have `nodedata`. * - If the legacy `nodeid` field is present then the new `id` and `depth` fields must not be - * present. + * present, and `nodeid` must be a valid serialized SHAMapNodeID. * - 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 0 and SHAMap::leafDepth (inclusive). + * - If the `id` field is present then it must be a valid serialized SHAMapNodeID. + * - If the `depth` field is present then it must be between 0 and SHAMap::kLEAF_DEPTH (inclusive). * * @param ledgerNode The ledger node to validate. * @return true if the ledger node has the expected fields, false otherwise. diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp index f8719f99e0..2a9d063608 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp @@ -199,7 +199,7 @@ TransactionAcquire::takeNodes( ConsensusTransSetSF sf(app_, app_.getTempNodeCache()); - for (auto const& d : data) + for (auto& d : data) { if (d.first.isRoot()) {