-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
p2p/sentry: StatusDataProvider ReadCurrentHeader error #9890
Conversation
core/rawdb/accessors_chain.go
Outdated
@@ -284,6 +288,16 @@ func ReadCurrentHeader(db kv.Getter) *types.Header { | |||
return ReadHeader(db, headHash, *headNumber) | |||
} | |||
|
|||
// ReadCurrentHeader2 reads the current canonical head header. | |||
func ReadCurrentHeader2(db kv.Getter) *types.Header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to ReadCurrentBlockHeader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand the difference between the 2 tables before renaming. I've asked on discord.
p2p/sentry/status_data_provider.go
Outdated
@@ -76,6 +76,9 @@ func (s *StatusDataProvider) GetStatusData(ctx context.Context) (*proto_sentry.S | |||
|
|||
func ReadChainHeadWithTx(tx kv.Tx) (ChainHead, error) { | |||
header := rawdb.ReadCurrentHeader(tx) | |||
if header == nil { | |||
header = rawdb.ReadCurrentHeader2(tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just call this directly instead of calling ReadCurrentHeader first? what matters to us is the which is the last block that has been sync-ed and get its header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason I originally did the patch like this was that I found the first call was failing and like Daniel I don't know what the difference is between the data in the tables.
Once this is merged I'll overwrite my local fix before completing my PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the ReadCurrentHeader call. One problem with it is that during initial sync (which can take a week) we will report to P2P that we are still on genesis, because HeadBlockKey is only updated at stage_finish. I think that conceptually it is correct (that we only report confirmed state blocks), but it is not nice for the P2P (because we are leeching for many days).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should report the last committed body. I'll make a follow-up with this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mh0lt I will merge it now, because it fixes a production issue. I can help to rebase your PR and fix conflicts.
p2p/sentry/status_data_provider.go
Outdated
@@ -76,6 +76,9 @@ func (s *StatusDataProvider) GetStatusData(ctx context.Context) (*proto_sentry.S | |||
|
|||
func ReadChainHeadWithTx(tx kv.Tx) (ChainHead, error) { | |||
header := rawdb.ReadCurrentHeader(tx) | |||
if header == nil { | |||
header = rawdb.ReadCurrentHeader2(tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason I originally did the patch like this was that I found the first call was failing and like Daniel I don't know what the difference is between the data in the tables.
Once this is merged I'll overwrite my local fix before completing my PR.
P2P fails on restart because rawdb.ReadCurrentHeader returns a nil header. It looks like ReadHeadHeaderHash fails to find the current header hash. However the correct hash is returned by ReadHeadBlockHash.
b35c37b
to
368beeb
Compare
P2P fails on restart because rawdb.ReadCurrentHeader returns a nil header. It looks like ReadHeadHeaderHash fails to find the current header hash. However the correct hash is returned by ReadHeadBlockHash.