-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wip] blockchain interface exploration #580
base: coreth-001-shared-statedb-triedb-backends
Are you sure you want to change the base?
Conversation
@@ -172,6 +172,15 @@ func (e *GenesisMismatchError) Error() string { | |||
// error is a *params.ConfigCompatError and the new, unwritten config is returned. | |||
func SetupGenesisBlock( | |||
db ethdb.Database, triedb *trie.Database, genesis *Genesis, lastAcceptedHash common.Hash, skipChainConfigCheckCompatible bool, | |||
) (*params.ChainConfig, common.Hash, error) { | |||
committable := AsCommittable(db, triedb) | |||
return SetupGenesisBlockWithCommitable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this pattern—it's what I was talking about in the patching demo when I referred to breaking AST trees in half. If more complex logic was required then you could imagine there being some intermediate function in between that's defined in an _ext.go
.
@@ -172,6 +172,15 @@ func (e *GenesisMismatchError) Error() string { | |||
// error is a *params.ConfigCompatError and the new, unwritten config is returned. | |||
func SetupGenesisBlock( | |||
db ethdb.Database, triedb *trie.Database, genesis *Genesis, lastAcceptedHash common.Hash, skipChainConfigCheckCompatible bool, | |||
) (*params.ChainConfig, common.Hash, error) { | |||
committable := AsCommittable(db, triedb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) This variable is unnecessary as the function call can be inlined.
return nil | ||
} | ||
|
||
func AsCommittable(db ethdb.Database, tdb *trie.Database) commitableStateDB { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsCommitableDB()
At the call sites you see x := AsCommitable()
which is slightly less informative than AsCommitableDB()
.
@@ -205,8 +205,6 @@ func (hc *HeaderChain) Config() *params.ChainConfig { return hc.config } | |||
// Engine retrieves the header chain's consensus engine. | |||
func (hc *HeaderChain) Engine() consensus.Engine { return hc.engine } | |||
|
|||
// GetBlock implements consensus.ChainReader, and returns nil for every input as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this? (For my understanding; not saying that you should leave it)
@@ -135,6 +135,9 @@ func newTester(t *testing.T, historyLimit uint64) *tester { | |||
if err := db.Update(root, parent, uint64(i), nodes, states); err != nil { | |||
panic(fmt.Errorf("failed to update state changes, err: %w", err)) | |||
} | |||
if err := db.tree.cap(root, 128); err != nil { | |||
panic(fmt.Errorf("failed to cap state changes, err: %w", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatalf()
instead of panic()
|
||
func (b *legacyBackend) Start() {} | ||
func (b *legacyBackend) Stop() error { | ||
b.txPool.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b.chain.Stop()
return errors.Join(
b.txPool.Close(),
b.engine.Close(),
)
chain BlockChain, | ||
poolConfig legacypool.Config, | ||
minerConfig *miner.Config, | ||
clock *mockable.Clock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fyi) Definitely not for this PR, but there's a lot of test-only code that creeps into production. The miner.worker
type should really accept interface { Now() time.Time }
or even just a func() time.Time
.
@@ -678,30 +676,11 @@ func (vm *VM) initializeMetrics() error { | |||
} | |||
|
|||
func (vm *VM) initializeChain(lastAcceptedHash common.Hash) error { | |||
nodecfg := &node.Config{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all moved inside createBackendV1()
?
@@ -0,0 +1,85 @@ | |||
package chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is as far as I got. Marking it for when I start again.
Overview
Uses a slice of the existing blockchain implementation to implement the
plugin/vm
expected interface.Storage
For Storage, I plugged in the pathdb implementation from coreth (which is totally untested other than UT) for now:
However this is behind a storage/state abstraction layer inspired by the one here.
Components
This gives a picture of the components that are needed:
Using the set of interfaces between these components, we can either work on replacing each one, or moving the interfaces to have better component boundaries.
Note I re-used some config structs for convenience, those can be cleaned up separately and don't represent the ideal interface.
builds on assumptions listed here
Side note: I have considered the path of refactoring coreth in a similar way, but that is not a primary goal and it will not work drop-in (API codepaths and some initialization will be broken).
Testing
✅ UTs
TODO: fuji / mainnet
Definitely broken stuff