perf: Replace node ID by depth in TMLedgerNode#6353
perf: Replace node ID by depth in TMLedgerNode#6353
TMLedgerNode#6353Conversation
bthomee
added
the
DraftRunCI
There was a problem hiding this comment.
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
fixLedgerNodeDepthamendment and extendTMLedgerNodeproto withnodedepth(deprecatingnodeid). - Update ledger-node send/receive paths to populate/consume
nodedepthinstead ofnodeid(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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6353 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.0%
=========================================
Files 858 859 +1
Lines 67757 67834 +77
Branches 7557 7569 +12
=========================================
+ Hits 54064 54121 +57
- Misses 13693 13713 +20
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| takeNodes( | ||
| std::vector<std::pair<SHAMapNodeID, Slice>> const& data, | ||
| std::shared_ptr<Peer> const&); | ||
| std::vector<std::pair<SHAMapNodeID, intr_ptr::SharedPtr<SHAMapTreeNode>>>&& data, |
There was a problem hiding this comment.
This can be an impactful and measurable optimisation. It would be useful to benchmark it. If the black-box test does not pick an improvement, then we should do CPU profiling of the takeNodes stack.
There was a problem hiding this comment.
I can use your help with the latter, if the performance benchmark doesn't show much difference. The run I started this morning doesn't contain the r-value change yet, so I'll start another one when it's done but then with the r-value change.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (childHash != treeNode->getHash()) | ||
| { | ||
| JLOG(journal_.warn()) << "Corrupt node received"; | ||
| return SHAMapAddNode::invalid(); | ||
| } |
There was a problem hiding this comment.
SHAMap::addKnownNode no longer verifies that a received leaf node’s key is consistent with the nodeID position (the extra post-hash validation that prevents “leaf at wrong position” inconsistencies). Since addKnownNode is a public sync entry-point, keeping this validation here provides defense-in-depth and avoids relying on every caller to remember this security-sensitive check; consider reintroducing the leaf position/key consistency validation (or an equivalent invariant check) before canonicalizing/inserting the node.
There was a problem hiding this comment.
All code paths leading to SHAMap::addKnownNode use data that has been validated, i.e. the ledger node proto message is valid, the data field correctly converts into a tree node, and the nodeid/depth field correctly converts into a node ID. As such, this is not an issue.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
High Level Overview of Change
This change introduces a new protocol version 2.3,
LedgerNodeDepth, which replaces the 32-byte node ID in theTMLedgerNodeproto message by a 2-byte node depth instead (1 byte for the field tag, 1 byte for the depth as it won't exceed 64) for leaf nodes.Context of Change
An earlier bug allowed a compromised UNL validator to send a maliciously crafted
TMLedgerNodemessage with a node ID that didn't match the actual node ID corresponding to where the leaf node 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 for leaf nodes, and the much smaller node depth suffices. The extra check is now moved to the places where theTMLedgerNodemessages are handled.The current logic passes the node data around and constructs a
SHAMapTreeNodeat the end. However, we need this instance earlier to check if a node is a leaf. The function definitions and implementations are updated to take a tree node instead, to avoid reconstructing the instance multiple times.Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)