Skip to content
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.

Sign up for GitHub

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

core: fix canonical hash marker update #24996

Merged
merged 3 commits into from
Jun 1, 2022
Merged

core: fix canonical hash marker update #24996

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0db899c
core: fix reorg
rjl493456442 May 31, 2022
31bae8a
core: revert change for memory efficiency
rjl493456442 May 31, 2022
9f409a4
core: revert changes
rjl493456442 Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions core/blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2063,6 +2063,11 @@ func (bc *BlockChain) reorg(oldBlock, newBlock *types.Block) error {
for _, tx := range newChain[i].Transactions() {
addedTxs = append(addedTxs, tx.Hash())
}
// Collect reborn logs due to chain reorg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MariusVanDerWijden Is it OK to move this logic back into this for loop?

I saw you moved it out in your last PR, I'm not sure if this is accidental behavior or intentional. Please correct me if there is a reason for it and I can change it back. Just for cleaning up the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the reason is memory consumption. If we do it here, the peak memory consumption is way higher since we need to keep the logs in memory at the same time as we keep the transactions in memory, moving it out allows the gc to collect the transaction ls before allocating the logs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted.

logs := bc.collectLogs(newChain[i].Hash(), false)
if len(logs) > 0 {
rebirthLogs = append(rebirthLogs, logs)
}
}

// Delete useless indexes right now which includes the non-canonical
Expand All @@ -2071,8 +2076,13 @@ func (bc *BlockChain) reorg(oldBlock, newBlock *types.Block) error {
for _, tx := range types.HashDifference(deletedTxs, addedTxs) {
rawdb.DeleteTxLookupEntry(indexesBatch, tx)
}
// Delete any canonical number assignments above the new head
number := bc.CurrentBlock().NumberU64()
// Delete all hash markers that are not part of the new canonical chain.
// Because the reorg function does not handle new chain head, all hash
// markers greater than or equal to new chain head should be deleted.
number := commonBlock.NumberU64()
if len(newChain) > 1 {
number = newChain[1].NumberU64()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as bc.CurrentBlock.NumberU64(), since the loop aove ends on bc.writeHeadBlock(newChain[1]), and that method sets bc.CurrentBlock.

So this PR only makes any change if the len(newChain) is 0 or 1. The 0 case should not happen, and for the 1 case, I agree it makes sense to delete everything above the commonBlock.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't quite get why it is the way it is,

// Insert the new chain(except the head block(reverse order)),
	// taking care of the proper incremental order.
	for i := len(newChain) - 1; i >= 1; i-- {

Why not write out the head block too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the semantic of reorg more like: prepare the chain state for new head block.
I don't plan to refactor or clean up this logic in this PR. Perhaps let's have this tiny fix in first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as bc.CurrentBlock.NumberU64(), since the loop aove ends on bc.writeHeadBlock(newChain[1]), and that method sets bc.CurrentBlock.

So this PR only makes any change if the len(newChain) is 0 or 1. The 0 case should not happen, and for the 1 case, I agree it makes sense to delete everything above the commonBlock.

Yes exactly. It mostly for this case len(newChain)=1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps let's have this tiny fix in first?

Sounds good to me (just need to get the linter silent)

}
for i := number + 1; ; i++ {
hash := rawdb.ReadCanonicalHash(bc.db, i)
if hash == (common.Hash{}) {
Expand All @@ -2084,14 +2094,6 @@ func (bc *BlockChain) reorg(oldBlock, newBlock *types.Block) error {
log.Crit("Failed to delete useless indexes", "err", err)
}

// Collect the logs
for i := len(newChain) - 1; i >= 1; i-- {
// Collect reborn logs due to chain reorg
logs := bc.collectLogs(newChain[i].Hash(), false)
if len(logs) > 0 {
rebirthLogs = append(rebirthLogs, logs)
}
}
// If any logs need to be fired, do it now. In theory we could avoid creating
// this goroutine if there are no events to fire, but realistcally that only
// ever happens if we're reorging empty blocks, which will only happen on idle
Expand Down
109 changes: 109 additions & 0 deletions core/blockchain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3758,3 +3758,112 @@ func TestSetCanonical(t *testing.T) {
chain.SetCanonical(canon[TriesInMemory-1])
verify(canon[TriesInMemory-1])
}

// TestCanonicalHashMarker tests all the canonical hash markers are updated/deleted
// correctly in case reorg is called.
func TestCanonicalHashMarker(t *testing.T) {
var cases = []struct {
forkA int
forkB int
}{
// ForkA: 10 blocks
// ForkB: 1 blocks
//
// reorged:
// markers [2, 10] should be deleted
// markers [1] should be updated
{10, 1},

// ForkA: 10 blocks
// ForkB: 2 blocks
//
// reorged:
// markers [3, 10] should be deleted
// markers [1, 2] should be updated
{10, 2},

// ForkA: 10 blocks
// ForkB: 10 blocks
//
// reorged:
// markers [1, 10] should be updated
{10, 10},

// ForkA: 10 blocks
// ForkB: 11 blocks
//
// reorged:
// markers [1, 11] should be updated
{10, 11},
}
for _, c := range cases {
var (
db = rawdb.NewMemoryDatabase()
gspec = &Genesis{
Config: params.TestChainConfig,
Alloc: GenesisAlloc{},
BaseFee: big.NewInt(params.InitialBaseFee),
}
genesis = gspec.MustCommit(db)
engine = ethash.NewFaker()
)
forkA, _ := GenerateChain(params.TestChainConfig, genesis, engine, db, c.forkA, func(i int, gen *BlockGen) {})
forkB, _ := GenerateChain(params.TestChainConfig, genesis, engine, db, c.forkB, func(i int, gen *BlockGen) {})

// Initialize test chain
diskdb := rawdb.NewMemoryDatabase()
gspec.MustCommit(diskdb)
chain, err := NewBlockChain(diskdb, nil, params.TestChainConfig, engine, vm.Config{}, nil, nil)
if err != nil {
t.Fatalf("failed to create tester chain: %v", err)
}
// Insert forkA and forkB, the canonical should on forkA still
if n, err := chain.InsertChain(forkA); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}
if n, err := chain.InsertChain(forkB); err != nil {
t.Fatalf("block %d: failed to insert into chain: %v", n, err)
}

verify := func(head *types.Block) {
if chain.CurrentBlock().Hash() != head.Hash() {
t.Fatalf("Unexpected block hash, want %x, got %x", head.Hash(), chain.CurrentBlock().Hash())
}
if chain.CurrentFastBlock().Hash() != head.Hash() {
t.Fatalf("Unexpected fast block hash, want %x, got %x", head.Hash(), chain.CurrentFastBlock().Hash())
}
if chain.CurrentHeader().Hash() != head.Hash() {
t.Fatalf("Unexpected head header, want %x, got %x", head.Hash(), chain.CurrentHeader().Hash())
}
if !chain.HasState(head.Root()) {
t.Fatalf("Lost block state %v %x", head.Number(), head.Hash())
}
}

// Switch canonical chain to forkB if necessary
if len(forkA) < len(forkB) {
verify(forkB[len(forkB)-1])
} else {
verify(forkA[len(forkA)-1])
chain.SetCanonical(forkB[len(forkB)-1])
verify(forkB[len(forkB)-1])
}

// Ensure all hash markers are updated correctly
for i := 0; i < len(forkB); i++ {
block := forkB[i]
hash := chain.GetCanonicalHash(block.NumberU64())
if hash != block.Hash() {
t.Fatalf("Unexpected canonical hash %d", block.NumberU64())
}
}
if c.forkA > c.forkB {
for i := uint64(c.forkB) + 1; i <= uint64(c.forkA); i++ {
hash := chain.GetCanonicalHash(i)
if hash != (common.Hash{}) {
t.Fatalf("Unexpected canonical hash %d", i)
}
}
}
}
}