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

Jump to bottom

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 2 commits
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
11 changes: 9 additions & 2 deletions core/blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2071,8 +2071,15 @@ 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()
deletedTxs, addedTxs = nil, nil // release the references in case the slices are huge
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, the linter doesn't like this

core/blockchain.go:2074:2: ineffectual assignment to deletedTxs (ineffassign)
	deletedTxs, addedTxs = nil, nil // release the references in case the slices are huge
	^
util.go:47: exit status 1

Not sure what to do here.

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this can be dropped, the GC will figure out that these things are not used anymore and will clean them up
(afaik)

Copy link
Member Author

Choose a reason for hiding this comment

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

done


// 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 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)
}
}
}
}
}