fix(tree): use in-memory data first to query total difficulty #11382
Conversation
fgimenez
added
C-bug
| .get_finalized_num_hash() | ||
| .unwrap_or_else(|| BlockNumHash::new(0, B256::default())); | ||
| self.database.header_td_by_number(last_finalized_num_hash.number) | ||
| } else if let Some(td) = self.database.header_td_by_number(number)? { |
There was a problem hiding this comment.
i think, if its not in memory we can just return whatever self.database method returns
There was a problem hiding this comment.
we won't ever hit that if we hardcoded the post merge ttd
but we can't use BlockNumHash::new(0, B256::default()) because here because this would return the genesis ttd.
this would only be reachable in custom setups, but the unwrap_or_else should use the last_block_number() instead, even if there's a race condition, but this only exists until there is a finalized block, so imo this is fine
There was a problem hiding this comment.
i think, if its not in memory we can just return whatever self.database method returns
good point, done here 36b11f7
There was a problem hiding this comment.
but we can't use
BlockNumHash::new(0, B256::default())because here because this would return the genesis ttd.
this would only be reachable in custom setups, but the unwrap_or_else should use thelast_block_number()instead, even if there's a race condition, but this only exists until there is a finalized block, so imo this is fine
yep, if get_finalized_num_hash returns None and we try to get last_block_number the test is flaky again, should we do that instead?
There was a problem hiding this comment.
I think the flaky test is fine, we know why it's flaky and why this isn't an issue.
also total difficulty will be removed from rpc so this case will eventually go away
There was a problem hiding this comment.
makes sense done here 418c48e ptal
not adding the hive test to expected failures bc it will be randomly passing/failing
| .get_finalized_num_hash() | ||
| .unwrap_or_else(|| BlockNumHash::new(0, B256::default())); | ||
| self.database.header_td_by_number(last_finalized_num_hash.number) | ||
| } else if let Some(td) = self.database.header_td_by_number(number)? { |
There was a problem hiding this comment.
we won't ever hit that if we hardcoded the post merge ttd
but we can't use BlockNumHash::new(0, B256::default()) because here because this would return the genesis ttd.
this would only be reachable in custom setups, but the unwrap_or_else should use the last_block_number() instead, even if there's a race condition, but this only exists until there is a finalized block, so imo this is fine
fgimenez
removed
the
C-hivetest
| // Otherwise, return what we have on disk for the input block | ||
| number | ||
| }; | ||
| self.database.header_td_by_number(number) |
There was a problem hiding this comment.
ah only updating the number is even better
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Flips the order of data retrieval in
header_td_by_number, first in-memory, then db.Additionally, to prevent races with the persistence task in cases of reorgs, when the required block is found in memory it uses the last finalized block to get the total difficulty, if none is found we will use the last block number found in memory (in this case we are still exposed to reorgs)
ref #10941