fix(backend): prevent panic when txn_idx exceeds fixed buffer size - #115
Open
ayushsingh82 wants to merge 2 commits into
Open
fix(backend): prevent panic when txn_idx exceeds fixed buffer size#115ayushsingh82 wants to merge 2 commits into
ayushsingh82 wants to merge 2 commits into
Conversation
Motivation: current_txn_hashes tracked per-transaction hashes in a Vec fixed at 10_000 entries, indexed directly by txn_idx (unbounded, taken straight off the event ring). Two of the three call sites wrote via `[]` indexing with no bounds check, so any block with txn_idx >= 10_000 panics and kills the event-forwarder task, silently cutting off all live data until the health check's exit threshold restarts the process. Modifications: Extract the tracking into a new TxnHashTracker (backend/src/lib/txn_hash_tracker.rs), backed by a HashMap<usize, [u8; 32]> instead of a fixed-size Vec, so there is no capacity to exceed. Replace the three inline call sites in run_event_forwarder_task with record/get/clear on the tracker. Added unit tests, including one that exercises a txn_idx of 50_000 to cover the exact scenario that used to panic. Result: Transaction hash tracking no longer has an upper bound on txn_idx. Existing behavior for txn_idx < 10_000 is unchanged.
ayushsingh82
requested review from
Camillebzd,
Im-Madhur-Gupta,
iamvukasin,
marcuspang and
mijovic
as code owners
August 6, 2026 14:39
|
@ayushsingh82 is attempting to deploy a commit to the MF Flagship Team on Vercel. A member of the Team first needs to authorize it. |
Motivation: Per review feedback on this PR (Greptile): switching from a fixed-size Vec to a HashMap removes the panic on txn_idx >= 10_000, but on its own it trades that crash for unbounded growth in the other direction. If a transaction's TxnEnd is ever missed (e.g. the event-ring reader hits a Gap and calls reset(), per event_listener.rs), record() has no matching clear() and that entry is retained for the life of the forwarder task. Modifications: Add TxnHashTracker::reset(), and call it on every BlockStart in run_event_forwarder_task. txn_idx is scoped to a single block, so nothing left over from a previous block is ever valid to keep - resetting on BlockStart bounds memory regardless of what gets missed mid-block. Added a test covering a txn_idx whose TxnEnd is never cleared, verifying reset() drops it. Result: Memory used by transaction hash tracking is now bounded by the transaction count of a single block, independent of any missed TxnEnd events.
Author
|
Addressed in 6cbf1b3: added |
3 tasks
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
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.
Summary
current_txn_hashesinrun_event_forwarder_tasktracked per-transaction hashes in aVec<Option<[u8; 32]>>fixed at 10,000 entries, indexed directly bytxn_idx.txn_idxcomes straight off the event ring with no upper bound. Two of the three call sites wrote via[]indexing with no bounds check (a third, read-only, site already used.get()defensively) — so any block withtxn_idx >= 10_000panics and kills the event-forwarder task, cutting off all live data to connected clients until the health check's exit threshold restarts the process.Changes
TxnHashTracker(backend/src/lib/txn_hash_tracker.rs), backed by aHashMap<usize, [u8; 32]>instead of a fixed-sizeVec, so there's no capacity to exceed.run_event_forwarder_task(server.rs) withrecord/get/clearon the tracker.TxnEndinstead of sitting asNoneforever in the old buffer.txn_idx = 50_000to cover the exact scenario that used to panic.Behavior for
txn_idx < 10_000is unchanged.Test plan
cargo fmt --checkpasses on the craterustc --test, since the full crate'smonad-event-ringdependency requires a native toolchain —cmake+ Linuxhugetlbfsheaders per the CI container — not available on my local macOS setup)cargo clippy,cargo build,cargo testin the Linux CI container) — deferred to this PR's CI run, since I could not run the full workspace build locallyGreptile Summary
The PR replaces the fixed-size transaction-hash buffer with a dynamically keyed tracker, preventing out-of-bounds panics for large transaction indices.
TxnHashTracker.TxnEnd.BlockStart, addressing the previously reported lifetime retention of orphaned entries.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the block-start reset bounds orphaned tracker entries and resolves the previous retention issue.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[BlockStart] -->|reset tracker| B[TxnHeaderStart] B -->|record txn_idx and hash| C[Transaction events] C -->|get hash by txn_idx| D[TxnEnd] D -->|clear txn_idx| B D --> E[BlockEnd] E --> AReviews (2): Last reviewed commit: "fix(backend): reset TxnHashTracker on Bl..." | Re-trigger Greptile