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

all: activate pbss as experimental feature #26274

Merged
merged 16 commits into from
Aug 10, 2023

Conversation

rjl493456442
Copy link
Member

No description provided.

@MariusVanDerWijden
Copy link
Member

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

some nits, review stopped at snap_disklayer

trie/database_wrapper.go Outdated Show resolved Hide resolved
trie/database_wrapper.go Outdated Show resolved Hide resolved
trie/iterator.go Outdated Show resolved Hide resolved
trie/nodeset.go Outdated Show resolved Hide resolved
trie/nodeset.go Outdated Show resolved Hide resolved
trie/snap_database.go Outdated Show resolved Hide resolved
trie/snap_database_test.go Outdated Show resolved Hide resolved
trie/snap_difflayer.go Outdated Show resolved Hide resolved
trie/snap_diskcache.go Outdated Show resolved Hide resolved
trie/snap_disklayer.go Outdated Show resolved Hide resolved
@rjl493456442 rjl493456442 force-pushed the activate-pbss-2 branch 2 times, most recently from 4049899 to 8a7b676 Compare November 30, 2022 11:23
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

Okay done, looks very good to me! Mostly nitpicks one or two clarifications maybe 1 issue

trie/snap_disklayer.go Outdated Show resolved Hide resolved
trie/snap_disklayer_snapshot.go Outdated Show resolved Hide resolved
trie/snap_disklayer_snapshot.go Outdated Show resolved Hide resolved
trie/snap_journal.go Outdated Show resolved Hide resolved
trie/snap_journal.go Outdated Show resolved Hide resolved
core/blockchain.go Show resolved Hide resolved
core/blockchain_test.go Show resolved Hide resolved
core/genesis_test.go Outdated Show resolved Hide resolved
core/genesis_test.go Outdated Show resolved Hide resolved
core/state/statedb_test.go Outdated Show resolved Hide resolved
@kumavis
Copy link
Member

kumavis commented May 18, 2023

how does #25963 differ / relate to this branch?

@MariusVanDerWijden
Copy link
Member

This one activates PBSS, while the other one implements it. @rjl493456442 split the two in order to make the review easier

@Neozaru
Copy link

Neozaru commented Jun 2, 2023

Hi. Is this still being worked on ?

@rjl493456442
Copy link
Member Author

Hi. Is this still being worked on ?

Yes

@@ -246,6 +247,10 @@ func handleGetNodeData66(backend Backend, msg Decoder, peer *Peer) error {
// ServiceGetNodeDataQuery assembles the response to a node data query. It is
// exposed to allow external packages to test protocol behavior.
func ServiceGetNodeDataQuery(chain *core.BlockChain, query GetNodeDataPacket) [][]byte {
// Request nodes by hash is not supported in path-based scheme.
if chain.TrieDB().Scheme() == rawdb.PathScheme {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Should we perhaps drop eth/xx that has GetNodeData?

eth/handler.go Outdated Show resolved Hide resolved
eth/handler.go Outdated Show resolved Hide resolved
core/state/statedb_test.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Show resolved Hide resolved
cmd/utils/flags_legacy.go Outdated Show resolved Hide resolved
@@ -1341,6 +1398,11 @@ func (bc *BlockChain) writeBlockWithState(block *types.Block, receipts []*types.
if err != nil {
return err
}
// If node is running in path mode, skip explicit gc operation
// which is unnecessary in this mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Later on (not this PR), it might be neat to move the gc-stuff away.
So we have

if bc.triedb.Scheme() == rawdb.PathScheme {
    return nil
}
// Hashdb requires explicit gc / flushing
blockchain.doHashdbGc(a,b,c...)

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good to me.

cmd/geth/snapshot.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

Left some comments, all in all the PR seems good to me. We should merge after the next release to max out time on master, since it's not really possible to meaningfully review every possible issue that could arise.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

This PR won't get any better unless people start poking holes in it. Let's merge and open the floodgates.

@karalabe karalabe merged commit 503f1f7 into ethereum:master Aug 10, 2023
1 check passed
@karalabe karalabe added this to the 1.13.0 milestone Aug 11, 2023
weiihann pushed a commit to weiihann/go-ethereum that referenced this pull request Aug 15, 2023
* all: activate pbss

* core/rawdb: fix compilation error

* cma, core, eth, les, trie: address comments

* cmd, core, eth, trie: polish code

* core, cmd, eth: address comments

* cmd, core, eth, les, light, tests: address comment

* cmd/utils: shorten log message

* trie/triedb/pathdb: limit node buffer size to 1gb

* cmd/utils: fix opening non-existing db

* cmd/utils: rename flag name

* cmd, core: group chain history flags and fix tests

* core, eth, trie: fix memory leak in snapshot generation

* cmd, eth, internal: deprecate flags

* all: enable state tests for pathdb, fixes

* cmd, core: polish code

* trie/triedb/pathdb: limit the node buffer size to 256mb

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
# Conflicts:
#	cmd/evm/runner.go
#	cmd/evm/staterunner.go
#	cmd/geth/chaincmd.go
#	core/chain_makers.go
#	core/rawdb/database.go
#	core/state/statedb_test.go
#	core/state/sync_test.go
#	eth/state_accessor.go
#	miner/miner_test.go
#	tests/state_test_util.go
holiman added a commit that referenced this pull request Aug 22, 2023
The PR #26274 broke the evm statetest command a bit, in that it stopped spitting out the stateroot following a non-successful statetest-execution. 

This PR changes it back, so the stateroot is unconditionally output on stderr, and makes it so fuzzing works again.
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
* all: activate pbss

* core/rawdb: fix compilation error

* cma, core, eth, les, trie: address comments

* cmd, core, eth, trie: polish code

* core, cmd, eth: address comments

* cmd, core, eth, les, light, tests: address comment

* cmd/utils: shorten log message

* trie/triedb/pathdb: limit node buffer size to 1gb

* cmd/utils: fix opening non-existing db

* cmd/utils: rename flag name

* cmd, core: group chain history flags and fix tests

* core, eth, trie: fix memory leak in snapshot generation

* cmd, eth, internal: deprecate flags

* all: enable state tests for pathdb, fixes

* cmd, core: polish code

* trie/triedb/pathdb: limit the node buffer size to 256mb

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
The PR ethereum#26274 broke the evm statetest command a bit, in that it stopped spitting out the stateroot following a non-successful statetest-execution. 

This PR changes it back, so the stateroot is unconditionally output on stderr, and makes it so fuzzing works again.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Nov 29, 2023
* all: activate pbss

* core/rawdb: fix compilation error

* cma, core, eth, les, trie: address comments

* cmd, core, eth, trie: polish code

* core, cmd, eth: address comments

* cmd, core, eth, les, light, tests: address comment

* cmd/utils: shorten log message

* trie/triedb/pathdb: limit node buffer size to 1gb

* cmd/utils: fix opening non-existing db

* cmd/utils: rename flag name

* cmd, core: group chain history flags and fix tests

* core, eth, trie: fix memory leak in snapshot generation

* cmd, eth, internal: deprecate flags

* all: enable state tests for pathdb, fixes

* cmd, core: polish code

* trie/triedb/pathdb: limit the node buffer size to 256mb

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Feb 26, 2024
Additions:
 eth/state_accessors.go:
Made StateAtBlock public

Conflicts:

  core/blockchain.go:
Upstream changes to setHeadBeyondRoot were taken, while keeping our
extra logic limiting rewind.

  core/state/pruner/pruner.go:
 trie.NewDatabase accepts new parameter. moved the change from where it
 was made upstream to where we use the same code

  eth/state_accessor.go:
  trie/triedb/hashdb/database.go
  trie/triedb/pathdb/database.go:
we had logic to clear caches avoiding memory leaks. Upstream implemented
their own solutions - use upstream solution in these cases.

  eth/backend.go
  tests/block_test_util.go
  core/block_validator_test.go
  core/blockchain_repair_test.go
  core/blockchain_sethead_test.go
  core/blockchain_snapshot_test.go
  core/blockchain_test.go
  core/chain_makers_test.go
  core/genesis_test.go
many conflicts, mostly in tests were either due to:
* changes in params to NewBlockChain -> we have an extra parameter set
  to nill, used updated calls with our extra nil param
* tests using TriesInMemory which we renamed to DefaultTriesInMemory
  (took changes and updated to our name)
* getting triedb config from cacheconfig, which now has a dedicated
  function. I used the new function.
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.

7 participants