fix(core): always pass the correct timestamp window to header validatior #5624
Conversation
sdbondi
force-pushed
the
core-validation-minor-fix-ts-count
branch
5 times, most recently
from
ebab778
to
cc27e92
Compare
sdbondi
changed the title
ghpbot-tari-project
added
P-acks_required
sdbondi
force-pushed
the
core-validation-minor-fix-ts-count
branch
3 times, most recently
from
6ba4bc2
to
4f2f415
Compare
sdbondi
force-pushed
the
core-validation-minor-fix-ts-count
branch
from
4f2f415
to
a1ff08e
Compare
| let expected_timestamp_count = cmp::min(consensus_constants.median_timestamp_count() as u64, header.height); | ||
| // Empty `timestamps` is never valid | ||
| if timestamps.is_empty() { | ||
| return Err(ValidationError::IncorrectNumberOfTimestampsProvided { | ||
| expected: expected_timestamp_count, | ||
| actual: 0, | ||
| }); | ||
| } | ||
|
|
||
| if timestamps.len() as u64 != expected_timestamp_count { | ||
| return Err(ValidationError::IncorrectNumberOfTimestampsProvided { | ||
| actual: timestamps.len() as u64, |
There was a problem hiding this comment.
This code looks fine.
but tbh I don't really understand it just yet. It looks like we're receiving a slice of timestamps and seeing if the number of timestamps is the same as the median timestamp count. But this slice could contain the same single timestamp repeated and this sanity check passes.
So what I don't yet understand is why we're using this method of counting timestamps as a validation.
There was a problem hiding this comment.
Yup the blockchain db code is super hard to reason about. It's not strictly a header validation since the timestamps come from headers previously inserted into the main/orphan chains so the contract we're making in the HeaderChainLinkedValidator trait is that the caller must provide the correct timestamps. This is why I renamed it to sanity_check because it's more about asserting an invariant we require when calling the validator. The other option is to take it out completely or to panic, but I went with the least risky approach.
sdbondi
changed the title
sdbondi
force-pushed
the
core-validation-minor-fix-ts-count
branch
from
00c6fb4
to
ab2f300
Compare
ghpbot-tari-project
removed
the
P-reviews_required
Description
fix(core): sanity check for timestamp window length checks for the correct length of the timestamp window
fix(core): always include the correct timestamp window to header validation
tests(core): add a reorg test of greater size than median timestamp window size
chore: remove allocations when logging hashes in blockchain database
Motivation and Context
sanity_check_timestamp_count(previouslycheck_timestamp_count) allowed timestamp windows ofmin(median_window_size, header.height - 1)or greater. However, the window size must always be the correct size for a given block. If it is not, the median timestamp calculation will be incorrect. This check also subtracted 1 from the header height BEFORE checking the height is correct. This would cause a panic/underflow if a header of height 0 is passed in.In the reorg logic, we passed in the same timestamp window for the candidate block when validating orphan chains, this could cause correct reorgs to fail. This PR also ensures that timestamps are sorted as this is required for a correct median calculation.
How Has This Been Tested?
Adds a new unit test
it_does_a_sanity_check_on_the_number_of_timestamps_providedAdds block chain unit test
it_links_many_orphan_branches_to_main_chain_with_greater_reorg_than_median_timestamp_windowExisting test
it_links_many_orphan_branches_to_main_chainfailed after the fix tocheck_timestamp_count. Fixed in this PRManually caused a about 90 block reorg on localnet nodes
What process can a PR reviewer use to test or verify this change?
Create a reorg using localnet
Breaking Changes