-
Notifications
You must be signed in to change notification settings - Fork 4
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
Statediff for full node #6
Changes from 5 commits
3a5f7b7
498831e
79686c9
08d0fe3
e380580
7995c5a
4d6ddf9
f2047cc
272d2f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,10 +71,11 @@ const ( | |
// CacheConfig contains the configuration values for the trie caching/pruning | ||
// that's resident in a blockchain. | ||
type CacheConfig struct { | ||
Disabled bool // Whether to disable trie write caching (archive node) | ||
TrieCleanLimit int // Memory allowance (MB) to use for caching trie nodes in memory | ||
TrieDirtyLimit int // Memory limit (MB) at which to start flushing dirty trie nodes to disk | ||
TrieTimeLimit time.Duration // Time limit after which to flush the current in-memory trie to disk | ||
Disabled bool // Whether to disable trie write caching (archive node) | ||
TrieCleanLimit int // Memory allowance (MB) to use for caching trie nodes in memory | ||
TrieDirtyLimit int // Memory limit (MB) at which to start flushing dirty trie nodes to disk | ||
TrieTimeLimit time.Duration // Time limit after which to flush the current in-memory trie to disk | ||
ProcessStateDiffs bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly does this flag entail? Hint hint, there are comments on the rest of them ;) |
||
} | ||
|
||
// BlockChain represents the canonical chain given a database with a genesis | ||
|
@@ -136,6 +137,8 @@ type BlockChain struct { | |
|
||
badBlocks *lru.Cache // Bad block cache | ||
shouldPreserve func(*types.Block) bool // Function used to determine whether should preserve the given block. | ||
|
||
stateDiffsProcessed map[common.Hash]int | ||
} | ||
|
||
// NewBlockChain returns a fully initialised block chain using information | ||
|
@@ -155,24 +158,26 @@ func NewBlockChain(db ethdb.Database, cacheConfig *CacheConfig, chainConfig *par | |
blockCache, _ := lru.New(blockCacheLimit) | ||
futureBlocks, _ := lru.New(maxFutureBlocks) | ||
badBlocks, _ := lru.New(badBlockLimit) | ||
|
||
stateDiffsProcessed := make(map[common.Hash]int) | ||
bc := &BlockChain{ | ||
chainConfig: chainConfig, | ||
cacheConfig: cacheConfig, | ||
db: db, | ||
triegc: prque.New(nil), | ||
stateCache: state.NewDatabaseWithCache(db, cacheConfig.TrieCleanLimit), | ||
quit: make(chan struct{}), | ||
shouldPreserve: shouldPreserve, | ||
bodyCache: bodyCache, | ||
bodyRLPCache: bodyRLPCache, | ||
receiptsCache: receiptsCache, | ||
blockCache: blockCache, | ||
futureBlocks: futureBlocks, | ||
engine: engine, | ||
vmConfig: vmConfig, | ||
badBlocks: badBlocks, | ||
chainConfig: chainConfig, | ||
cacheConfig: cacheConfig, | ||
db: db, | ||
triegc: prque.New(nil), | ||
stateCache: state.NewDatabaseWithCache(db, cacheConfig.TrieCleanLimit), | ||
quit: make(chan struct{}), | ||
shouldPreserve: shouldPreserve, | ||
bodyCache: bodyCache, | ||
bodyRLPCache: bodyRLPCache, | ||
receiptsCache: receiptsCache, | ||
blockCache: blockCache, | ||
futureBlocks: futureBlocks, | ||
engine: engine, | ||
vmConfig: vmConfig, | ||
badBlocks: badBlocks, | ||
stateDiffsProcessed: stateDiffsProcessed, | ||
} | ||
|
||
bc.SetValidator(NewBlockValidator(chainConfig, bc, engine)) | ||
bc.SetProcessor(NewStateProcessor(chainConfig, bc, engine)) | ||
|
||
|
@@ -922,6 +927,20 @@ func (bc *BlockChain) WriteBlockWithoutState(block *types.Block, td *big.Int) (e | |
return nil | ||
} | ||
|
||
func (bc *BlockChain) AddToStateDiffProcessedCollection(hash common.Hash) { | ||
count, ok := bc.stateDiffsProcessed[hash] | ||
if count > 1 { | ||
log.Error("count is too high", "count", count, "hash", hash.Hex()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this error need to be handled in any additional capacity? Right now it still falls through to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch. This log statement was more for me as informational to see if |
||
} | ||
|
||
if ok { | ||
count++ | ||
bc.stateDiffsProcessed[hash] = count | ||
} else { | ||
bc.stateDiffsProcessed[hash] = 1 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is equivalent: count, _ := bc.stateDiffsProcessed[hash]
bc.stateDiffsProcessed[hash] = count + 1 If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch! |
||
} | ||
|
||
// WriteBlockWithState writes the block and all associated state to the database. | ||
func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types.Receipt, state *state.StateDB) (status WriteStatus, err error) { | ||
bc.wg.Add(1) | ||
|
@@ -994,6 +1013,30 @@ func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types. | |
bc.triegc.Push(root, number) | ||
break | ||
} | ||
|
||
if bc.cacheConfig.ProcessStateDiffs { | ||
count, ok := bc.stateDiffsProcessed[root.(common.Hash)] | ||
//if we haven't processed the statediff for a given state root and it's child, don't dereference it yet | ||
if !ok { | ||
log.Debug("Current root NOT found root in stateDiffsProcessed", "root", root.(common.Hash).Hex()) | ||
bc.triegc.Push(root, number) | ||
break | ||
} | ||
if count < 2 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth putting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a really good catch. Yeah, my intention was that when we processes a state diff for a given hash, it gets added to the Considering reorgs is a really good thought, I'm honestly not sure how that would affect this expectation. I think I'd need to spend some time digging into this - do you think it makes sense to hold off merging this in? Or, merging it, and creating a new story to take a look in the future? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely sounds like another story to me, and a bit hard to simulate without getting to the head of the chain and watching it execute in the middle of one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it definitely will be tricky. I've added another story so we can make sure to circle back: https://makerdao.atlassian.net/browse/VDB-393 |
||
log.Debug("Current root has not yet been processed for it's child", "root", root.(common.Hash).Hex()) | ||
bc.triegc.Push(root, number) | ||
break | ||
} else { | ||
log.Debug("Current root found in stateDiffsProcessed collection with a count of 2, okay to dereference", | ||
"root", root.(common.Hash).Hex(), | ||
"blockNumber", uint64(-number), | ||
"size of stateDiffsProcessed", len(bc.stateDiffsProcessed)) | ||
|
||
delete(bc.stateDiffsProcessed, root.(common.Hash)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we clean up the map when we're finished with a certain diff? Was just about to ask about memory leaks here, but probably okay if that's the case! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, if a root is okay to be dereferenced, we removed it from the stateDiffsProcessed collection just to make sure that that collection doesn't get huge. And then it will fall through to |
||
} | ||
} | ||
|
||
log.Debug("Dereferencing", "root", root.(common.Hash).Hex()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to retain all these logging statements when we merge? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably a good idea to remove some of them π |
||
triedb.Dereference(root.(common.Hash)) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1483,3 +1483,84 @@ func BenchmarkBlockChain_1x1000Executions(b *testing.B) { | |
|
||
benchmarkLargeNumberOfValueToNonexisting(b, numTxs, numBlocks, recipientFn, dataFn) | ||
} | ||
|
||
func TestProcessingStateDiffs(t *testing.T) { | ||
defaultTrieCleanCache := 256 | ||
defaultTrieDirtyCache := 256 | ||
defaultTrieTimeout := 60 * time.Minute | ||
cacheConfig := &CacheConfig{ | ||
Disabled: false, | ||
TrieCleanLimit: defaultTrieCleanCache, | ||
TrieDirtyLimit: defaultTrieDirtyCache, | ||
TrieTimeLimit: defaultTrieTimeout, | ||
ProcessStateDiffs: true, | ||
} | ||
db := ethdb.NewMemDatabase() | ||
genesis := new(Genesis).MustCommit(db) | ||
numberOfBlocks := triesInMemory | ||
engine := ethash.NewFaker() | ||
blockchain, _ := NewBlockChain(db, cacheConfig, params.AllEthashProtocolChanges, engine, vm.Config{}, nil) | ||
blocks := makeBlockChain(genesis, numberOfBlocks + 1, engine, db, canonicalSeed) | ||
_, err := blockchain.InsertChain(blocks) | ||
if err != nil { | ||
t.Fatalf("failed to create pristine chain: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. β¨ pristine β¨ |
||
} | ||
defer blockchain.Stop() | ||
|
||
//when adding a root hash to the collection, it will increment the count | ||
firstStateRoot := blocks[0].Root() | ||
blockchain.AddToStateDiffProcessedCollection(firstStateRoot) | ||
value, ok := blockchain.stateDiffsProcessed[firstStateRoot] | ||
if !ok { | ||
t.Error("state root not found in collection") | ||
} | ||
if value != 1 { | ||
t.Error("state root count not correct", "want", 1, "got", value) | ||
} | ||
|
||
blockchain.AddToStateDiffProcessedCollection(firstStateRoot) | ||
value, ok = blockchain.stateDiffsProcessed[firstStateRoot] | ||
if !ok { | ||
t.Error("state root not found in collection") | ||
} | ||
if value != 2 { | ||
t.Error("state root count not correct", "want", 2, "got", value) | ||
} | ||
|
||
moreBlocks := makeBlockChain(blocks[len(blocks) - 1], 1, engine, db, canonicalSeed) | ||
_, err = blockchain.InsertChain(moreBlocks) | ||
|
||
//a root hash can be dereferenced when it's state diff and it's child's state diff have been processed | ||
//(i.e. it has a count of 2 in stateDiffsProcessed) | ||
nodes := blockchain.stateCache.TrieDB().Nodes() | ||
if containsRootHash(nodes, firstStateRoot) { | ||
t.Errorf("stateRoot %s in nodes, want: %t, got: %t", firstStateRoot.Hex(), false, true) | ||
} | ||
|
||
//a root hash should still be in the in-mem db if it's child's state diff hasn't yet been processed | ||
//(i.e. it has a count of 1 stateDiffsProcessed) | ||
secondStateRoot := blocks[1].Root() | ||
blockchain.AddToStateDiffProcessedCollection(secondStateRoot) | ||
if !containsRootHash(nodes, secondStateRoot) { | ||
t.Errorf("stateRoot %s in nodes, want: %t, got: %t", secondStateRoot.Hex(), true, false) | ||
} | ||
|
||
//the stateDiffsProcessed collection is cleaned up once a hash has been dereferenced | ||
_, ok = blockchain.stateDiffsProcessed[firstStateRoot] | ||
if ok { | ||
t.Errorf("stateRoot %s in stateDiffsProcessed collection, want: %t, got: %t", | ||
firstStateRoot.Hex(), | ||
false, | ||
ok, | ||
) | ||
} | ||
} | ||
|
||
func containsRootHash(collection []common.Hash, hash common.Hash) bool{ | ||
for _, n := range collection { | ||
if n == hash { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This diff shows how we should write our tests instead of that damn Ginkgo! Looks so clean, and probably runs way faster. π Why are we even using Could probably be split up in smaller tests, but I figure the setup is a bit annoying. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,8 @@ var DefaultConfig = Config{ | |
Blocks: 20, | ||
Percentile: 60, | ||
}, | ||
|
||
StateDiff: false, | ||
} | ||
|
||
func init() { | ||
|
@@ -135,6 +137,8 @@ type Config struct { | |
|
||
// Constantinople block override (TODO: remove after the fork) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π π΄ |
||
ConstantinopleOverride *big.Int | ||
|
||
StateDiff bool | ||
} | ||
|
||
type configMarshaling struct { | ||
|
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.
would it make sense to combine this with the
RegisterStateDiffService
call, if both are based off of the call toctx.GlobalBool(utils.StateDiffFlag.Name
)?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.
yeah, good call!