core/state, trie, triedb: split CachingDB into MPT and UBT databases (v1.17.4 sync, #34700 adoption) - #2343
Draft
pratikspatil024 wants to merge 3 commits into
Draft
Conversation
Adopts the type split from geth ba215fd (#34700), deferred out of batch 22 where it produced 31 of the batch's 34 conflicts. Scheduled rather than dropped: #34763 applies the same split to core/state/reader.go inside batch 23, and #34843 follows in batch 30, so batch 23 should not land against a non-split tree. Declining the family would fork core/state on the name of its primary type for the rest of the sync. CachingDB becomes MPTDatabase and UBTDatabase, selected by DatabaseType, with Type() on the Database interface. IsVerkle becomes IsUBT on the trie interface, on triedb.Database and in triedb.Config, VerkleDefaults becomes UBTDefaults, and the core/state call sites stop reaching through to the trie database: they ask the state database its type instead. types.EmptyVerkleHash folds into EmptyBinaryHash, which held the same value. Every Bor divergence lands on MPTDatabase, because Bor only ever runs MPT and UBTDatabase is dormant code Bor never constructs outside tooling and tests: the snap-in-reader guard, ReaderTrieOnly, the two ReadersWithCacheStats variants returning Bor's ReaderWithStats, ContractCodeWithPrefix, and Snapshot(), which Bor carries on the Database interface where upstream does not. UBTDatabase implements Snapshot() as nil since a unified binary trie has no snapshot layer. Bor has no CodeDB, so the constructors take the snapshot and own the inline code caches instead, and NewDatabaseForTesting returns the concrete MPT database because reader_test.go needs a Bor-only method. Only the type split is taken, not the runtime fork-boundary selection also in that commit: StateAt growing a header parameter, StateAtForkBoundary, and ProcessBlock choosing per block from chainConfig.IsUBT(number, time). That half is keyed on the timestamp fork fields Bor deleted in favour of block-based ones, it ripples StateAt's signature through every caller across eth, internal, miner and the tracers, and it buys nothing while VerkleBlock is nil on every preset. The params rename and the operator-facing --override.verkle flag stay for the same reason: params is where the fork schedule lives, and the sequels touch core/state. So the vocabulary is deliberately split — core, trie and triedb say UBT, params says Verkle — and both halves are recorded in needs-wiring.md. The dropped transition-state check in OpenTrie is not a behaviour change on the dispatcher path: overlay.LoadTransitionState returns Ended set to the verkle flag when nothing is stored, so a UBT trie database already took the binary branch. The one place it is real, core/blockchain.go now building the MPT database unconditionally, needs the declined fork-boundary plumbing to fix properly and is recorded there rather than papered over with invented dispatch. CachingDB.TransitionStatePerRoot was declared, initialised and never read; the only two references in the tree were those two lines. Removed rather than carried into a newly written file. No fork gate was flipped. Amsterdam, Verkle/UBT and the binary trie remain dormant.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (58.13%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## ppatil-upstream-v1.17.3-part2 #2343 +/- ##
=================================================================
+ Coverage 54.31% 54.34% +0.02%
=================================================================
Files 925 927 +2
Lines 166726 166765 +39
=================================================================
+ Hits 90565 90625 +60
+ Misses 70436 70423 -13
+ Partials 5725 5717 -8
... and 28 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Develop-drift cascade: carries develop's #2347 (Kurtosis e2e and stateless-e2e on every PR base) and #2333 (complete witnesses under BlockSTM v2) one hop further up the stack. No conflict, but one silent break that only the compiler found. This hop splits CachingDB into MPTDatabase and its UBT counterpart, which moves ReadersWithCacheStatsTriple off the Database interface and onto *MPTDatabase. develop's witness regeneration test reaches for it through state.NewDatabase, which now returns the interface. Switched that one call to state.NewMPTDatabase. The substitution is exact rather than a narrowing: CachingDB was the MPT database, the method moved across with an identical body, and the test wants an MPT-backed production reader stack. The returned *MPTDatabase still satisfies Database for the NewWithReader call below it. Verified: build clean; vet clean apart from the pre-existing parallel_state_processor.go:341 lock-copy finding; gofmt clean; #2333's prewalk and read-set tests pass, and both regeneration tests pass with 241/241 real mainnet blocks round-tripped and no skips.
Ancestry only. The newTrieReader point-cache fix this carries was already present here, so the merge records the relationship without changing a byte. That is the reason it exists. Without it this branch would not contain its predecessor, and a stacked pull request whose head does not contain its base misreports its own diff and turns an eventual merge into an argument. Deliberately not re-verified, because there is nothing new to verify: the merge result's tree is identical to this branch's previous tree, which is the tree that already passed build, full-tree vet, #2333's prewalk and read-set tests, and CI. A merge with no tree delta cannot break what that tree established.
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
1 participant
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
Adopts the type split from go-ethereum
ba215fd92(#34700, cmd, core, trie,triedb: split CachingDB into merkle + binary dbs) as a hand-written port rather
than a merge resolution. 33 modified + 2 new files, +137/−299 of code.
This is an out-of-band adoption in the ongoing v1.17.4 sync, stacked between the
v1.17.3 merge PRs — the same pattern as the eth/70 PR (#2341) and the
core/vmcatch-up (#2337).
Why it is a separate PR, and why now
#34700 produced 31 of batch 22's 34 conflicts and was deferred out of #2342 so
a hand-written restructure of consensus-critical
core/statewould not rideinside a 20-commit merge commit.
The deferral was scheduled, not open-ended: #34763 applies the same MPT/UBT split
to
core/state/reader.goin the very next batch (batch 23), and #34843 follows inbatch 30. Declining the family permanently would fork
core/stateon the name ofits primary type for the rest of the sync — the compounding-divergence trap already
three deep on the eth-protocol surface.
Upstream frames #34700 as groundwork for #34004, the UBT state transition, which is
not in this sync's range at all — its only mention anywhere in the fetched
upstream history is #34700's own commit message.
The one decision: which half of #34700 to take
#34700 does two separable things. Only the first is adopted.
1. The type split — adopted.
CachingDBbecomesMPTDatabase+UBTDatabase,selected by a new
DatabaseType(TypeMPT/TypeUBT) withType()on theDatabaseinterface.IsVerklebecomesIsUBTon the trie interface, ontriedb.Databaseand intriedb.Config;triedb.VerkleDefaultsbecomesUBTDefaults;types.EmptyVerkleHashfolds intoEmptyBinaryHash, which held thesame value. The
core/statecall sites stop reaching through to the trie databaseand ask the state database its type instead (
db.Type().Is(TypeUBT)).This is where the future-merge value is, and it is the whole reason to do this now.
2. Runtime fork-boundary selection — declined. Upstream also rewrites
BlockChain.StateAt(root)to take a*types.Header, addsStateAtForkBoundary(parent, header)for the last-MPT/first-UBT boundary, makesHistoricStaterefuse UBT, and hasProcessBlockpick the implementation perblock from
chainConfig.IsUBT(number, time)behind a localprewarmReadertypeassertion. Declined because:
ones, so
IsUBT(num, time)targets fields Bor does not have;StateAt's signature through every caller acrosseth,internal,minerand the tracers;VerkleBlockis nil on every Bor preset.Also declined: upstream's
params/config.gorename (already declined in #2342 —Bor has no
VerkleTimeto rename) andChainOverrides.OverrideVerkle→OverrideUBT, which would rename the operator-facing--override.verkleCLI flagand its
ethconfigTOML key, and require regeneratinggen_config.go, for adormant fork.
The rule, so the boundary is not ad hoc
The visible consequence is a few mixed-vocabulary lines —
EnableUBTAtGenesisreturning
genesis.Config.EnableVerkleAtGenesis, andtests/block_test_util.gosetting
IsUBT: gspec.Config.IsVerkleGenesis(). That is the boundary showingthrough, not an oversight.
Where Bor's divergences landed
Every Bor-only member of
CachingDBmoved ontoMPTDatabase, because Bor onlyever runs MPT and
UBTDatabaseis dormant code Bor never constructs outsidetooling and tests:
DisableSnapInReader/EnableSnapInReaderand theuseSnapInReaderguardReaderTrieOnly(V2 parallel execution)ReadersWithCacheStatsandReadersWithCacheStatsTriple, returning Bor'sReaderWithStatsrather than upstream'sReaderContractCodeWithPrefixSnapshot(), which Bor carries on theDatabaseinterface where upstream doesnot.
UBTDatabaseimplements it asreturn nil— a unified binary trie has nosnapshot layer.
Two constructor deviations from upstream, both forced by pre-existing divergence:
CodeDB. Upstream's constructors take(triedb, codedb); Bor'stake
(triedb, snap)for MPT and(triedb)for UBT, and each owns the inlinecodeCache/codeSizeCachepair thatnewCachingCodeReaderneeds.NewDatabaseForTestingreturns*MPTDatabase, not upstream'sDatabase,because
core/state/reader_test.gocalls the Bor-onlyReadersWithCacheStats.NewDatabasebecomes upstream's deprecated dispatcher, returningDatabaseandchoosing from
tdb.IsUBT(). Bor's footprint is 35 files against upstream's 67because Bor's callers already held
state.Database— the concrete type appeared inexactly one production declaration (
BlockChain.statedb), so only three sitesneeded the concrete constructor.
Two things verified rather than assumed
The dropped
OpenTrietransition-state check is behaviour-preserving on thedispatcher path. Bor's
CachingDB.OpenTrieconsulted the overlay transitionstate when the triedb was verkle: panic if
InTransition(),BinaryTrieifTransitioned(), else fall through to aStateTrie. The split drops that.overlay.LoadTransitionState(db, root, isVerkle)with no stored state returns&TransitionState{Ended: isVerkle}, so for a UBT triedbTransitioned()wasalready true and the old code already returned a
BinaryTrie;OpenStorageTrielikewise returned
self, which is whatUBTDatabasedoes.The one place the narrowing is real is
core/blockchain.go, which now buildsNewMPTDatabaseunconditionally. That only matters for a hand-written genesissetting
enableVerkleAtGenesis: true— a configuration no Bor preset uses and notest drives through
NewBlockChain— and fixing it properly is the declinedfork-boundary plumbing, so it is recorded in
needs-wiring.mdrather than paperedover with invented dispatch.
CachingDB.TransitionStatePerRootwas dead. Anlru.Cacheof*overlay.TransitionStatesized 1000, declared and initialised and never read —the only two references in the tree were its declaration and its initialiser.
Upstream's
MPTDatabasehas no equivalent. Removed rather than carried into anewly written file, where it would have pulled the
overlayimport in for nothing.Executed tests
Beyond CI's standard gates:
go build ./...,go vet ./...,gofmt -l,go mod tidy— clean apart from thetwo pre-existing
//nolintcopylocks.make lint(golangci-lint v2.11.4, the version CI runs) — 0 issues.tests/borintegration suite — 622.3 s, exit 0.TestReinforceMultiClientPreCompilesTest,TestV2ForkParity) — noRulesfield name moved, since theparamsrename isdeclined.
go testgreen oncore(171.0 s),eth(46.8 s),miner(199.0 s),consensus/bor(41.7 s),triedb/pathdb(44.5 s),core/state,core/state/snapshot,core/types,core/types/bal,trie,trie/bintrie,trie/trienode,triedb, alleth/...including every tracer package,tests,internal/ethapi, all otherconsensus/...,cmd/utils.e15a2b63brather than assumed, because this change touches both packages — all eight fail
identically before it:
cmd/evm'sTestT8n/TestEVMTracing/TestEvmRun/TestEvmRunRegEx, andcmd/geth'sTestConsoleWelcome/TestCustomGenesis/TestCustomBackend/TestExport.TestCustomGenesiswas worth confirmingspecifically, since this renames
Genesis.IsVerkleandhashAlloc's parameter.UBTDatabaseis exercised, not merely compiled. Two existing tests reach itthrough the
NewDatabasedispatcher by opening a triedb withtriedb.UBTDefaults:TestVerklePrefetcher(coveringOpenTrie's binary branch,OpenStorageTriereturning self, the prefetcher's
isUBTpath, and theAccessEventsallocation inNewWithReader) andTestStateDBCopyBinaryTrie(ported from #34758 in #2342).Twin scan:
ParallelStateDBcarries no verkle/UBT branch and does not mirrorIntermediateRootorhandleDestruction, so the fivestatedb.goconversions toType().Is(...)have no V2 counterpart. No Bor-only implementor ofstate.Databaseorstate.Trieexists beyondHistoricDB— givenType()returning
TypeMPTwith upstream's own TODO — and the three trie types.Rollout notes
the binary trie all remain dormant (
VerkleBlocknil on every preset), soUBTDatabaseis never constructed on any Bor network. Thecore/stateconversions are equivalence-preserving:
Type().Is(TypeUBT)⟺ the oldTrieDB().IsVerkle()for every database the dispatcher can produce.--override.verkleflag and its TOML key aredeliberately left alone.
docs/upstream-merges/v1.17.4/needs-wiring.md; the fork decision is infork-register.md; full per-file reasoning inledger.md. Those doc updates arewritten but ride in the milestone chores commit in v1.17.3 part 3, so this PR
carries no doc changes.
Stacked PR — do not squash
This is part of a stack. Merge order matters and squash-merging any PR in it
breaks every PR above it, because squashing rewrites commits into new SHAs and
the PRs above would then re-show all of this PR's changes and conflict against
their base. Team standard for upstream syncs is a merge commit, never squash.
Reviews are deliberately not being requested yet — the sync is mid-flight and
several decisions are still open. This will be marked ready once all six milestones
are complete.