fix: Extend the fix in #7763 to re-store more, targeted nodes - #7793
fix: Extend the fix in #7763 to re-store more, targeted nodes#7793ximinez wants to merge 16 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
fcfba84 to
67e4cd2
Compare
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
f865fc2 to
c0cd461
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
ximinez
added
the
DraftRunCI
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
adb511e to
3dbcec5
Compare
ximinez
changed the title
- Add a special case to healthWait() to not pause when the server is
DISCONNECTED.
- Log sequence differences at the start and end of rotation.
- Limit copy-forward to ledgers we're not about to delete, and unknown.
(Changes rotationInFlight_ from a bool to a LedgerIndex.)
- Set the "rotation in flight" index right at the beginning of the rotation
- Because the node copy process can take a long time, other ledgers may
get validated. Any reads for those ledgers have the potential to be
served by the archive DB and thus lost, too. Why wait?
- Add an assertion suggested on @vlntb in #7763.
- Don't wait as long for ledgers that should be built soon.
- Rescue nodes from the tree node cache, too.
bae52af to
5f087f6
Compare
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
|
Hey — I noticed something while working on a memory-only node some months back: SHAMap trees are only partially and opportunistically linked, and normally rely on NodeStore to relink children on demand. There is also substantial competition for canonical nodes. Because canonicalization picks one same-hash object without merging linkage, a poorer canonical can win and discard child pointers from a richer copy. Those children then have to be resynthesized through the cache or NodeStore. I directly observed this producing SHAMapMissingNode after switching to NullFactory; changing canonicalization from pick to merge stopped those failures. I asked Fable and Sol to gather the relevant code paths because I suspect this is part of the MissingNode puzzle here and may interact with the rotation durability issue. I can’t guarantee every detail in either document is correct (don't agree with Fable's maybe that's why flapping? for example), but they should point toward the relevant areas: https://gist.github.com/sublimator/d89356a3933f6c64432089810440afeb |
…ximinez/online-delete-gaps2 * XRPLF/ximinez/online-delete-gaps: build: Patсh binary in local Linux nix environment (7859)
- Nodes rescued from freshenCaches are checked dynamically, and not written if not AccountNode.
This is fascinating, but also beyond the scope of this PR. Could you create an Issue (or PR with the merging changes) so that we can track it independently of this PR? This insight could not just make memory-only nodes work more reliably, but could also help reduce I/O load for disk-backed nodes, since they'll be needing a lot fewer reads. Now, the flipside of this is that, aside from memory-only stuff, we don't want to hold the entire ledger in memory all the time. There are more than 27 million nodes in the mainnet ledger - keeping all of those all the time is going to also be expensive. And a typical ledger is going to need, what, a few thousand leaf nodes, plus their corresponding inner nodes? I doubt this is the only way that untouched, or infrequently touched nodes are allowed to expire from RAM, or whether it's even intentional to let it go, but it is something to keep in mind for the more widely-used disk-backed SHAMap. FWIW, running my node with the current changes in this PR, I have not yet seen a single missing node or re-stored node. I think the copy forward mechanism, and particularly moving it early is preventing active nodes from being deleted from disk in the first place. If that pans out, it would be a big win. |
9eff879 to
e61b134
Compare
- The "copied ledger" count may differ from the "Rotating:" count, because the latter will also count nodes from the cache.
ximinez
removed
the
DraftRunCI
There was a problem hiding this comment.
Pull request overview
This PR expands the online-delete (NodeStore rotating backend) durability fix from #7763 by widening the “copy-forward” protection window, reducing unnecessary copy-forward work for soon-to-be-deleted ledgers, and letting rotation continue during full disconnection.
Changes:
- Start the rotation “copy-forward” window earlier and track it with a ledger-index threshold (instead of a boolean).
- Add targeted “rescue” writes for cache-resident SHAMap nodes that are missing from both backends, and add additional rotation diagnostics/counters.
- Adjust rotation health gating to treat
DISCONNECTEDas “healthy” for rotation progress.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/app/rdb/backend/detail/Node.cpp | Improves accepted-ledger missing-node logging by including exception details. |
| src/xrpld/app/misc/SHAMapStoreImp.h | Adds cache-driven node rescue during cache freshening (rotation). |
| src/xrpld/app/misc/SHAMapStoreImp.cpp | Implements rescueNode, starts rotation flag earlier, improves rotation/healthWait logging and behavior. |
| src/libxrpl/nodestore/DatabaseRotatingImp.cpp | Implements ledger-index-gated copy-forward, adds counters/logging, and exposes rotation state. |
| include/xrpl/nodestore/detail/DatabaseRotatingImp.h | Updates rotating DB implementation API and adds new counters/state members. |
| include/xrpl/nodestore/DatabaseRotating.h | Extends the DatabaseRotating public interface to support ledger-index rotation windows and metrics. |
Comments suppressed due to low confidence (1)
src/xrpld/app/misc/SHAMapStoreImp.cpp:781
- Grammar in comment: "A disconnected state is should never be caused" -> "A disconnected state should never be caused".
auto healthy = [&]() {
// Special case: If the server is disconnected, it's not doing any ledger I/O, because
// it's focused on trying to get peers. A disconnected state is should never be caused by
// the activity of the server. It's usually limited to hardware or connectivity issues. Take
// advantage of that to run as much rotation I/O as possible before it comes back online.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JLOG(journal_.warn()) | ||
| << "finished rotation. validatedSeq: " << validatedSeq | ||
| << ", lastRotated: " << lastRotated << " diff " << diff | ||
| << ". Updated validated seq is " << currentValidatedSeq << ", " << processingDiff | ||
| << " ledgers were validated during the rotation processs. Complete ledgers: " | ||
| << ledgerMaster_->getCompleteLedgers(); |
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
| void | ||
| DatabaseRotatingImp::setRotationInFlight(bool inFlight) | ||
| DatabaseRotatingImp::setRotationInFlight(LedgerIndex inFlight) | ||
| { | ||
| rotationInFlight_.store(inFlight, std::memory_order_release); | ||
| JLOG(j_.debug()) << "Rotating: copy-forward on archive reads " | ||
| << (inFlight ? "enabled" : "disabled"); | ||
| JLOG(j_.debug()) << "Rotating: copy-forward on archive reads from " << inFlight << " forward"; | ||
| } |
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
|
Yeah, in practice it keeps it all largely resident, albeit haphazardly, so I agree that the policy should be more explicit. I am also very much with you in thinking that fully materializing the tree all the time is not scaling well. I've done a lot of research on hybrid trees with mmap snapshotskeee, and I think that's the future. That said, lower hanging fruit may be available. This is completely off the top of my head (i.e. maybe not feasible), but I suppose you could make a configuration option to actually /unlink/ on sweep (or flush at extremes??) if you wanted by direct policy choice. How that would interact with syncing and backfill settings, with potential disk thrash? I'm not sure, but I could imagine a steady-state node using the current paradigm with lazily materialized nodes. Opened an issue re: pick vs merge: |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…ximinez/online-delete-gaps2 * XRPLF/ximinez/online-delete-gaps: chore: Move semantic version tests to gtest (7872) style: Make clang-tidy format files using clang-format rules (7880) ci: Change `server_definitions` upload config name (7878) chore: Trivial gtest migrations (7865) ci: Group github-actions dependabot updates (7876) ci: Update CI image and prepare-runner action (7874) test: Migrate `nodestore` tests from Beast to GTest (7292) test: Improve the server status test to not race and randomly fail (7304) ci: [DEPENDABOT] bump actions/checkout from 7.0.0 to 7.0.1 (7871) chore: Fix clang version in devshell (7860) chore: Verify tooling version for Nix-managed environments (7862)
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
…ximinez/online-delete-gaps2 * XRPLF/ximinez/online-delete-gaps: Update lastLedger in the healthWait loop in the right order
| if (duplicate || rotationInFlight_.load(std::memory_order_acquire)) | ||
| auto const inFlight = getRotationInFlight(); | ||
| if (duplicate || (inFlight != 0 && (ledgerSeq == 0 || ledgerSeq >= inFlight))) | ||
| { |
There was a problem hiding this comment.
I think I purposely included all possible duplicate=false reads as in-flight reads to guarantee that anything in the TreeNodeCache is backed by the DB store. Here is an example I can think of:
Let say, T0 - rotation starts. setRotationInFlight(90,000,000).
T1 - a peer requests an old ledger or getObjectByHash() after the getKeys() during freshenCache. This inner node H found in 89,999,000, since the rotation swap hasn't happened, so was loaded from the archive, it won't be copy forwarded in this logic since ledgerSeq = 89,999,000 < inFlight = 90,000,000, but now it lives in the treeNodeCache with a cowid==0
T2 - swap. rotate() drops the old archive. H's only on-disk copy is gone. H now exists only in RAM (TreeNodeCache).
T3 - rotation completes. New lastRotated = 90,001,024.
T4 - Now days later at ledger 90,050,000, a transaction made the inner node hash back to H again. Building 90,050,000's state map, SHAMap looks up H, finds the clean cached node from T1, and reuses it (cowid == 0).
T5 - flush. flushDirty writes only dirty nodes (cowid == current). H is clean → skipped. Ledger 90,050,000 is validated and persisted referencing H, which is on no backend.
T6 - TreeNodeCache evicts H, or the node restarts
T7 - The fetch of H misses both backends → SHAMapMissingNode .
The uncertainty is that we can't guarantee all objects in the cache will always come from ledgers within the complete_ledgers range. Neither Sweep nor clearPrior clears objects based on ledger ranges for the TreeNodeCache and we don't do duplicate==true read unless it's during the rotation copy.
There was a problem hiding this comment.
The uncertainty is that we can't guarantee all objects in the cache will always come from ledgers within the complete_ledgers range. Neither Sweep nor clearPrior clears objects based on ledger ranges for the TreeNodeCache and we don't do duplicate==true read unless it's during the rotation copy.
🤔 I did an experiment where I requested all the nodes from an ledger that was about to be deleted until I was unable to continue. I had zero missing nodes. I'll need to re-run that experiment in light of this scenario, and see what the results look like. I'll also try to do it with just object indices.
If I see what you're indicating, or if I get unclear results, I have no problem switching back to a bool flag instead of a value. My big concern is someone forcing a large amount of irrelevant data to be copied continuously kept "alive", defeating the purpose of online delete in the first place.
| { | ||
| auto const node = cache.fetch(key); | ||
| if (node) | ||
| rescueNode(*node); |
There was a problem hiding this comment.
freshenCache() calls rescueNode() without expectedType; this defaults inner nodes to Unknown, triggering UNREACHABLE instead of persisting the rescue. Pass the node type or infer it from cache metadata:
| rescueNode(*node); | |
| rescueNode(*node, NodeObjectType::AccountNode); |
| case SHAMapNodeType::TnAccountState: | ||
| return NodeObjectType::AccountNode; | ||
| // We don't expect to see transaction nodes. The check below will prevent writing them. | ||
| case SHAMapNodeType::TnTransactionNm: |
There was a problem hiding this comment.
rescueNode() from freshenCache() without expectedType for TnInner yields Unknown, fails type check, hits UNREACHABLE (data loss risk). Choose: (a) skip TnInner with debug log, (b) track SHAMap type per cache entry, or (c) silent-skip instead of UNREACHABLE for missing expectedType.
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
| return fdRequired_; | ||
| } | ||
|
|
||
| void |
There was a problem hiding this comment.
Assert message says 'copyNode' but function is now 'rescueNode'—update label:
| void | |
| XRPL_ASSERT(node.cowid() == 0, "SHAMapStoreImp::rescueNode : rescued node must be clean"); |
| void | |
| XRPL_ASSERT(node.cowid() == 0, "SHAMapStoreImp::rescueNode : rescued node must be clean"); |
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
Bound the number of manifests carried in a single TMManifests message (kMaxManifestsPerMessage). Trusted manifests are always included and processed; untrusted gossip is capped per message on both send and receive, and the sender is charged only when untrusted entries are actually skipped. Oversized TMManifests messages are dropped without penalty at the protocol layer so an unpatched peer is not disconnected. Complements the cache bound from #276/#323.
* release/3.2.x: chore: Bump version to 3.2.1 chore: Bump version to 3.2.1-rc1 fix: Cap untrusted manifests per message and drop oversized ones fix: Reject oversized validator manifest before decoding fix: Reduce untrusted manifest cache cap to 100 fix: Bound untrusted manifest cache
| { | ||
| auto const node = cache.fetch(key); | ||
| if (node) | ||
| rescueNode(*node); |
There was a problem hiding this comment.
Inner nodes resolve to Unknown type and fail rescueNode's type check. Skip TnInner nodes:
if (node && node->getType() != SHAMapNodeType::TnInner)
rescueNode(*node);
| { | ||
| auto const node = cache.fetch(key); | ||
| if (node) | ||
| rescueNode(*node); |
There was a problem hiding this comment.
🟠 Severity: HIGH
freshenCache calls rescueNode without an expectedType. For TnInner nodes, the switch yields NodeObjectType::Unknown, which fails the AccountNode guard and silently drops the node. Inner nodes in the tree-node cache that exist only in RAM will be permanently lost during rotation, making entire state subtrees unreachable.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Pass NodeObjectType::AccountNode as the expectedType argument to rescueNode in the freshenCache call, just as copyNode does at line 326. This ensures that inner nodes are correctly mapped to NodeObjectType::AccountNode (since all state-map inner nodes are part of the account state tree) and will pass the guard condition, allowing them to be stored during rotation instead of being silently dropped.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| rescueNode(*node); | |
| rescueNode(*node, NodeObjectType::AccountNode); |
High Level Overview of Change
Expands on the changes in #5531 and #7763:
Context of Change
This PR is built on top of #5531, and expands on the changes in #7763 to improve data integrity and reduce data loss when the online_delete archive store is deleted.
Before / After
There should be no directly user-observable effects from this change.
Operators and administrators should stop seeing any missing node errors or crashes.