diff --git a/include/xrpl/proto/xrpl.proto b/include/xrpl/proto/xrpl.proto index ae50c4760b..0af7deb35d 100644 --- a/include/xrpl/proto/xrpl.proto +++ b/include/xrpl/proto/xrpl.proto @@ -244,8 +244,7 @@ message TMGetObjectByHash { message TMLedgerNode { required bytes nodedata = 1; - optional bytes nodeid = 2; // Used when fixLedgerNodeDepth is not active. - optional uint32 nodedepth = 3; // Used when fixLedgerNodeDepth is active. + optional bytes nodeid = 2; // missing for ledger base data } enum TMLedgerInfoType { diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 68fb3fbb29..f83a4b442d 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,7 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. -XRPL_FIX (LedgerNodeDepth, Supported::yes, VoteBehavior::DefaultYes) +XRPL_FIX (LedgerNodeID, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index e8840f398d..c0433e5327 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -113,7 +113,7 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash) SHAMapNodeID SHAMapNodeID::createID(int depth, uint256 const& key) { - XRPL_ASSERT((depth >= 0) && (depth < 65), "xrpl::SHAMapNodeID::createID : valid branch input"); + XRPL_ASSERT(depth >= 0 && depth <= SHAMap::leafDepth, "xrpl::SHAMapNodeID::createID : valid branch input"); return SHAMapNodeID(depth, key & depthMask(depth)); } diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index c968cb875e..42998ae2f7 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -830,28 +830,26 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) } auto const node_slice = makeSlice(ledger_node.nodedata()); - auto const tree_node_opt = getTreeNode(node_slice); - if (!tree_node_opt) + auto const tree_node = getTreeNode(node_slice); + if (!tree_node) { JLOG(journal_.warn()) << "Got invalid node data"; san.incInvalid(); return; } - auto const tree_node = *tree_node_opt; - auto const node_id_opt = getSHAMapNodeID(app_, ledger_node, tree_node); - if (!node_id_opt) + auto const node_id = getSHAMapNodeID(app_, ledger_node, *tree_node); + if (!node_id) { JLOG(journal_.warn()) << "Got invalid node id"; san.incInvalid(); return; } - auto const& node_id = *node_id_opt; - if (node_id.isRoot()) + if (node_id->isRoot()) san += map.addRootNode(rootHash, node_slice, f); else - san += map.addKnownNode(node_id, node_slice, f); + san += map.addKnownNode(*node_id, node_slice, f); if (!san.isGood()) { diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index ca3ad196d2..1d6ae72313 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -222,16 +222,16 @@ public: return; auto const node_slice = makeSlice(ledger_node.nodedata()); - auto const tree_node_opt = getTreeNode(node_slice); - if (!tree_node_opt) + auto const tree_node = getTreeNode(node_slice); + if (!tree_node) return; - auto const tree_node = *tree_node_opt; + auto const& tn = *tree_node; s.erase(); - tree_node->serializeWithPrefix(s); + tn->serializeWithPrefix(s); app_.getLedgerMaster().addFetchPack( - tree_node->getHash().as_uint256(), std::make_shared(s.begin(), s.end())); + tn->getHash().as_uint256(), std::make_shared(s.begin(), s.end())); } } catch (std::exception const&) diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index 50d089ccb2..396c0653bf 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -141,25 +141,23 @@ public: } auto const node_slice = makeSlice(ledger_node.nodedata()); - auto const tree_node_opt = getTreeNode(node_slice); - if (!tree_node_opt) + auto const tree_node = getTreeNode(node_slice); + if (!tree_node) { JLOG(j_.warn()) << "Got invalid node data"; peer->charge(Resource::feeInvalidData, "node_data"); return; } - auto const tree_node = *tree_node_opt; - auto const node_id_opt = getSHAMapNodeID(app_, ledger_node, tree_node); - if (!node_id_opt) + auto const node_id = getSHAMapNodeID(app_, ledger_node, *tree_node); + if (!node_id) { JLOG(j_.warn()) << "Got invalid node id"; peer->charge(Resource::feeInvalidData, "node_id"); return; } - auto const& node_id = *node_id_opt; - data.emplace_back(std::make_pair(node_id, node_slice)); + data.emplace_back(std::make_pair(*node_id, node_slice)); } if (!ta->takeNodes(data, peer).isUseful()) diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h index 3de1bcace6..82ae7bd4bd 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -16,9 +17,14 @@ validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node) if (!ledger_node.has_nodedata()) return false; - if (app.getAmendmentTable().isEnabled(fixLedgerNodeDepth)) - return ledger_node.has_nodedepth() && ledger_node.nodedepth() < 65; + // When the amendment is enabled, we expect the node ID to be present in the ledger node for + // inner nodes, while we expect it to not be present in the ledger node for leaf nodes. However, + // at this point we don't yet know whether the node is an inner node or a leaf node, so we + // allow both cases. + if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) + return true; + // When the amendment is disabled, we expect the node ID to always be present. return ledger_node.has_nodeid(); } @@ -41,26 +47,48 @@ getSHAMapNodeID( protocol::TMLedgerNode const& ledger_node, intr_ptr::SharedPtr const& tree_node) { - SHAMapNodeID node_id; - if (app.getAmendmentTable().isEnabled(fixLedgerNodeDepth)) + // When the amendment is enabled, we can get the node ID directly from the ledger node for inner + // nodes, while we compute it for leaf nodes. + if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) { - node_id = SHAMapNodeID(ledger_node.nodedepth(), tree_node->getHash().as_uint256()); - } - else - { - auto const nid = deserializeSHAMapNodeID(ledger_node.nodeid()); - if (!nid) - return std::nullopt; - node_id = *nid; + if (tree_node->isInner()) + { + XRPL_ASSERT(ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is present"); + if (!ledger_node.has_nodeid()) + return std::nullopt; + + return deserializeSHAMapNodeID(ledger_node.nodeid()); + } + + if (tree_node->isLeaf()) + { + XRPL_ASSERT(!ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is not present"); + if (ledger_node.has_nodeid()) + return std::nullopt; + + auto const key = static_cast(tree_node.get())->peekItem()->key(); + return SHAMapNodeID::createID(SHAMap::leafDepth, key); + } + + return std::nullopt; } - // For leaf nodes, verify that the node ID is actually the same as what the node ID should be, - // given the position of the node in the SHAMap. + // 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; + + auto const node_id = deserializeSHAMapNodeID(ledger_node.nodeid()); + if (!node_id) + return std::nullopt; + if (tree_node->isLeaf()) { - auto const nodeKey = dynamic_cast(tree_node.get())->peekItem()->key(); - auto const expectedID = SHAMapNodeID::createID(static_cast(node_id.getDepth()), nodeKey); - if (node_id.getNodeID() != expectedID.getNodeID()) + auto const key = static_cast(tree_node.get())->peekItem()->key(); + auto const expected_id = SHAMapNodeID::createID(static_cast(node_id->getDepth()), key); + if (node_id->getNodeID() != expected_id.getNodeID()) return std::nullopt; } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 9c596317c5..9f92c75060 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -3154,9 +3154,12 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) break; protocol::TMLedgerNode* node{ledgerData.add_nodes()}; node->set_nodedata(d.second.data(), d.second.size()); - if (app_.getAmendmentTable().isEnabled(fixLedgerNodeDepth)) - node->set_nodedepth(d.first.getDepth()); - else + // When the amendment is enabled, we only include the node ID in the ledger node for inner + // nodes, while we do not include it for leaf nodes as it can be calculated from the data. When + // the amendment is disabled, we always need to include the node ID in the ledger node. + if ((app_.getAmendmentTable().isEnabled(fixLedgerNodeID) && + d.first.getDepth() != SHAMap::leafDepth) || + !app_.getAmendmentTable().isEnabled(fixLedgerNodeID)) node->set_nodeid(d.first.getRawString()); } }