fix(l1): iterate from the seek key in the in-memory backend - #7137
Open
IIITManjeet wants to merge 1 commit into
Open
fix(l1): iterate from the seek key in the in-memory backend#7137IIITManjeet wants to merge 1 commit into
IIITManjeet wants to merge 1 commit into
Conversation
`get_receipts_for_block_from_index` builds a `block_hash || start_index` seek key and hands it to `prefix_iterator`, relying on the backend to position there and iterate forward. RocksDB's `prefix_iterator_cf` does exactly that, and the caller stops itself once the 32-byte block hash stops matching. The in-memory backend instead filtered on `key.starts_with(seek_key)`, so only the key equal to `block_hash || start_index` survived. On `EngineType::InMemory`, `get_receipts_for_block` therefore returned a single receipt per block, and `eth_getTransactionReceipt` could only ever resolve transaction index 0. `cleanup_old_witnesses` was affected the same way: it iterates from the oldest witness block number and breaks past a threshold, intending to prune the whole range, but only ever saw one block number's keys. The regression test is added to the `store_tests` suite, which runs against every engine, so it pins in-memory/RocksDB parity rather than just in-memory correctness.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Author
|
Hey @ilitteri can you please have a look at my PR. |
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.
Motivation
Store::get_receipts_for_block_from_indexbuilds ablock_hash || start_indexseek key and hands it toprefix_iterator, relying on the backend to position there and iterate forward:RocksDB's
prefix_iterator_cfdoes exactly that, and the loop stops itself once the 32-byte block hash stops matching. The in-memory backend instead filtered on a literalkey.starts_with(seek_key), so only the key equal toblock_hash || start_indexsurvived.On
EngineType::InMemory,get_receipts_for_blocktherefore returned a single receipt per block, no matter how many were stored — soeth_getTransactionReceiptcould only ever resolve transaction index 0.EngineType::InMemoryis reachable outside tests viacmd/ethrex/initializers.rs:212.cleanup_old_witnessesis affected the same way: it iterates from the oldest witness block number and breaks past a threshold, intending to prune the whole range, but under the old filter only ever saw one block number's keys.Description
Changes the in-memory backend's
prefix_iteratorto seek semantics — retain keys>= prefix, then sort — matching RocksDB. Entries were already sorted afterwards, and every caller either passes an empty prefix (the migrations, unaffected) or bounds its own scan, so no caller relies on the old truncating behaviour.The regression test goes in the
store_testssuite, which runs against every engine, so it pins in-memory/RocksDB parity rather than just in-memory correctness. It covers all three read shapes: the whole block, a non-zero start index, and a bounded count.Question for reviewers: is
EngineType::InMemoryintended to be production-reachable, or is it considered test-only? That determines whether this is a user-facing RPC bug or "only" a backend-parity defect that made in-memory tests unable to observe more than one receipt per block. Either way the backends should agree, but it affects how this should be prioritised and whether it warrants a backport.Testing
cargo test -p ethrex-storage— 87 passed, 0 failedcargo test -p ethrex-test --test ethrex_tests storage::— 24 passed, 0 failedcargo fmt --all -- --checkclean;cargo clippy -p ethrex-storage --all-targetscleanChecklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.Not applicable — this changes read-path iteration only. No stored bytes, key schema, or table layout change, so no re-sync is required.