core: fix import errors on clique crashes + empty blocks #19544
Conversation
core/blockchain.go
Outdated
| } | ||
| stats.processed++ | ||
|
|
||
| // TODO(karalabe): Can we assume canonicalness here? Can we assume no logs? |
There was a problem hiding this comment.
The logs are annoying here. Essentially we should fire new logs events if exists. But known block insertion is mainly caused by Rollback and Re-import. In the Rollback function, we didn't fire some events to notify users these logs are removed, so if we fire these logs again, probably users can receive 2 notifications.
There was a problem hiding this comment.
Perhaps we can judge whether it's a canonical one by total difficulty comparison.
There was a problem hiding this comment.
I guess if we're importing a long sidechain (known blocks can only occur with side chains, or lost canons), then we can assume that the TD beat our canon TD, so we can accumulate as canonical.
| @@ -1190,15 +1193,16 @@ func (bc *BlockChain) insertChain(chain types.Blocks, verifySeals bool) (int, [] | |||
| } | |||
There was a problem hiding this comment.
Can we also do events = append(events, ChainEvent{block, block.Hash(), nil}) for prefix known blocks?
There was a problem hiding this comment.
Yeah, my reluctance around these is that I'm not sure if we're supposed to fire canon or side things? Are we sure it's always canon? If so we can definitely do.
But how do we fire logs?
There was a problem hiding this comment.
In this case, known blocks are canonical since the externTd > localTd
There was a problem hiding this comment.
I'm not really sure we should announce: if we get 10 new blocks from the network, and we in the mean time imported the first 2, we should not double announce the chain events.
|
same state (e.g. no transactions, self-transactions with 0 gas price, self-transactions by the miner).
Nitpick: it must be no transactions, since any tx inevitably changes a nonce at sender account.
|
|
@holiman @rjl493456442 I think I've addressed the review questions now and also added the missing repro test with the log output. PTAL |
|
@holiman Addressed the nit. |
|
@karalabe will this be included in 1.9? |
|
@zulhfreelancer yes |
this addresses clique non-archive node re-syncing state after a non-graceful shutdown: ethereum#19838 code is borrowed from: ethereum#19544
This PR fixes an issue during reimporting old blocks on a Clique network after a node crash.
The root of the issue is that Clique doesn't have block subsidy, so it can happen that two consecutive blocks have the same state (e.g. no transactions, self-transactions with 0 gas price, self-transactions by the miner).
If the node crashes (we lose the recent state), Geth will rewind the head block to the first one that we do have the state for. From that point onward, we try to reprocess all the blocks. If however two previously known blocks have the same state, processing the first one will also complete the second. Currently the block importer rejects the second with a "known block" error, since it doesn't expect this scenario (it rewound because no state was present, how did the state reappear out of the blue?).
This PR adds a new clause to the block importer, so that instead of rejecting an already known block, we simply ignore it and proceed to the next one. Although the code seems simple, we should try to ensure that nothing breaks.
Q: Until now we rejected known blocks in such cases. Is it a problem that we do not any more?
A: Although we did reject a known block previously, it just failed the sync, we restarted it, and the restart completely skipped the block (since it was known). As such, ignoring it instead of temporarily rejecting seems fine.
Test output for inspection:
Fixes: #19360, #19302, #19258.