Repository navigation
Cjl/giga state hash - #4341
Cjl/giga state hash#4341cody-littley wants to merge 8 commits into
Conversation
cody-littley
commented
Sep 25, 2026
- New methods on StateDB for getting recent hashes, and for signaling when safe to delete
- HashVault internalized inside StateDB
- HashVault no longer permits gaps in blocks
- HashVault's "crash on equivocation" feature is now optional via configuration
- StateDB setup/recovery now must consider HashVault
PR SummaryHigh Risk Overview Operator config replaces the old unsafe kill switch. Reviewed by Cursor Bugbot for commit e7df1c3. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
The PR moves the hash vault into the giga StateDB and makes it record FlatKV state checksums. Two problems block it: existing Autobahn nodes keep the same vault directory, which still holds the old app hashes, so they will fail to open on upgrade; and the app hash sent to AppQC voting is no longer checked for equivocation before it is published.
Findings: 2 blocking | 2 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
hash-vault-disabled-unsafeis removed with no deprecation policy. Viper ignores unknown keys, so an operator who set it totruegets a node that silently runs the vault with halting on. Log a startup warning when the old key is present, or add a release note pointing tohash-vault-halt-on-mismatch = false(see REVIEW_GUIDELINES §5). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4341 +/- ##
==========================================
- Coverage 67.74% 67.48% -0.27%
==========================================
Files 2174 2120 -54
Lines 168819 164708 -4111
==========================================
- Hits 114374 111156 -3218
+ Misses 54436 53543 -893
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
| // PLACEHOLDER: keeps the newest placeholderBlockHashRetention blocks' hashes. A real threshold, set by | ||
| // what giga execution needs block hashes for, should be wired in here. | ||
| if err := stateStore.PruneBlockHashesBelow(blockHashesPrunedBelow(req.Context.Number)); err != nil { |
There was a problem hiding this comment.
I'm a bit confused how we see things work. Why the prune signal comes from execution?
I thought our agreed API in March offsite was:
- consensus prepares the blocks ready for execution, and notifies storage (by saving into BlockDB)
- storage triggers execution whenever it sees fit
- after execution, storage notifies consensus which AppHash is ready for voting
- then consensus notifies storage which ones are ready for pruning
?
Did I remember that correctly?
There was a problem hiding this comment.
Based on our discussion (offline), I've changed things in the following way:
- Pruning methods are now removed from public API
- HashVault now determines its retention window via the rollback window and the lookback window, just like other stores
- A hard requirement: it is now unsafe to allow the lookback window config to be smaller than the number of blocks in an epoch. Not enforced in code, we may enforce a minimum in code at a later date.
| } | ||
|
|
||
| // GetBlockHash returns the hash the hash vault holds for blockNumber. | ||
| func (s *StateDB) GetBlockHash(blockNumber uint64) ([32]byte, gigatypes.BlockHashStatus, error) { |
There was a problem hiding this comment.
Hmm, I thought we said GetAppHash. If GetBlockHash succeeds means this blocks has executed and the states are persisted in storage that works as well, but the consensus protocol might need AppHash to send out AppVote.
There was a problem hiding this comment.
Active discussion on this on slack, will continue the discussion there.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bfb833b. Configure here.
|
@seidroid review |
There was a problem hiding this comment.
This PR moves the hash vault into the giga StateDB, where it records FlatKV state checksums. It adds recovery and refill logic and includes good tests. Nothing blocks merge, but two points need an explicit decision or a small fix: the AppQC-published app hash no longer has an equivocation guard, and legacy app hashes stay in the reused vault directory.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
Superseded: latest AI review found no blocking issues.
