fix: NULL RWDB and mutex locks#728
Conversation
Sync: Ripple(d) 2.4.0
* Add AMM bid/create/deposit/swap/withdraw/vote invariants:
- Deposit, Withdrawal invariants: `sqrt(asset1Balance * asset2Balance) >= LPTokens`.
- Bid: `sqrt(asset1Balance * asset2Balance) > LPTokens` and the pool balances don't change.
- Create: `sqrt(asset1Balance * assetBalance2) == LPTokens`.
- Swap: `asset1BalanceAfter * asset2BalanceAfter >= asset1BalanceBefore * asset2BalanceBefore`
and `LPTokens` don't change.
- Vote: `LPTokens` and pool balances don't change.
- All AMM and swap transactions: amounts and tokens are greater than zero, except on withdrawal if all tokens
are withdrawn.
* Add AMM deposit and withdraw rounding to ensure AMM invariant:
- On deposit, tokens out are rounded downward and deposit amount is rounded upward.
- On withdrawal, tokens in are rounded upward and withdrawal amount is rounded downward.
* Add Order Book Offer invariant to verify consumed amounts. Consumed amounts are less than the offer.
* Fix Bid validation. `AuthAccount` can't have duplicate accounts or the submitter account.
Due to rounding, the LPTokenBalance of the last LP might not match the LP's trustline balance. This was fixed for `AMMWithdraw` in `fixAMMv1_1` by adjusting the LPTokenBalance to be the same as the trustline balance. Since `AMMClawback` is also performing a withdrawal, we need to adjust LPTokenBalance as well in `AMMClawback.` This change includes: 1. Refactored `verifyAndAdjustLPTokenBalance` function in `AMMUtils`, which both`AMMWithdraw` and `AMMClawback` call to adjust LPTokenBalance. 2. Added the unit test `testLastHolderLPTokenBalance` to test the scenario. 3. Modify the existing unit tests for `fixAMMClawbackRounding`.
|
Thanks! We had a PR that touched this area and I vaguely recall fixing /some/ issues I will look at this next week and also check if there's some overlap, but either way, given this is surely smaller, will try and prioritize it for you |
|
Mutex changes aside, which seem sensible and will look at next Monday, some other thoughts How does RWDB perform on ripple, with a giant tree? I suppose that was where things were stressed enough to expose these inefficiencies? I do wonder if you could somehow make the rotation more tree aware, and simply use one chain of structurally shared trees with garbage collection? The SHAMap leaves reference map items themselves, allocated in their own arena, so to keep them in an in-memory node store too seems somewhat redundant, especially when you consider the delta between the very latest tree once synced and back even 2048 ledgers is going to be quite small relative to the whole, ditto for the rotation! You could potentially simply keep a long chain of Ledger/SHAMap objects in memory, pruning when the ledger range goes out of range? iirc, some p2p handlers reference the nodestore |
|
I actually tried that because I really hate this copy bollox... just prune the tree... but I ended up chasing my tail more than I care to share. so I went for next, simply fix the mutex. which still required changes to the copy, lol! I really dislike that conception with a passion. but meh, we get a stable rotation with not much fusss.... is a middle ground and I think they be happy with it as well. But yes I also pref that one day a direct prune of the heap is done. |
Yeah, I hear ya! |
|
Actually, there DOES seem to be a way you can avoid the copy "bollox", and route all requests (peers etc) to the in-memory caches, meaning you can get away with a NULL backend (no rotating backend at all really needed, just the rwdb relational database) As the inbound ledgers come in, if you walk the trees and link them up properly (inner nodes have children pointers lazily linked on first walk/use otherwise), a Ledger -> SHAMap -> root node -> nested inners -> child node It's sitting slightly under 1400MiB for Xahau after ~23m, where I think normally, iirc, it sits around 4GiB. It's still a mess, bla/disclaimer et,c and I don't really have time to develop/test it further but I pushed it if you want to "compare notes" or the like: edit: latest run is using more memory, might have vibe coded a regression, but I'm sure you get the general direction/possibility |
|
Required |
sublimator
left a comment
There was a problem hiding this comment.
Some checks seem to be failing
Introduces a 'NULL' node-store mode (via XAHAU_RWDB_NULL) that operates entirely in-memory by leveraging a sliding window of retained Ledger objects. Key changes: - SHAMapSync: Bypass FullBelowCache in null mode to force full tree wiring. - Ledger: Add 'fullyWired' state tracking and mandatory wiring before use. - LedgerMaster: Implement 'mRetainedLedgers' sliding window to pin SHAMap graphs. - PeerImp: Add fallbacks to TreeNodeCache and LedgerMaster for peer requests. - contract: Add boost::stacktrace to LogThrow for easier debugging of misses. - basics: Add ReaderPreferringSharedMutex to mitigate reader starvation.
- Search both LedgerMaster and InboundLedgers for the closest fully wired base. - Implement sameChainDistance helper to accurately calculate distance between ledgers on the same chain. - Use findBestFullyWiredBase to minimize the 'prime walk' delta.
|
okay let me have a play with that branch as well, as is something I would prefer as well skipping this rotation. |
|
Afk and I can't remember if I pushed all tweaks but I let it run 2hrs and
it topped out around 1.9gb in a memory monitor. Configured with online
delete at 16.
Transaction master cache by default keeps 64k txns for up to 30m iirc, so
it can balloon a lot.
If you get it working/tested nicely, ofc please share :)
|
|
Nice work! Seems to be running fine so far here by me https://github.com/shortthefomo/xahaud/tree/null-rdwb-experiment some minor adjustments. Also ported this into the branch im testing on XRPL https://github.com/shortthefomo/rippled/tree/null-rdwb-experiment both seem to be good so far. *edited: getting state rotations on the x86 hardware so there still some long running lock vs the previous code but will find it. |
- Replace std::mutex with reader_preferring_shared_mutex in DatabaseRotatingImp (shared_lock for reads, unique_lock for writes) - Skip expensive full state tree walk when no base ledger exists in primeInboundLedgerForUse — trust sync pinning, just wire tx map - Allow null_backend to be set via [node_db] config section - Remove early RWDB online_delete requirement from Config (now defaulted by SHAMapStoreImp) - Fix SHAMapSync: only gate shared FullBelowCache operations behind useFullBelowCache(), not local per-node isFullBelow() checks - Update Config_test for removed RWDB online_delete requirement
The null-mode rotation path was calling clearLedgerCachePrior() directly instead of clearCaches(), which also clears the FullBelowCache. Stale 'full below' markers from a previous sync pass persisted across rotation, causing SHAMap sync to skip subtrees that actually need re-fetching, leading to the node oscillating between 'full' and 'syncing' states.
|
Yeah, the full below cache needs some thought/work, and you might be able to do the node linking as it happens as the tree is built, rather than after the fact. And once you go that far, you might even want to simply not even create the NodeObjects for flushing in that mode, cause it's pointless heap churn. If you can get the null mode working properly, is there even any need to have it rotating? Maybe all you really need is a direct null backend ? And the rwdb relational db (sqlite) impl ? |
Hrmmm, this is because PeerImp directly consults the SHAMap now in that prototype code? |
|
Yeah ive stumbled across a few issues my self esp in the larger DB version. Will mark this rather as draft and leave a bit more time. I chased my tail a bit before in this none rotation version(s). apologies. |
No need :) ! |
|
I pushed to the branch before with more lazy linking without the walk, and reenabling the FBC, consulting the TreeNodeCache for liveness, which keeps weak references to any retention chain resident nodes to make sure its authoritative. I also patched it to work with Without the walk requirement it will probably perform better for large maps like rippled. See here for reasoning: Not much testing yet but I'm not getting any of the immediate linking related crashes that were the impetus for the ham fisted walks. I believe we should introduce some TreeNodeCodec application level service that can encode into the wire format for peers directly from canonical shamap nodes? |
|
Another observation, currently the mRetainedLedgers isn't really matching mCompleteLedgers in LedgerMaster. Which leads to questioning whether rotation even really makes sense in this memory resident mode vs growing to some bound and pruning out of bound tails after each ledger, rather than bulk operations batched to some big 'rotation' ? edit: I implemented that, and depending on the current txn throughput (xahau seems to be quite various), it was sitting only only around 850MiB usage, by keeping only the last 16 ledgers in memory |
|
@sublimator any idea to this? seems to have tripped me up. (typically im working off xrpld as its got most of the issues present in the main net vs xahau) basically everything larger and breaks more. I then work to stable there and bring back changes here, but then also trip over some changes. eg, this one as its something I counted on but now is different here. Dang this two ledger walk is difficult. also looks like that could possibly have meant to be a change to 64MB? |
|
What am I looking at ? That's a radical difference though huh !
Yeah, I hear ya |
|
config.cpp |
|
yeah seems comments say 64MB on commit but its 64TB a15d0b2 The value is wrapped in megabytes |
|
Yeah, seems a typo/brainfart bug |
|
okay so moving that back to gets me back to where expected, im going to look to figure why, what where that change came from. Simply because it's tripping me up. Now I have to back that out to get a response I expected. *also im be out traveling for +- month, these PR prob run past that. So just understand the stop (for now). |
|
Enjoy your trip ! |
back-port Mutex fixes that cause locks and cause transitions XRPLF/rippled#6549
High Level Overview of Change
Adds a null-backend mode to the RWDB in-memory node store. When enabled (
XAHAU_RWDB_NULL=1ornull_backend=1in[node_db]),fetch()always returnsnotFoundandstore()is a no-op. State is retained entirely through Ledger → SHAMap shared_ptr chains, with a sliding window of recent ledgers kept resident in memory.This eliminates all node-store I/O and map overhead while keeping the node able to validate, build ledgers, and serve peers.
What Changed
fetch()/store(); TOCTOU race fixed (isOpen_check moved inside lock)std::mutex→reader_preferring_shared_mutex(shared_lockfor reads,unique_lockfor writes); removed unusedcopyArchiveTo()null_backendsettable via config;online_deleteauto-defaults toledger_historyFullBelowCachedisabled in null mode to prevent subtree-skipping across SHAMaps; localisFullBelow()checks no longer gated by the cache flagmRetainedLedgerssliding window pins N recent ledgers;getClosestFullyWiredLedger()for efficient delta-walk base selectionprimeInboundLedgerForUse()wires ledgers via delta walk against closest base, or trusts sync pinning when no base exists (avoids 70M+ leaf walk)recentHistoryLedgers_cache;onLedgerFetched()now passes the inbound ledger for retentionTreeNodeCacheand in-memory ledger lookup when node store returns emptyisFullyWired()/setFullyWired()tracking;fullWireForUse()helperConfig::null_backend()consolidates env var check; removed earlyonline_deleterequirementAPI Impact
libxrplchangeConfig
Standard RWDB mode
Null-backend mode
Option A — environment variable:
Option B — config file:
ledger_historymust be > 0.online_deleteis auto-defaulted toledger_historyif not set.