-
Notifications
You must be signed in to change notification settings - Fork 4
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
analyzer/consensus: Skip nonexistent mainnet block 8048955 #677
Conversation
bdd4537
to
5faadc5
Compare
analyzer/consensus/consensus.go
Outdated
if err != nil { | ||
if strings.Contains(err.Error(), fmt.Sprintf("%d must be less than or equal to the current blockchain height", height)) { | ||
return analyzer.ErrOutOfRange | ||
isBlockAbsent := m.chain == common.ChainNameMainnet && height == 8048955 |
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 feel like this could go as some kind of addition to the History config. this is the height just below when mainnet damask started. maybe we could put something in the mainnet cobalt record to say height 8048955 was absent
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.
That was my first instinct too. But Jernej was saying how this was definitely a bug/mistake when doing the Damask dump-restore. In the future, a) dump-restores are unlikely, b) if they do happen, we're unlikely to make the same mistake twice. So it's IMO an overgeneralization to bloat the history config with this, and make the consuming code (= here) more complex and general and indirect. Let's expand History once we have more than one such block in the future.
I also thought about at least referencing the Damask genesis height from History config here. So instead of comparing to 8048955, I could compare to (genesis_height - 1). But then I decided that's no more informative, and just harder to grep for.
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.
opposite opinion from the same principles:
- it'll take at least a little code change no matter what
- it happened once and we anticipate not again, so this only exists as a historical artifact
- better to shove the details into the "history" data rather than bloat the code with special (name, height) coordinates
ok on not optimizing generality or auto computing the height though
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.
this would prevent us from having to add this special purpose chain name arg throughout
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.
extra thing: I'm against doing special behavior for named default chains that a custom history config can't do. we've used custom history to work around things before, don't want to lose this fix if we need to do that later
5faadc5
to
20669d7
Compare
4152c29
to
823c2cd
Compare
823c2cd
to
2f7d9f2
Compare
Ok updated to address comments. Tried testing on the same range as the PR description but it's having issues connecting to the production cobalt archive node. |
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.
thanks for the changes, still looks very clean
2f7d9f2
to
1d833f8
Compare
On Oasis mainnet, consensus block 8048955 does not exist: 8048954 is the last block of Cobalt, and 8048956 is the first block of Damask. This was an unintended side effect of the dump-restore Damask upgrade.
Nexus was unaware of this, and would get stuck on that block when reindexing the entire history. The only way to "unstick" it was (small) manual DB surgery. This PR hardcodes the block as an exception, and makes Nexus skip it.
Testing:
One-off manual test. Ran mainnet analysis with the following config:
and verified that blocks are analyzed as expected.