Follow-ups from PR review, bundled together because they are all
non-functional documentation and helper polish:
- Move accountDeletedMarkerKey to key_encoding.go alongside the
other leaf-key constants, and drop the stale "Keep this in sync"
directive. After this PR both GetAccount and DeleteAccount
reference the constant by name, so there is nothing left to
manually keep in sync.
- Replace the hard-coded "trie.go:219" line references in the
DeleteAccount doc and body comments with function-name references
("GetAccount's deletion-detection branch"). Line references rot
on any edit above the target line.
- Clarify what protects header storage from DeleteAccount: it shares
the same stem as BasicData/CodeHash, so the safety comes from
non-colliding offsets plus the nil-means-"do not overwrite"
semantics of StemNode.InsertValuesAtStem, not from living at a
different stem. Mirror the clarification in the
TestDeleteAccountDoesNotAffectMainStorage comment and
cross-reference the header-storage test.
- Rename newTestTrie to newEmptyTestTrie so readers can pick between
"empty" (this helper) and "pre-populated with entries" (makeTrie
in iterator_test.go) without guessing.
The package already declares `var zero [32]byte` in binary_node.go,
and both GetAccount and UpdateAccount reference `zero[:]` directly.
The PR-introduced `zeroBlob` mirrored the older shadowing pattern
from DeleteStorage but is unnecessary here — drop it and reuse the
package-level value to match the canonical style on the create side.
DeleteAccount was assigning t.root unconditionally and then
returning the error, matching UpdateAccount but diverging from the
safer pattern in UpdateStorage/DeleteStorage (assign to a temp,
return on error, mutate t.root only on success).
This is a partial fix to a broader footgun: InternalNode.Insert-
ValuesAtStem mutates its receiver in place on error paths, so the
root pointer is already shared and the mutation has already leaked
by the time we return. The proper fix is either rewriting that
function or documenting the contract; both are out of scope here.
At minimum, stop pointing t.root at whatever InsertValuesAtStem
returned on error (which could be nil or a different node
altogether), and bring DeleteAccount in line with its DeleteStorage
sibling.
DeleteStorage, UpdateStorage, and UpdateContractCode all wrap the
underlying InsertValuesAtStem/Insert error with the offending
address ("(%x) error: %v"). UpdateAccount is the lone outlier and
DeleteAccount inherited that pattern. When InsertValuesAtStem fails
deep inside the recursion (e.g. a HashedNode resolver error),
"which address" is exactly the context the caller wants. Match
DeleteStorage, the closest sibling.
TestDeleteAccountDoesNotAffectMainStorage was using `if got, _ :=
tr.GetAccount(addr); got != nil` to assert deletion. If GetAccount
ever returns a non-nil error on this path (a future change to
GetValuesAtStem, a resolver-error case), `got` is nil and the
assertion passes silently — exactly the kind of suppression the
broader fix is designed to eliminate.
Match the error-checking pattern used four lines earlier in the
same test, and rename the now-shadowed `got` for the GetStorage
result to `stored`.
Two independent tries running the same UpdateAccount → DeleteAccount
sequence must produce identical root hashes. Deletion does not
return the trie to a pristine-empty root (zero blobs hash to
non-zero leaves under the tombstone model), but the post-delete
root must be deterministic across runs — any non-determinism in
the tombstone-write path would silently fork the network on the
first self-destruct after enabling flat state.
Also pin the post-delete root != empty root invariant so a future
change to the tombstone semantics surfaces here instead of in
GetAccount's deletion-detection branch.
Header-range storage slots (key[31] < 64) live at the same stem as
BasicData and CodeHash, at offsets 64-127. The existing
TestDeleteAccountDoesNotAffectMainStorage uses a main-storage slot
(key[31] = 0x80) which lives at a different stem, giving zero
coverage of the same-stem case.
DeleteAccount's safety against header storage relies on
StemNode.InsertValuesAtStem treating nil entries as "do not
overwrite". Pin that invariant so a future change filling the values
slice with zeroBlob[:] instead of leaving nils cannot silently
corrupt slots 0-63 of any contract.
BinaryTrie.DeleteAccount was a no-op, silently ignoring the caller's
deletion request and leaving the old BasicData and CodeHash in the trie.
The GetAccount deletion-detection branch (trie.go:219) already expected
a tombstone convention — "BasicData and CodeHash are 32-byte zero blobs
AND a non-nil 32-byte sentinel is present at a reserved offset" — but
nothing was writing that sentinel, so the check was effectively dead
code.
Implement the deletion as an InsertValuesAtStem that:
- writes a 32-byte zero blob to BasicData (offset 0)
- writes a 32-byte zero blob to CodeHash (offset 1)
- writes a 32-byte zero blob to a deletion sentinel offset in the
EIP-7864 reserved range (offset 10, promoted to the named constant
accountDeletedMarkerKey for cross-referencing with GetAccount)
This matches the bintrie's existing "write zeros to delete" convention
seen in DeleteStorage, keeps GetAccount's deletion branch consistent,
and still distinguishes "deleted" from "never existed" (the latter has
all-nil slots so the empty-account check fires first).
Storage slots and code chunks are intentionally left untouched. Wiping
storage on self-destruct is a separate concern handled at the StateDB
level — the bintrie's unified keyspace has no cheap way to enumerate
every slot of a given account, so a blanket wipe is not possible here.
Regression tests cover:
- round-trip: UpdateAccount -> GetAccount -> DeleteAccount -> GetAccount nil
- delete on missing account: no panic, subsequent read still nil
- unrelated accounts at different stems are preserved
- delete + recreate: second read sees the new values, not the old ones
- main storage slots at different stems survive DeleteAccount
Fix three issues in the binary trie NodeIterator:
1. Empty nodes now properly backtrack to parent and continue iteration
instead of terminating the entire walk early.
2. `HashedNode` resolver handles `nil` data (all-zeros hash) gracefully
by treating it as Empty rather than panicking.
3. Parent update after node resolution guards against stack underflow
when resolving the root node itself.
---------
Co-authored-by: tellabg <249254436+tellabg@users.noreply.github.com>
## Summary
At tree depths below `log2(NumCPU)` (clamped to [2, 8]), hash the left
subtree in a goroutine while hashing the right subtree inline. This
exploits available CPU cores for the top levels of the tree where
subtree hashing is most expensive. On single-core machines, the parallel
path is disabled entirely.
Deeper nodes use sequential hashing with the existing `sync.Pool` hasher
where goroutine overhead would exceed the hash computation cost. The
parallel path uses `sha256.Sum256` with a stack-allocated buffer to
avoid pool contention across goroutines.
**Safety:**
- Left/right subtrees are disjoint — no shared mutable state
- `sync.WaitGroup` provides happens-before guarantee for the result
- `defer wg.Done()` + `recover()` prevents goroutine panics from
crashing the process
- `!bt.mustRecompute` early return means clean nodes never enter the
parallel path
- Hash results are deterministic regardless of computation order — no
consensus risk
## Benchmark (AMD EPYC 48-core, 500K entries, `--benchtime=10s
--count=3`, post-H01 baseline)
| Metric | Baseline | Parallel | Delta |
|--------|----------|----------|-------|
| Approve (Mgas/s) | 224.5 ± 7.1 | **259.6 ± 2.4** | **+15.6%** |
| BalanceOf (Mgas/s) | 982.9 ± 5.1 | 954.3 ± 10.8 | -2.9% (noise, clean
nodes skip parallel path) |
| Allocs/op (approve) | ~810K | ~700K | -13.6% |
Binary tree hashing is quite slow, owing to many factors. One of them is
the GC pressure that is the consequence of allocating many hashers, as a
binary tree has 4x the size of an MPT. This PR introduces an
optimization that already exists for the MPT: keep a pool of hashers, in
order to reduce the amount of allocations.
This is an optimization that existed for verkle and the MPT, but that
got dropped during the rebase.
Mark the nodes that were modified as needing recomputation, and skip the
hash computation if this is not needed. Otherwise, the whole tree is
hashed, which kills performance.
The computation of `MAIN_STORAGE_OFFSET` was incorrect, causing the last
byte of the stem to be dropped. This means that there would be a
collision in the hash computation (at the preimage level, not a hash
collision of course) if two keys were only differing at byte 31.
GetStorage and DeleteStorage used GetBinaryTreeKey to compute the tree
key, while UpdateStorage used GetBinaryTreeKeyStorageSlot. The latter
applies storage slot remapping (header offset for slots <64, main
storage prefix for the rest), so reads and deletes were targeting
different tree locations than writes.
Replace GetBinaryTreeKey with GetBinaryTreeKeyStorageSlot in both
GetStorage and DeleteStorage to match UpdateStorage. Add a regression
test that verifies the write→read→delete→read round-trip for main
storage slots.
The `Witness` method was not implemented for the binary tree, which
caused `debug_excutionWitness` to panic. This PR fixes that.
Note that the `TransitionTrie` version isn't implemented, and that's on
purpose: more thought must be given to what should go in the global
witness.
Based on [EIP-7864](https://eips.ethereum.org/EIPS/eip-7864), the tree
index should be 32 bytes instead of 31 bytes.
```
def get_tree_key(address: Address32, tree_index: int, sub_index: int):
# Assumes STEM_SUBTREE_WIDTH = 256
return tree_hash(address + tree_index.to_bytes(32, "little"))[:31] + bytes(
[sub_index]
)
```
In order to reduce the amount of code that is embedded into the keeper
binary, I am removing all the verkle code that uses go-verkle and
go-ipa. This will be followed by further PRs that are more like stubs to
replace code when the keeper build is detected.
I'm keeping the binary tree of course. This means that you will still
see `isVerkle` variables all over the codebase, but they will be renamed
when code is touched (i.e. this is not an invitation for 30+ AI slop
PRs).
---------
Co-authored-by: Gary Rong <garyrong0905@gmail.com>
This is broken off of #31730 to only focus on testing networks that
start with verkle at genesis.
The PR has seen a lot of work since its creation, and it now targets
creating and re-executing tests for a binary tree testnet without the
transition (so it starts at genesis). The transition tree has been moved
to its own package. It also replaces verkle with the binary tree for
this specific application.
---------
Co-authored-by: Gary Rong <garyrong0905@gmail.com>
Implement the binary tree as specified in [eip-7864](https://eips.ethereum.org/EIPS/eip-7864).
This will gradually replace verkle trees in the codebase. This is only
running the tests and will not be executed in production, but will help
me rebase some of my work, so that it doesn't bitrot as much.
---------
Signed-off-by: Guillaume Ballet
Co-authored-by: Parithosh Jayanthi <parithosh.jayanthi@ethereum.org>
Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
