remove acceptor queue (part 1) #1334
Conversation
core/blockchain.go
Outdated
| @@ -1041,10 +914,10 @@ func (bc *BlockChain) LastConsensusAcceptedBlock() *types.Block { | |||
| // | |||
| // Note: During initialization, [acceptorTip] is equal to [lastAccepted]. | |||
| func (bc *BlockChain) LastAcceptedBlock() *types.Block { | |||
| bc.acceptorTipLock.Lock() | |||
| defer bc.acceptorTipLock.Unlock() | |||
| bc.chainmu.Lock() | |||
core/blockchain.go
Outdated
| @@ -1041,10 +914,10 @@ func (bc *BlockChain) LastConsensusAcceptedBlock() *types.Block { | |||
| // | |||
| // Note: During initialization, [acceptorTip] is equal to [lastAccepted]. | |||
There was a problem hiding this comment.
should comment change here?
| @@ -1041,10 +914,10 @@ func (bc *BlockChain) LastConsensusAcceptedBlock() *types.Block { | |||
| // | |||
| // Note: During initialization, [acceptorTip] is equal to [lastAccepted]. | |||
| func (bc *BlockChain) LastAcceptedBlock() *types.Block { | |||
There was a problem hiding this comment.
can we remove "LastConsensusAcceptedBlock"?
There was a problem hiding this comment.
also I think we don't need to hold a lock everywhere we call this. seems we can just directly use the variable. should be careful with that though.
CleanBlockRootsAbcoveLastAcceptedthis function can be removed altogetherpopulateMissingTrieswarmAcceptedCaches
There was a problem hiding this comment.
CleanBlockRootsAboveLastAccepted is still used in offline pruning (to remove roots of non-accepted blocks), but I inlined the use of the variable in the 3 cases you mentioned
There was a problem hiding this comment.
CleanBlockRootsAboveLastAccepted is a bit weird since it refetches the last accepted everytime rather than trying to clean the same block root when called, so I feel there can be a race.
There was a problem hiding this comment.
do we still need to store the latest tip with WriteAcceptorTip? I mean stopping updating the stored value (not to completely remove it).
There was a problem hiding this comment.
If I'm not mistaken acceptorTip can only be =< to lastAccepted. So if we decide not to update this we might end up always reprocessing the state.
I wonder after removing the acceptor queue we will ever need to reprocess the state. Maybe we can keep the check bc.HasState(current.Root()).
I also don't fully understand how that reprocessState works. If acceptorTip =< lastAccepted then from what I see it would go back from acceptorTip since we do this current = bc.GetBlockByHash(acceptorTip). what happens to the state between acceptorTip and lastAccepted then? I might be completely wrong though.
There was a problem hiding this comment.
it is correct that acceptorTip <= lastAccepted or that acceptorTip == common.Hash{}
reprocessState is kind of confusing:
- If
acceptorTip < lastAccepted, execution will start atacceptorTip. - Next, we find the block with state present on disk so we can start re-executing (this can be less than acceptorTip/lastAccepted since we only persist roots to disk once per commit interval in pruning mode)
- When the acceptorTip is reached, from that point we will start processing the snapshot along with the state & also write the tx accepted indexes.
- We continue up to lastAccepted.
We can stop updating it once it matches lastAccepted, but I would prefer to keep that to the next PR so we get it right. Let me know if you prefer to include stop updating it in this PR.
There was a problem hiding this comment.
no I think it's fine if we remove them altogether in a safer way.
core/blockchain.go
Outdated
| return | ||
| // Update last processed and transaction lookup index | ||
| if err := bc.writeBlockAcceptedIndices(next); err != nil { | ||
| log.Crit("failed to write accepted block effects", "err", err) |
There was a problem hiding this comment.
should we return errs here instead of just log them? I think we should still keep the logs.
There was a problem hiding this comment.
it is correct to return the error (I will use fmt.Errorf)
There was a problem hiding this comment.
log.Crit uses os.Exit so it seems it should be fine, no?
* race-safe block root cleaning * Revert formatting changes --------- Signed-off-by: Ceyhun Onur <[email protected]>
Why this should be merged
Reduces differences with upstream and simplifies code.
Note this does not completely remove the acceptorTip, to avoid issues in case of a version bump during an unclean shutdown. (We can discuss how to handle this case more, but this part can already be removed)
How this works
Removes starting the acceptor queue as a goroutine, instead the same work is done inline during the call to Accept, as it was before the acceptor queue was added.
How this was tested
CI
How is this documented
TBD: remove the related config option from docs