perf: Replace node ID by depth in `TMLedgerNode` #6353

bthomee · 2026-02-11T21:12:24Z

High Level Overview of Change

This change introduces a new fix amendment, fixLedgerNodeDepth, which replaces the 32-byte node ID in the TMLedgerNode proto message by a 2-byte node depth instead (1 byte for the field tag, 1 byte for the depth as it won't exceed 127).

Context of Change

An earlier bug allowed a compromised UNL validator to send a maliciously crafted TMLedgerNode message with a node ID that didn't match the actual node ID corresponding to where the node data was located in the SHAMap. That bug was fixed by adding an extra check, which reconstructs the node ID from the node data and its depth. Hence, we don't actually need the node ID, and the much smaller node depth suffices. The extra check is now moved to the places where the messages are received from a peer, avoiding an expensive SHAMapTreeNode::makeFromWire call in SHAMapSync.

Although this change marks the nodeid field as deprecated, we cannot just ignore it as nodes running older releases won't be able to understand the new field. This change therefore requires an amendment, named fixLedgerNodeDepth.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Test Plan

As none of these files ever had tests, they need to be added.

Copilot

Pull request overview

This PR introduces a new fix amendment (fixLedgerNodeDepth) intended to reduce TMLedgerNode message size by replacing the serialized SHAMapNodeID (nodeid) with a smaller nodedepth field, and it relocates a leaf-position integrity check from SHAMapSync::addKnownNode to the peer-ingress paths.

Changes:

Add fixLedgerNodeDepth amendment and extend TMLedgerNode proto with nodedepth (deprecating nodeid).
Update ledger-node send/receive paths to populate/consume nodedepth instead of nodeid (gated by amendment table checks).
Remove the leaf position mismatch check from SHAMap::addKnownNode (moving validation to message ingress).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/xrpld/overlay/detail/PeerImp.cpp	Sends `nodedepth` instead of `nodeid` in `TMLedgerNode` replies when the amendment is considered supported.
src/xrpld/app/ledger/detail/InboundTransactions.cpp	Validates incoming `TMLedgerNode` fields and reconstructs node IDs (attempts to use `nodedepth`).
src/xrpld/app/ledger/detail/InboundLedgers.cpp	Applies new field-completeness checks when caching stale fetch-pack data.
src/xrpld/app/ledger/detail/InboundLedger.cpp	Validates incoming nodes and reconstructs node IDs (attempts to use `nodedepth`) before inserting into SHAMaps.
src/libxrpl/shamap/SHAMapSync.cpp	Removes the additional leaf-position consistency check inside `addKnownNode`.
include/xrpl/protocol/detail/features.macro	Adds the new fix amendment definition entry.
include/xrpl/proto/xrpl.proto	Deprecates `nodeid` and adds `nodedepth` to `TMLedgerNode`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/xrpld/overlay/detail/PeerImp.cpp

src/xrpld/app/ledger/detail/InboundTransactions.cpp

src/xrpld/app/ledger/detail/InboundLedger.cpp

src/xrpld/app/ledger/detail/InboundLedgers.cpp

codecov · 2026-02-11T21:51:54Z

Codecov Report

❌ Patch coverage is 0% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.8%. Comparing base (cd21834) to head (8e7889c).

Files with missing lines	Patch %	Lines
src/xrpld/app/ledger/detail/InboundLedger.cpp	0.0%	37 Missing ⚠️
...rc/xrpld/app/ledger/detail/InboundTransactions.cpp	0.0%	28 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedgers.cpp	0.0%	8 Missing ⚠️
src/xrpld/overlay/detail/PeerImp.cpp	0.0%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6353     +/-   ##
=========================================
- Coverage     79.9%   79.8%   -0.1%     
=========================================
  Files          840     840             
  Lines        65513   65557     +44     
  Branches      7256    7295     +39     
=========================================
- Hits         52347   52341      -6     
- Misses       13166   13216     +50

Files with missing lines	Coverage Δ
src/libxrpl/shamap/SHAMapSync.cpp	`69.2% <ø> (+0.2%)`	⬆️
src/xrpld/overlay/detail/PeerImp.cpp	`5.6% <0.0%> (-<0.1%)`	⬇️
src/xrpld/app/ledger/detail/InboundLedgers.cpp	`37.3% <0.0%> (-0.5%)`	⬇️
...rc/xrpld/app/ledger/detail/InboundTransactions.cpp	`39.0% <0.0%> (-9.8%)`	⬇️
src/xrpld/app/ledger/detail/InboundLedger.cpp	`34.5% <0.0%> (-1.6%)`	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-12T22:42:35Z

src/xrpld/app/ledger/detail/InboundLedger.cpp

+            if (app_.getAmendmentTable().isEnabled(fixLedgerNodeDepth))
            {
-                san += map.addRootNode(rootHash, makeSlice(node.nodedata()), f);
+                nodeID = SHAMapNodeID(ledgerNode.nodedepth(), treeNode->getHash().as_uint256());


When the fixLedgerNodeDepth amendment is enabled, the nodedepth value from the peer is used directly to construct a SHAMapNodeID without validating that it's within valid bounds (0-64). The SHAMapNodeID constructor has an assertion that depth <= 64, but assertions are only active in debug builds. A malicious peer could send a nodedepth > 64 which could lead to undefined behavior in release builds. Add validation to ensure nodedepth <= 64 before constructing the SHAMapNodeID, and charge the peer with feeInvalidData and call san.incInvalid() if the check fails.

Suggested change

nodeID = SHAMapNodeID(ledgerNode.nodedepth(), treeNode->getHash().as_uint256());

auto const depth = ledgerNode.nodedepth();

// Validate node depth before constructing SHAMapNodeID to avoid undefined behavior.

// Valid depths are in the range [0, 64].

if (depth > 64)

{

JLOG(journal_.warn()) << "Got invalid node depth: " << depth;

peer->charge(resource::feeInvalidData);

san.incInvalid();

return;

}

nodeID = SHAMapNodeID(depth, treeNode->getHash().as_uint256());

Copilot · 2026-02-12T22:42:35Z

src/xrpld/app/ledger/detail/InboundLedger.cpp

+                auto const nid = deserializeSHAMapNodeID(ledgerNode.nodeid());
+                if (!nid)
+                {
+                    JLOG(journal_.warn()) << "Got unexpected node id";


The error message "Got unexpected node id" is inconsistent with the similar error message at line 867 which says "Got invalid node id" and with the error message at line 164 in InboundTransactions.cpp which says "Got invalid node id". For consistency, this should be "Got invalid node id".

Suggested change

JLOG(journal_.warn()) << "Got unexpected node id";

JLOG(journal_.warn()) << "Got invalid node id";

Copilot · 2026-02-12T22:42:35Z

src/xrpld/app/ledger/detail/InboundLedger.cpp

+            // 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.
+            if (treeNode->isLeaf())
+            {
+                auto const nodeKey = dynamic_cast<SHAMapLeafNode const*>(treeNode.get())->peekItem()->key();
+                auto const expectedID = SHAMapNodeID::createID(static_cast<int>(nodeID.getDepth()), nodeKey);
+                if (nodeID.getNodeID() != expectedID.getNodeID())
+                {
+                    JLOG(journal_.warn()) << "Got invalid node id";
+                    san.incInvalid();
+                    return;
+                }
+            }


The validation logic for checking node IDs (lines 859-871) is duplicated in InboundTransactions.cpp (lines 171-183) and appears very similar. This code duplication makes maintenance harder and increases the risk of inconsistencies. Consider extracting this validation into a shared helper function that can be reused across these files.

perf: Replace node ID by depth in TMLedgerNode

2d78d41

bthomee added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Feb 11, 2026

bthomee marked this pull request as ready for review February 11, 2026 21:12

Copilot AI review requested due to automatic review settings February 11, 2026 21:12

Copilot started reviewing on behalf of bthomee February 11, 2026 21:13 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

Remove deprecated stanza on nodeid field

52dabc1

bthomee marked this pull request as draft February 11, 2026 22:17

bthomee added 3 commits February 12, 2026 06:31

Check if amendment is enabled

1cb7c02

Merge branch 'develop' into bthomee/node_depth

d836c37

Refactor

8e7889c

bthomee requested a review from Copilot February 12, 2026 22:35

Copilot started reviewing on behalf of bthomee February 12, 2026 22:36 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Replace node ID by depth in `TMLedgerNode` #6353

perf: Replace node ID by depth in `TMLedgerNode` #6353

Uh oh!

bthomee commented Feb 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-                nodeID = SHAMapNodeID(ledgerNode.nodedepth(), treeNode->getHash().as_uint256());
+                auto const depth = ledgerNode.nodedepth();
+                // Validate node depth before constructing SHAMapNodeID to avoid undefined behavior.
+                // Valid depths are in the range [0, 64].
+                if (depth > 64)
+                {
+                    JLOG(journal_.warn()) << "Got invalid node depth: " << depth;
+                    peer->charge(resource::feeInvalidData);
+                    san.incInvalid();
+                    return;
+                }
+                nodeID = SHAMapNodeID(depth, treeNode->getHash().as_uint256());

	JLOG(journal_.warn()) << "Got unexpected node id";
	JLOG(journal_.warn()) << "Got invalid node id";

perf: Replace node ID by depth in TMLedgerNode #6353

Are you sure you want to change the base?

perf: Replace node ID by depth in TMLedgerNode #6353

Uh oh!

Conversation

bthomee commented Feb 11, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

perf: Replace node ID by depth in `TMLedgerNode` #6353

perf: Replace node ID by depth in `TMLedgerNode` #6353

codecov bot commented Feb 11, 2026 •

edited

Loading