Lr/variable stake table #3893
Lr/variable stake table #3893
Conversation
| let proposal_block_number = proposal.data.block_header.block_number(); | ||
| let proposal_epoch = TYPES::Epoch::new(epoch_from_block_number( | ||
| proposal_block_number, | ||
| validation_info.epoch_height, | ||
| )); | ||
| let justify_qc_epoch = if validation_info.epoch_height != 0 | ||
| && proposal_block_number % validation_info.epoch_height == 1 | ||
| { | ||
| // if the proposal is for the first block in an epoch, the justify QC must be from the previous epoch | ||
| proposal_epoch - 1 | ||
| } else { | ||
| // otherwise justify QC is from the same epoch | ||
| proposal_epoch | ||
| }; |
There was a problem hiding this comment.
is it worth making this a helper function? i.e. take a proposal and the previous block header and return the proposal's epoch number
There was a problem hiding this comment.
yes, I can make it into a helper function but I didn't fully understand what you mean. This piece of code calculates the proposed block's epoch and the justify QC block's epoch. Do you want the function to return both?
There was a problem hiding this comment.
I guess I meant:
- the proposed block's epoch is already
epoch_from_block_number(proposal.data.block_header.block_number())(straightforward) - the preceding block's epoch is the check based on the block height modulo the epoch height, but we could make a function like
preceding_block_epoch(proposal, epoch_height)
(maybe these could even be generic in a HasBlockNumber trait or something)
it's not that important though!
There was a problem hiding this comment.
Got it! yeah, these are good ideas. Can we agree to make it a separate PR? I don't think this exact code is repeated anywhere else so it's not that it's non-DRY at the moment. I've already created a trait called HasEpoch so we may extend this one if it makes sense. I also think it'll be nice to clean the whole epoch-related code up after everything is more or less finished. I would of course take care of that.
There was a problem hiding this comment.
yes, I agree -- I think it's fine to merge as-is!
Closes #<ISSUE_NUMBER>
This PR:
TestTwoStakeTablesTypeswhich uses a newMembershipMembershipcalledTwoStaticCommittees. The membership is used to test the basic scenarios where we have two different stake tables. It works in the following way: takes the nodes created in the test suite and splits the in half based on the even/odd node id. The even id nodes are added to the stake table used for even epochs and the odd id nodes are added to the stake table used for odd epochs.LeafandViewInner. The epoch fields are used to validate the messages against the proper stake table.This PR does not:
Key places to review: