core: fix chain indexer reorg bug

[zsfelfoldi]

This PR fixes a bug in the chain indexer that caused repeated "chain reorged during section processing" errors under some rare circumstances.
https://github.com/ethereum/go-ethereum/blob/master/core/chain_indexer.go#L400
Here processSection checks for each processed header whether is parentHash equals the last block hash. The parent of the first block in a section is compared against the last section head which is passed as a parameter.
https://github.com/ethereum/go-ethereum/blob/master/core/chain_indexer.go#L333
The last section head is passed here which is assumed to be canonical bacause a reorg should theoretically trigger a rollback of stored sections. The problem is that this is not guaranteed to happen before the new section processing begins. If there is a stored section whose head does not match the most recent canonical hash at this point then processSection will fail but the invalid last stored section is never rolled back so processSection will try and fail again and again, assuming a wrong lastHead and failing on the first parent check.
In this fix verifyLastHead() always verifies whether the last stored section is still canonical and rolls it back if necessary. This prevents being stuck in this invalid state and as an extra bonus it always makes sure that the result of Sections() is up to date.

Fixes
#15169
#17227
#19711
#19720

[rjl493456442]

LGTM.

[rjl493456442]

Is this necessary to put the statement here? Since we will verify the validity of stored sections before processing anyway.

[zsfelfoldi]

You are right, it does not matter really but still I would prefer not leaving the indexer in a bad state after detecting an error.

[rjl493456442]

Could you please also fix the issue here https://github.com/ethereum/go-ethereum/blob/master/core/chain_indexer.go#L246 ?. The Head here is the number of a common ancestor which should be regarded valid.
Although this issue won't cause some bad errors since we will re-process the reverted sections anyway. But I think it would be nice to include it in this PR.

[thomasmodeneis]

The chain_indexer_test seems to be broken due to this changes, weird thing is that the test breaks only when executed alone, but works when running the complete core tests...

GOROOT=/usr/local/go
GOPATH=/opt/gocode
/usr/local/go/bin/go test -c -o /tmp/___chain_indexer_test_go github.com/ethereum/go-ethereum/core
/usr/local/go/bin/go tool test2json -t /tmp/___chain_indexer_test_go -test.v -test.run "^TestChainIndexerSingle|TestChainIndexerWithChildren$"
=== RUN   TestChainIndexerSingle
--- FAIL: TestChainIndexerSingle (5.79s)
    chain_indexer_test.go:155: Canonical section count mismatch: have 79, want 78
=== RUN   TestChainIndexerWithChildren
--- FAIL: TestChainIndexerWithChildren (5.77s)
    chain_indexer_test.go:155: Canonical section count mismatch: have 79, want 78
FAIL

Process finished with exit code 1

It seems that when reverting: (head + 1) to old statement head fixes the test:

GOROOT=/usr/local/go
GOPATH=/opt/gocode
/usr/local/go/bin/go test -c -o /tmp/___chain_indexer_test_go github.com/ethereum/go-ethereum/core
/usr/local/go/bin/go tool test2json -t /tmp/___chain_indexer_test_go -test.v -test.run "^TestChainIndexerSingle|TestChainIndexerWithChildren$"
=== RUN   TestChainIndexerSingle
--- PASS: TestChainIndexerSingle (2.53s)
=== RUN   TestChainIndexerWithChildren
--- PASS: TestChainIndexerWithChildren (10.61s)
PASS

Any ideas ?
source

[icodezjb]

The chain_indexer_test seems to be broken due to this changes, weird thing is that the test breaks only when executed alone, but works when running the complete core tests...

GOROOT=/usr/local/go
GOPATH=/opt/gocode
/usr/local/go/bin/go test -c -o /tmp/___chain_indexer_test_go github.com/ethereum/go-ethereum/core
/usr/local/go/bin/go tool test2json -t /tmp/___chain_indexer_test_go -test.v -test.run "^TestChainIndexerSingle|TestChainIndexerWithChildren$"
=== RUN   TestChainIndexerSingle
--- FAIL: TestChainIndexerSingle (5.79s)
    chain_indexer_test.go:155: Canonical section count mismatch: have 79, want 78
=== RUN   TestChainIndexerWithChildren
--- FAIL: TestChainIndexerWithChildren (5.77s)
    chain_indexer_test.go:155: Canonical section count mismatch: have 79, want 78
FAIL

Process finished with exit code 1

It seems that when reverting: (head + 1) to old statement head fixes the test:

GOROOT=/usr/local/go
GOPATH=/opt/gocode
/usr/local/go/bin/go test -c -o /tmp/___chain_indexer_test_go github.com/ethereum/go-ethereum/core
/usr/local/go/bin/go tool test2json -t /tmp/___chain_indexer_test_go -test.v -test.run "^TestChainIndexerSingle|TestChainIndexerWithChildren$"
=== RUN   TestChainIndexerSingle
--- PASS: TestChainIndexerSingle (2.53s)
=== RUN   TestChainIndexerWithChildren
--- PASS: TestChainIndexerWithChildren (10.61s)
PASS

Any ideas ?
source

we met the same issue, the chain_indexer_test failed #20497