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: initialize current block/fastblock atomics to nil, fix #19286 #19352

Merged
merged 2 commits into from
Sep 26, 2019
Merged

core: initialize current block/fastblock atomics to nil, fix #19286 #19352

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb6b3db
core: initialize current block/fastblock atomics to nil, fix #19286
holiman Mar 29, 2019
3376125
Merge branch 'master' into fix_19286
fjl Sep 26, 2019
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
4 changes: 4 additions & 0 deletions core/blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ func NewBlockChain(db ethdb.Database, cacheConfig *CacheConfig, chainConfig *par
if bc.genesisBlock == nil {
return nil, ErrNoGenesis
}
var nilBlock *types.Block
bc.currentBlock.Store(nilBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to put bc.genesisBlock here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not convinced that would be correct. That would change the semantics, and make it appear as if the current block is genesis -- which has a different meaning than nil. The latter indicates it is simply not set (yet), the former indicates that all is in order, we should start importing from genesis.

Copy link
Contributor

@Matthalp-zz Matthalp-zz Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is actually what is already done in HeaderChain for HeaderChain.currentHeader, so I don't think this changes semantics.

My understanding is that BlockChain.loadLastState() will overwrite this value because it reads directly from the database as well. IMHO there should be an invariant where BlockChain.currentBlock is always set.

Also note that the code in #19286 is attempting to set the head to genesis block.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is also worth wondering how it got into this state. The chain configuration and genesis block were already present or SetupGenesisBlockWithOverride would have handled this, so maybe the "pull the plug" was when the head block hash key in the database got corrupted in #19286?

bc.currentFastBlock.Store(nilBlock)

if err := bc.loadLastState(); err != nil {
return nil, err
}
Expand Down