Modify ticks and hashes_per_tick check for alpenglow blocks #98
Conversation
|
|
||
| if let Some(first_alpenglow_slot) = bank | ||
| .feature_set | ||
| .activated_slot(&solana_feature_set::secp256k1_program_enabled::id()) |
There was a problem hiding this comment.
nit: I think I keep seeing this check everywhere, is it possible we do it in one place and pass around?
There was a problem hiding this comment.
i think it's uglier to do that, you would have to add arguments to all these functions that already take a bank
core/src/consensus/progress_map.rs
Outdated
| // 2. One extra tick for the actual alpenglow slot | ||
| (first_alpenglow_slot - parent_slot - 1) * bank.ticks_per_slot() + 1 | ||
| } else { | ||
| 1 |
There was a problem hiding this comment.
Add comment that in Alpenglow world we have no tick for skipped slots?
core/src/consensus/progress_map.rs
Outdated
| 1 | ||
| } | ||
| }; | ||
| bank.set_tick_height(bank.max_tick_height() - num_expected_ticks); |
There was a problem hiding this comment.
Do you think we need to add assertion here this value is something reasonable?
There was a problem hiding this comment.
it's strictly less than max_tick_height() which is something reasonable
| // check above. Because in Alpenglow the last tick does not have any | ||
| // hashing guarantees, we pass everything but that last tick to the | ||
| // entry verification. | ||
| entries = &entries[..entries.len() - 1]; |
There was a problem hiding this comment.
Err, do we have to change entries inside verify_ticks? Can we do skip_verification for all Alpenglow blocks instead in the caller?
There was a problem hiding this comment.
we need to verify alpenglow blocks that have parents from before the epoch boundary because those parents have ticks, otherwise anybody could make a new alpenglow block with a very early parent from the previous epoch
There was a problem hiding this comment.
Can we someday do without that one tick in Alpenglow block?
| if bank.slot() >= first_alpenglow_slot && next_bank_tick_height == max_bank_tick_height { | ||
| if entries.is_empty() { | ||
| // This shouldn't happen, but good to double check | ||
| error!("Processing empty entries in verify_ticks()"); |
There was a problem hiding this comment.
If this shouldn't happen, should we return Ok(()) here?
There was a problem hiding this comment.
It shouldn't happen because I'm assuming the callers will not pass this function an empty set of entries to verify, i.e. blockstore shouldn't deserialize empty entries
But an empty set of entries always passes the hashes_per_tick() check, so returning Ok(()) is fine here
2 participants
Problem
Xand its parentYSummary of Changes
Fixes #