Statediff for full node #6

Merged
elizabethengelman merged 9 commits from statediff-for-full-node into statediff-for-archive-node 2019-02-21 20:36:04 +00:00
2 changed files with 13 additions and 24 deletions
Showing only changes of commit f2047cc503 - Show all commits

View File

@ -155,12 +155,14 @@ func makeFullNode(ctx *cli.Context) *node.Node {
if ctx.GlobalIsSet(utils.ConstantinopleOverrideFlag.Name) {
cfg.Eth.ConstantinopleOverride = new(big.Int).SetUint64(ctx.GlobalUint64(utils.ConstantinopleOverrideFlag.Name))
}
if ctx.GlobalBool(utils.StateDiffFlag.Name) {
cfg.Eth.StateDiff = true
}
utils.RegisterEthService(stack, &cfg.Eth)
if ctx.GlobalBool(utils.StateDiffFlag.Name) {
cfg.Eth.StateDiff = true
rmulhol commented 2019-02-12 22:39:39 +00:00 (Migrated from github.com)
Review

would it make sense to combine this with the RegisterStateDiffService call, if both are based off of the call to ctx.GlobalBool(utils.StateDiffFlag.Name)?

would it make sense to combine this with the `RegisterStateDiffService` call, if both are based off of the call to `ctx.GlobalBool(utils.StateDiffFlag.Name`)?
elizabethengelman commented 2019-02-14 21:45:08 +00:00 (Migrated from github.com)
Review

yeah, good call!

yeah, good call!
utils.RegisterStateDiffService(stack, ctx)
}
if ctx.GlobalBool(utils.DashboardEnabledFlag.Name) {
utils.RegisterDashboardService(stack, &cfg.Dashboard, gitCommit)
}
@ -185,9 +187,6 @@ func makeFullNode(ctx *cli.Context) *node.Node {
utils.RegisterEthStatsService(stack, cfg.Ethstats.URL)
}
if ctx.GlobalBool(utils.StateDiffFlag.Name) {
utils.RegisterStateDiffService(stack, ctx)
}
return stack
}

View File

@ -929,9 +929,6 @@ func (bc *BlockChain) WriteBlockWithoutState(block *types.Block, td *big.Int) (e
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())
}
if ok {
count++
@ -1015,28 +1012,14 @@ func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types.
}
rmulhol commented 2019-02-14 16:49:22 +00:00 (Migrated from github.com)
Review

Maybe worth putting 1 and 2 behind variables/constants to clarify what's going on here? My understanding is that a hash needs to be added once as the current hash, and once as the parent hash - and then we're done. Is that right? Do reorgs potentially complicate that expectation?

Maybe worth putting `1` and `2` behind variables/constants to clarify what's going on here? My understanding is that a hash needs to be added once as the current hash, and once as the parent hash - and then we're done. Is that right? Do reorgs potentially complicate that expectation?
rmulhol commented 2019-02-14 16:52:01 +00:00 (Migrated from github.com)
Review

Do we want to retain all these logging statements when we merge?

Do we want to retain all these logging statements when we merge?
elizabethengelman commented 2019-02-14 21:51:13 +00:00 (Migrated from github.com)
Review

probably a good idea to remove some of them 👍

probably a good idea to remove some of them 👍
elizabethengelman commented 2019-02-14 22:48:01 +00:00 (Migrated from github.com)
Review

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 stateDiffsProcessed collection. It then gets added again when the state diff is processed when it is the parent, so we'll need to make sure it isn't pruned before these diffs have been processed.

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?

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 `stateDiffsProcessed` collection. It then gets added again when the state diff is processed when it is the parent, so we'll need to make sure it isn't pruned before these diffs have been processed. 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?
rmulhol commented 2019-02-15 00:31:09 +00:00 (Migrated from github.com)
Review

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.

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.
elizabethengelman commented 2019-02-15 15:12:05 +00:00 (Migrated from github.com)
Review

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

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
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 {
log.Debug("Current root has not yet been processed for it's child", "root", root.(common.Hash).Hex())
if !bc.allowedRootToBeDereferenced(root.(common.Hash)) {
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))
}
}
log.Debug("Dereferencing", "root", root.(common.Hash).Hex())
triedb.Dereference(root.(common.Hash))
}
}
@ -1091,6 +1074,13 @@ func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types.
return status, nil
}
//if we haven't processed the statediff for a given state root and it's child, don't dereference it yet
func (bc *BlockChain) allowedRootToBeDereferenced(root common.Hash) bool {
diffProcessedForSelfAndChildCount := 2
count := bc.stateDiffsProcessed[root]
return count >= diffProcessedForSelfAndChildCount
}
// addFutureBlock checks if the block is within the max allowed window to get
// accepted for future processing, and returns an error if the block is too far
// ahead and was not added.