…an `Option<H256>` prefix
`new_for_storages`/`new_for_storages_with_view` and
`new_for_account_storage`/`new_for_account_storage_with_view` were the same
code twice: identical `STORAGE_TRIE_NODES`/`STORAGE_FLATKEYVALUE` tables,
identical `last_computed_flatkeyvalue` derivation, differing only in
`address_prefix: None` versus `address_prefix: Some(address_prefix)`.
Encoding that one field in the function name made it the caller's job to pick
the right name out of two that both compile everywhere, and `address_prefix`
feeds `make_key`, so the wrong pick silently mis-keys every storage read and
write. Make it a parameter instead: `new_for_storages` and
`new_for_storages_with_view` now take `address_prefix: Option<H256>`, and the
`new_for_account_storage` pair is deleted.
Each call site keeps the value its old constructor hard-coded.
`open_storage_trie` and `open_storage_trie_shared` pass `None` because the
`TrieWrapper` above them already prepends `Some(account_hash)` and a second
prefix would corrupt the keys; `open_direct_storage_trie` and the storage loop
in `flatkeyvalue_generator` pass `Some(account_hash)` because no wrapper sits
between them and the trie. `new_for_accounts` and `LockedTrieDB` are untouched,
and the on-disk key layout is unchanged, so `STORE_SCHEMA_VERSION` stays put.
Motivation
BackendTrieDBhad four constructors for the storage tables and they came in two copies of the same code.new_for_storages/new_for_storages_with_viewandnew_for_account_storage/new_for_account_storage_with_viewset the identicalSTORAGE_TRIE_NODES/STORAGE_FLATKEYVALUEtables, derivelast_computed_flatkeyvaluethe identical way, and differ in exactly one expression:address_prefix: Noneversusaddress_prefix: Some(address_prefix).That duplication is a liability precisely because the one differing field is the dangerous one.
address_prefixfeedsapply_prefixon every key path (make_key, behindgetandput_batch, plusflatkeyvalue_computeddirectly), so picking the wrong constructor silently changes every key written to and read from the storage tables. Encoding the choice in the function name means the compiler cannot help:new_for_storagesandnew_for_account_storageare both in scope, both compile at every call site, and only one of them is correct at each. Any future edit to one copy also has to be mirrored into the other, and a copy that drifts is a data-corruption bug, not a style problem.Description
The prefix is now a parameter instead of a name.
new_for_storagesandnew_for_storages_with_viewtakeaddress_prefix: Option<H256>and assign it straight to the field;new_for_account_storageandnew_for_account_storage_with_vieware deleted. Net 20 non-comment, non-blank lines removed fromcrates/storage. The surviving name is the neutral one -new_for_account_storage(db, None, ..)would have read as a contradiction.Which value each call site passes is load-bearing, and each was derived from whether the caller already applies the prefix:
Store::open_storage_trieNoneBackendTrieDBis wrapped inTrieWrapper::new(.., Some(account_hash)), andTrieWrapperprepends the prefix itself (layering.rs,prefix_nibblesbuilt innewand concatenated inget/flatkeyvalue_computed). Passing the hash here too would prefix every key twice.Store::open_storage_trie_sharedNoneTrieWrapperprefix.Store::open_direct_storage_trieSome(account_hash)Trie::opensits directly on theBackendTrieDB, so the prefix has to come from here.flatkeyvalue_generator's inner storage loopSome(account_hash)STORAGE_FLATKEYVALUEentries.Both
Nonesites are the ones that previously called anew_for_storages*constructor, and bothSomesites are the ones that previously called anew_for_account_storage*constructor, so the value reachingaddress_prefixis unchanged at all four.new_for_accounts/new_for_accounts_with_view(theACCOUNT_*tables) andLockedTrieDBare untouched.The doc comment on
new_for_storagesnow spells out theNonecase, because that is the invariant a future caller can get wrong and the type system will not catch.If this change were wrong, one of these would have to be true:
TrieWrapperdoes not apply the account prefix itself, so the twoNonesites now under-prefix their keys. It does apply it: the prefix nibbles are built inTrieWrapper::newand concatenated onto the key on every read path incrates/storage/layering.rs.last_writtenhandling. The deleted bodies are in this diff; every other field is character-for-character identical.BackendTrieDBis public, butgrep -rn 'new_for_account_storage'over the repo now returns nothing, and before this change the only callers were the four incrates/storage/store.rsand two intest/tests/storage/trie_db_tests.rs.address_prefixis read only where keys are derived (make_keyforget/put_batch, andflatkeyvalue_computed), and the value it receives at each of the four sites is the same as before, so the bytes written toSTORAGE_TRIE_NODES/STORAGE_FLATKEYVALUEare identical.No test was deleted.
test_trie_db_with_address_prefixstill covers theSomepath; it just calls the merged constructor.How to test
Results on this branch:
cargo fmt --all -- --check- clean.cargo clippy --all-targets --workspace -- -D warnings- clean (exit 0).cargo clippy -p ethrex-storage --all-targets --features rocksdb -- -D warnings- clean; the RocksDB-gated code compiles against the new signature too, since the default gate does not build it.cargo test -p ethrex-storage- 91 passed, 0 failed; doc-tests 1 passed, 1 ignored.cargo test -p ethrex-test --test ethrex_tests- 964 passed, 0 failed, 1 ignored.The targeted coverage inside that suite, for reviewers who want to run less:
cargo test -p ethrex-test --test ethrex_tests storage::- 24 passed, includingstorage::trie_db_tests::test_trie_db_with_address_prefix(write-then-read round trip through theSome(address)path) andstorage::storage_batch_tests::*(parity between the sharded and serial storage writers).cargo test -p ethrex-test --test ethrex_tests blockchain::storage_sharding- 11 passed; these build storage tries throughopen_direct_storage_trie(theSomepath) and compare sharded against serial roots, so a double-prefixed or unprefixed key shows up as a root mismatch.cargo test -p ethrex-test --test ethrex_tests p2p::snap_server- 18 passed;storage_ranges_*serves storage slots back out of the same tables that were written through the changed constructors.Checklist
Storeschema change, soSTORE_SCHEMA_VERSION(crates/storage/lib.rs) is untouched: the only field affected isBackendTrieDB::address_prefix, every call site passes the same value it passed before, and the key bytes written toSTORAGE_TRIE_NODES/STORAGE_FLATKEYVALUEare byte-identical - no re-sync is required.