Rewrite the LeiosFetch decision logic - #2237
Merged
ch1bo merged 49 commits intoAug 29, 2026
Merged
Conversation
ch1bo
changed the base branch from
leios-prototype
to
nfrisby/leios-first-txcache-increment
August 25, 2026 19:52
ch1bo
marked this pull request as ready for review
August 28, 2026 08:21
ch1bo
self-requested a review
August 28, 2026 08:21
ch1bo
approved these changes
Aug 28, 2026
ch1bo
left a comment
ch1bo
left a comment
Contributor
There was a problem hiding this comment.
I have not read this code! However, I was using the API and running this a lot while working on #2235, including several night long runs and stress tests. I might be able to review this today, but would also be comfortable with merging to the leios-prototype as-is.
ch1bo
force-pushed
the
nfrisby/leios-first-LeiosFetch-increment
branch
from
August 28, 2026 08:59
80e244a to
798ff41
Compare
ch1bo
force-pushed
the
nfrisby/leios-first-LeiosFetch-increment
branch
from
August 28, 2026 13:18
7d7b201 to
9f4ba52
Compare
nfrisby
changed the title
…EbBodies If we already have the EB body _and it's still marked as `BodyAlreadyInserted` in the LeiosTxCache_, don't add it to `missingEbBodies`.
…EbTxs If we already have the tx _and it's still marked as `TxAlreadyInserted` in the LeiosTxCache_, don't add it to `missingEbTxs`.
The LeiosTxCache supplants the need for this functions. The corresponding checks now has a (tunable) false miss rate, but it also now has in-memory latency instead of requiring trips to the SQLite backing store. Prior to this commit, the protototype's LeiosFetch decision loop reconciled its outstanding set against the on-disk LeiosDb once per iteration (the function was named `filterMissingWork`) — a synchronous by-hash membership probe sitting directly on the path from "a peer offers an EB body/closure" to "we issue the fetch request(s)." The `leios-txcache-bench` microbenchmark quantifies that probe: for just a single (full) EB's transactions, the SQLite lookup runs about 10× slower than the in-memory index when its pages are warm and about 190× slower when they're cold — hundreds of milliseconds versus on the order of a millisecond. Because that probe sat on the decision path, its execution time was added to per-hop fetch latency on every hop of an EB body/closure: a steady latency tax in the warm case, and — if an adversary can force the page cache cold, which we cannot prove they cannot — a hundreds-of-milliseconds latency spike in the cold case (multiple by the number of (full) in-flight EBs in whole outstanding set). This patch replaces the probe with the in-memory LeiosTxCache, so the decision consults an always-ready in-memory index (about a millisecond, independent of DB size and page-cache state) instead of waiting on the database. That shaving of per-hop latency directly benefits Leios diffusion, which the security argument depends on — and because the DB probe was the one per-iteration cost that could spike ~unboundedly with DB size and cache state, removing it is a necessary step toward running the fetch loop more often than its current meager 2 Hz.
The msgLeiosBlock handler always looks up the txs immediately after inserting the body, so fuse those lookups with the body's ref count bumping pass (when it's not a no-op).
`packedRequest` was the function that could violate it, but now the `LeiosFetchDecisions` data type that carries the fetch decisions to `packedRequest` no longer provides that degree of freedom: it's provides one SlotNo-EbHash pair per tx, and a new test requires its real pair. The LeiosFetch code was inherited from the exploratory burst demo in October 2025. One of the things that demo tried out was issuing requests for EBs where the EbSlot accompanying the EbHash was not necessarily the slot of an announcement of that hash, but instead the _potentially greater_ slot of a _potentially different_ EB that shared that tx. In order to get a higher-priority EB's tx, we could fetch it from a peer who hadn't even (yet) offered that EB but had offered an EB that we know shares txs with it. Subsequent LeiosFetch design ruled this optimization out; it's a DoS vector. If the higher-priority EB is adversarial and withheld, then the victim might never fetch the lower-priority honest EB's txs from its peers that would actually serve them. If the peer had also offered the higher-priority EB, then the request could simply list that hash instead. Thus, it's not an actual loss to eliminate this optimization. This commit does so because this complexity was muddling a bug hunt (next couple commits fix that bug; `filterMissingWork` was masking it). When the reply arrives, the priority was being interpreted as the EbHash's actual slot, which was arming the bug. As of the bugfix, that wouldn't actually cause any problems anymore, but it's still preferable to just remove the ultimately undesired feature's complexity.
Prior to this commit, the msgLeiosBlockTxs handler was only removing its arrived
tx hashs from `missingEbTxs` _for the slotNo_ carried by the
MsgLeiosBlockTxsRequest that incurred this reply. That has two consequences.
- First, it was causing a crash, since it resulted in `missingEbTxs` and
`reverseEbIndexByTx` falling out of sync. (The tx was _completely_ deleted
from `reverseEbIndexByTx`.)
- Second, the persistence in `missingEbTxs` would lead to additional
requests. (Were it not for `filterMissingWork`, see below.)
But the Leios prototype's current LeiosDb and current LeiosFetch logic dedup EB
closures, so the arrival of a tx for one EB discharges it for all other EBs as
well. Additional requests for that tx shouldn't be redundantly sent on behalf of
those EBs.
If the LeiosDb didn't dedup EB closures, then this augmentation of the deletion
logic would still be correct, but something (eg the msgLeiosBlockTxs, the next
decision logic iteration, or etc) would need to _also_ store the arrived tx to
the closure of other EBs that reference it.
In the imminent LeiosFetch logic rewrite, we'll no longer be tracking the
overlap between EB closures, and this bugfix will be trampled. But until that
rewrite, this is a correct bugfix for the Leios prototype's current LeiosFetch
and its current LeiosDb.
Notes:
- This bug (both the crash and the redundant tx fetches) surfaced now because
the recently removed `filterMissingWork` was masking it.
- For EB bodies, the LeiosFetch decisions are simple: they don't try to dedup.
So, announcements of the same EB from multiple slots will arise in redundant
fetches of the body. We consider that harmless for bodies.
- Honest nodes will very rarely announce the same EB body, so this is not a
worthwhile optimization---it's not part of the work-preservation argument.
- An announcement is entirely free to pick which EB it announces, so this
would be nothing more than an optimization, one the adversary could always
trivially choose to avoid.
- In some sense, this commit supplants `95bff62b5c0ab0937dc03e964c9e24bc731b1399
LeiosFetch: bugfix, only emit MsgLeiosBlockTxsRequest with a real LeiosPoint`,
but in another sense, that other commit wasn't merely avoiding the crash.
I erroneously removed acquiredEbBodies alongside filterMissingWork, replacing _both_ with the LeiosTxCache. That was a mistake for at least these reasons: - acquiredEbBodies had important relationships to data under the outstanding lock, and the LeiosTxCache is not under that lock, so using it instead requires multi-lock coordination. Unsurprisingly, the first attempt at that coordination was wrong/under-appreciated the challenge, and so had a bug: the missingEbBodies and LeiosTxCache could get out of sync in a way that resulted in the node constantly fetching an EB body over and over. - acquiredEbBodies merely contains the points, so it's OK for it to include _all_ volatile EBs---we don't _need_ to accept the false negatives that the cache would bring. However, replacing acquiredEbBodies with LeiosTxCache provided a couple of improvements. So this commit not only reintroduces acquiredEbBodies but also improves it in those same ways: - It's now pruned as the immutable tip advances. - It's directly updated by the forge (the filterMissingWork sledgehammer had been compensating for the old acquiredEbBodies not doing that). Now that it's being pruned, some of its uses needed to be enriched to correctly handle events (eg late message arrivals) related to data that has already been pruned out; hence the addition of acquiredEbBodiesPrunedSlot. ----- There were also some minor improvements done in passing.
With this commit, the relevant regression test fails.
```
a concurrent offer and body arrival never leave a held EB body listed (IOSimPOR): FAIL (0.04s)
*** Failed! Falsified (after 1 test):
Schedule control: ControlAwait [ScheduleMod (Thread {2,2}.14) ControlDefault [Thread {2}.7,Thread {2}.8,Thread {1}.0,Thread {1}.1]]
Thread {2,2} delayed at time Time 0s
until after:
Thread {1}
Thread {2}
held EB body still listed for fetching: [47f6c6404a56ea658d0b40e31c47eba73beaad3d3d71a5e61fc0e90ea48d6d99]
Use --quickcheck-replay="(SMGen 17010102127566570440 17528764287311168251,0)" to reproduce.
Use -p '/LeiosDemoLogic.Invariants/&&/a concurrent offer and body arrival never leave a held EB body listed (IOSimPOR)/' to rerun this test only.
```
That's because this commit (temorarily!) re-introduces the bug that was by
reintroducing acquireEbBodies.
…est" This reverts commit 60345472159e4ac0e0569ac4368539a7a634fd50.
Main idea: send fetch requests very aggressively. We're on a very tight deadline and cannot trust our peers. Thus, tail latency is our primary concern. We cannot afford to deduplicate _requests_ because any time we spend waiting on a withholding peer harms the tail latency. Our latency budget is so tight that we don't have an patience, and so any affordably-aggressive timeout would be so tight that it would cause false alarms from honest peers that are hiccuping for otherwise-tolerable reasons. Next refinement, in a subsequent commit: treat BigLedgerPeers specially, request _all_ jobs from them at once (they're our only source of truth and they might be intercontinental, so we need maximum utilization---again, for the sake of tail latency).
- Prior to this commit, we retained each EB body in memory until its closure fully arrived. - For up to 10,000 EBs with up to 512 kB per body, that's up to ~5 gigabytes---unjustifiable. - We could retrieve them from disk, but then there's extra disk IO and/or caches to maintain, etc etc. - Instead, we're storing at most one 32 byte hash per job, with at most 184 jobs per EB body, which is up to ~60 megabytes since there are up to 10000 bodies. Affordable. And since that's affordable, we have no need to _directly_ limit how many EBs are inflight at any given time (neither globally nor with each peer); a nice simplification. A future version of the LeiosDb might require some the tx's byte-offsets within the EB for storage. That might reintroduce the need for in-memory storage and hence bounding the inflight EB count---but we don't already have that need.
The node was crashing shortly after announcing an EB. The error was raised because the node was voting for the same announcement twice (which causes a crash, at least currently). The node votes once per AcquiredEbTxs, and there were two of those for EBs the node announces: one caused by the forge inserting the body and the closure and the other by LeiosFetch also fetching the body a second time, and seeing that its entire closure was present upon its arrival. (LeiosDb.InMemory actually catches this and suppresses it, but SQLite doesn't. Even so, it shouldn't be the responsibility of the LeiosDb to do so.) The forge wasn't updating the LeiosOutstanding bookkeeping to record that the body was already present. This commit fixes that.
It's grossly FreshestFirst, but the truly fresh EBs --- those younger than L slots --- are prioritized StalestFirst. So it's FreshestFirst except StalestFirst among the VeryFreshest. Another way to specifiy it is that we have two tiers. The high-priority tier contains the EBs younger than L, and, within that tier, the _stale_ EBs are prioritized. The low-priority tier contains the EBs older than L, and, within the tier, the _fresh_ EBs are prioritized. So the the priority of the high tier over the low tier respects FreshestFirst. And the priority within the low tier respects FreshestFirst. But the priority within the high tier is instead StalestFirst.
Remove maxRequestedBytesSizePerBigLedgerPeer and redefine it in terms of maxLeiosFetchIngressQueue. They're not independent, and I couldn't see any use to have the scheduling bound be tighter than the downstream buffer bound.
Before this commit, the initial LeiosOutstanding state after startup completely ignored the initial contents of the (on-disk, persisted) LeiosDb. That's _sound_ but it might lead to a lot of unnecessary refetching on startup. On the other hand, healthy (important) nodes shouldn't be restarting often. This commit is a compromise with a high power-to-weight ratio: we initialize the LeiosOutstanding state to not re-fetch/re-process the EBs we already _completely_ processed. We could do more, eg reprocess the EB bodies whose _entire_ closure we don't have. And we could warm-up the LeiosTxCache. But that's all extra complexity that's not obviously _necessary_ or even worthwhile yet.
…n stats The data include wall-clock duration of the iteration, bytes/counts about the newly decided requests, and bytes/counts about the new LeiosOutstanding state.
Key difference is that it also summarizes the Mempool hits not just the LeiosTxCache hits.
Now that LeiosFetch also pulls from the Mempool, it's important for the name to be less vague/surprising.
Neither test suite compiled after the age metrics were added. 'recordAnnouncedEb' gained an announcement onset, so its two call sites in the fetch invariants pass SNothing: those invariants are about the fetch bookkeeping, which never reads the onset. 'TraceLeiosBlockTxsAcquired' gained the age, so the ThreadNet pattern that collects acquired points ignores it. The ambiguous-type error two hundred lines below was a cascade of that one -- with the pattern ill-typed, the element type of 'acquiredPoints' could not be inferred.
Output of scripts/ci/run-fourmolu.sh and scripts/ci/run-cabal-gild.sh, which CI enforces. Mostly the layout of multi-line SPECIALISE pragmas, which fourmolu also spells SPECIALIZE, plus stanza spacing in the cabal file.
ch1bo
force-pushed
the
nfrisby/leios-first-LeiosFetch-increment
branch
from
August 28, 2026 19:06
ff3907c to
07fae70
Compare
github-project-automation
Bot
moved this from 🏗 In progress
to ✅ Done
in Consensus Team Backlog
ch1bo
added a commit
that referenced
this pull request
Aug 29, 2026
…#2235, #2245) Merges the stacked series, bottom to top: * #2188 Leios: add LeiosTxCache, but don't rely on it at all yet * #2237 Rewrite the LeiosFetch decision logic * #2235 Leios prototype: EB validation in vote logic * #2245 LeiosDb: bound the WAL, remove the write hotspot, stop dying on lock contention IMPORTANT: the schema change in #2245 has no migration. An existing database is not readable by this version and requires a wipe & resync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #2188 should merge before this one; this PR's branch extends that PR's branch.
TODO better description, link to correct Issue, etc
Major differences: