fix: correctly detect first sync on headers stage #12085
Conversation
| self.hash_collector.insert(hash.key()?, 0)?; | ||
| cursor_header_numbers.delete_current()?; | ||
| first_sync = true; | ||
| if provider.tx_ref().entries::<RawTable<tables::HeaderNumbers>>()? == 1 { |
There was a problem hiding this comment.
I'm not sure I understand how we can end up in a state where the original check would not be sufficient, can you expand on it a bit?
There was a problem hiding this comment.
yep, in my case we had a chain of hashes like this (just prefixes): [0xe, 0x2, 0xb, 0x9]
because .last() is returning the lexicographically highest hash in database, for all of those imports we ended up with first_sync = true because we've always got the genesis hash (0xe)
this was fine for first 2 blocks because their hashes were increasing and the table keys evolved like [0xe] -> [] -> [0x2, 0xe] -> [0x2] -> [0x2, 0xb, 0xe]
however on the 0x9 block we tried to do [0x2, 0xb, 0xe] -> [0x2, 0xb] -> [0x2, 0xb, 0x9] which is incorrect because 0x9 should go before 0xb
hopefully this makes sense..
There was a problem hiding this comment.
basically it is edge case which can occur if we have only few blocks in database and hash of genesis block is higher than hashes of all others
likely impossible during normal sync
There was a problem hiding this comment.
ahh right i forgot this table is keyed by hash
| if provider.tx_ref().entries::<RawTable<tables::HeaderNumbers>>()? == 1 { | ||
| if let Some((hash, block_number)) = cursor_header_numbers.last()? { |
There was a problem hiding this comment.
because this table is keyed by hash, we definitely should check length as well
There was a problem hiding this comment.
I wonder if there's a perf diff first vs last but really doesn't matter here I assume
onbjerg
added
C-bug
|
@onbjerg tested locally and this seems to fix all left hive tests |
|
thank you sir, great work! |
We need to ensure that we only have a single entry here because this table is keyed by hash