Skip to content
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

core, eth, les, tests, trie: abstract node scheme #25532

Merged
merged 3 commits into from
Nov 28, 2022

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Aug 16, 2022

This PR mainly introduces a node scheme abstraction. The interface is only implemented by hashScheme now and will also be implemented by pathScheme very soon.

Apart from that, a few changes are also included and highlight them here:

  • port the changes in the stacktrie, track the path prefix of nodes when commit
  • use ethdb.Database for constructing trie.Database, it's not necessary right now, but it's required for path-based used to open reverse diff freezer

@rjl493456442 rjl493456442 force-pushed the abstract-node-scheme branch 3 times, most recently from 443ca34 to 4b831b6 Compare August 17, 2022 07:47
@rjl493456442 rjl493456442 marked this pull request as ready for review August 17, 2022 07:48
@@ -377,11 +379,11 @@ func NewBlockChain(db ethdb.Database, cacheConfig *CacheConfig, chainConfig *par
var recover bool

head := bc.CurrentBlock()
if layer := rawdb.ReadSnapshotRecoveryNumber(bc.db); layer != nil && *layer > head.NumberU64() {
if layer := rawdb.ReadSnapshotRecoveryNumber(bc.db); layer != nil && *layer >= head.NumberU64() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a mistake before. If we rewind the chain state to disk layer, then in this case recovery mode should be enabled.


sz := hexToCompactInPlace(st.key)
n := rawShortNode{Key: st.key[:sz]}
n := rawShortNode{Key: hexToCompact(st.key)}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hexToCompactInPlace is replaced by hexToCompact for not changing the st.key. Otherwise the path prefix of children will be affected since they are sharing the same byte slice.

@rjl493456442 rjl493456442 force-pushed the abstract-node-scheme branch 3 times, most recently from 6eb4c1e to 32791cc Compare August 24, 2022 07:26
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly ok, but it's a kind of complex PR -- the introductin of different node schemes takes some getting used to. I think we should have a review call about this one.

@rjl493456442
Copy link
Member Author

@holiman for sure, we can have a call for it!

@rjl493456442 rjl493456442 force-pushed the abstract-node-scheme branch 2 times, most recently from 56f9252 to 7065f7e Compare September 8, 2022 03:23
@rjl493456442 rjl493456442 requested a review from s1na as a code owner September 8, 2022 03:23
@rjl493456442 rjl493456442 force-pushed the abstract-node-scheme branch 2 times, most recently from 7cb0c4e to 899d4bd Compare September 9, 2022 03:33
@rjl493456442 rjl493456442 force-pushed the abstract-node-scheme branch 2 times, most recently from e952201 to 61f6832 Compare September 28, 2022 06:26
@rjl493456442
Copy link
Member Author

Rebased against the latest master

core/genesis.go Outdated Show resolved Hide resolved
core/state/database.go Outdated Show resolved Hide resolved
for _, name := range []string{"chaindata", "lightchaindata"} {
chaindb, err := stack.OpenDatabaseWithFreezer(name, 0, 0, ctx.String(utils.AncientFlag.Name), "", false)
if err != nil {
utils.Fatalf("Failed to open database: %v", err)
}
_, hash, err := core.SetupGenesisBlock(chaindb, genesis)
triedb := trie.NewDatabaseWithConfig(chaindb, &trie.Config{
Preimages: ctx.Bool(utils.CachePreimagesFlag.Name),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we enable preimage-recording by default? Or just based on the flags passed by users.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I think I've reviewed as far as I can. I think we need to run it a bit, e..g do syncs and generate snapshot.
Perhaps snapshot verify-state too?

core/state/database.go Outdated Show resolved Hide resolved
trie/stacktrie.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member Author

rjl493456442 commented Nov 11, 2022

@holiman Yes.

  • Snap sync: bench01
  • Snapshot generation after snap sync: bench01
  • Snapshot verification after snap sync: bench01
  • Trie traverse after snap sync: bench01
  • Full sync: bench02

@holiman holiman added this to the 1.11.0 milestone Nov 28, 2022
@holiman holiman merged commit 743e404 into ethereum:master Nov 28, 2022
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
This PR introduces a node scheme abstraction. The interface is only implemented by `hashScheme` at the moment, but will be extended by `pathScheme` very soon.

Apart from that, a few changes are also included which is worth mentioning:

-  port the changes in the stacktrie, tracking the path prefix of nodes during commit
-  use ethdb.Database for constructing trie.Database. This is not necessary right now, but it is required for path-based used to open reverse diff freezer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants