migration: validate that migrationary blocks are VoM #566
Conversation
AshwinSekar
force-pushed
the
migration-VoM-validation
branch
from
e8b5a68 to
d59ef21
Compare
Split from #502 #### Problem When we are in the migrationary period we don't want to include user txs in blocks. #### Summary of Changes Change `maybe_start_leader` to make leader banks as VoM. This value is used by banking stage to only include vote txs in packing. We do not need to undo this after genesis as replay stage is no longer responsible for starting leader - block creation loop will create leader banks as normal. NOTE: this is best effort, e.g. a malicious guy could comment this out. Validation will be handled in #566
AshwinSekar
force-pushed
the
migration-VoM-validation
branch
from
d59ef21 to
3e226f9
Compare
| entry, | ||
| starting_index: tx_starting_index, | ||
| .map(|(entry, tx_starting_index)| { | ||
| // If bank is in vote-only mode, validate that entries contain only vote transactions |
There was a problem hiding this comment.
for some context the reason we never validated this before was because VoM is based on a local-trigger (tower distance) so we could end up rejecting blocks from honest nodes.
I believe that this is safe, since the migration slot is known via a rooted bank. The only scenario where this could be an issue is:
- Migration happens very quickly, first vote only bank is S
- Everyone purges S and it's descendants and we remake S and further slots as alpenglow
- Our node is really slow / has network issues somehow misses the tower bft blocks, and instead sees the alpenglow version of S and its descendants.
In this scenario we'd mark the Alpenglow block as dead because it's not vote only - however we would already mark it as dead because it's missing PoH ticks.
The only way to remedy this situation is to wait for our slow node to eventually see the genesis certificate / a finalization certificate and then we'd purge, repair, and re-replay the Alpenglow blocks as not vote only.
There was a problem hiding this comment.
For the current, Tower case, we only ever mark are our own leader slot banks as VoM, right?
So they wouldn't actually go through this filtering process IIUC, which seems fine.
There was a problem hiding this comment.
yep we don't validate (and with this change won't validate) tower blocks as VoM.
Only concern is if this validation could screw up something in Alpenglow which I don't believe it can
There was a problem hiding this comment.
Our node is really slow / has network issues somehow misses the tower bft blocks, and instead sees the alpenglow version of S and its descendants.
If we check for the migration certificate in the block header, could we avoid this?
| ); | ||
| // Migration period banks are VoM | ||
| let options = NewBankOptions { | ||
| vote_only_bank: child_slot >= migration_slot && genesis_slot.is_none(), |
There was a problem hiding this comment.
Along the lines of this comment: #560 (review)
I think it would be nice to have a MigrationStatus API that just answered this question for us.
Something like migration_status.slot_in_tower_to_alpenglow_transition(child_slot)
There was a problem hiding this comment.
Agreed, i'll put up a PR improving the MigrationStatus API and then merge this after
| entry, | ||
| starting_index: tx_starting_index, | ||
| .map(|(entry, tx_starting_index)| { | ||
| // If bank is in vote-only mode, validate that entries contain only vote transactions |
There was a problem hiding this comment.
For the current, Tower case, we only ever mark are our own leader slot banks as VoM, right?
So they wouldn't actually go through this filtering process IIUC, which seems fine.
| if let EntryType::Transactions(ref transactions) = entry { | ||
| if is_vote_only_bank | ||
| && transactions | ||
| .iter() | ||
| .any(|tx| !tx.is_simple_vote_transaction()) |
There was a problem hiding this comment.
thinking about performance, this seems fine. is_vote_only_bank always false except during the transition (in which case load is already lighter. And even then, I believe these are RuntimeTransaction types, so the is_simple_vote_transaction is super cheap
There was a problem hiding this comment.
Also, is the check on number of transactions is bounded at this point based on shred count and compute units?
3 participants
Split from #502
Problem
Blocks in the migration period should not contain user transactions. #565 handles this on the block packing side, but we need verification on the replay side.
Summary of Changes
Mark migration period blocks as vote only. During replay mark vote only blocks which contain user transactions as dead.