refactor: do not store epoch related state in CertificatePool#360
refactor: do not store epoch related state in CertificatePool#360
CertificatePool#360Conversation
akhi3030
force-pushed
the
akhi3030/cert-pool-no-epoch-stakes
branch
3 times, most recently
from
64bddef to
7100785
Compare
akhi3030
changed the title
CertificatePool
akhi3030
force-pushed
the
akhi3030/cert-pool-no-epoch-stakes
branch
2 times, most recently
from
252b512 to
8c2fd9a
Compare
akhi3030
force-pushed
the
akhi3030/cert-pool-no-epoch-stakes
branch
3 times, most recently
from
4d4705d to
8dc2224
Compare
akhi3030
changed the title
CertificatePoolCertificatePool
| let (new_finalized_slot, new_certificates_to_send) = cert_pool.add_message( | ||
| bank.epoch_schedule(), | ||
| bank.epoch_stakes_map(), | ||
| bank.slot(), |
There was a problem hiding this comment.
Just curious, why is passing three members of a Bank better than passing bank itself?
There was a problem hiding this comment.
because then the pool depends upon the bank for information that only changes once an epoch. We need to clean things up so that we are not unnecessarily using banks for information that should be stored in an object like EpochInfo.
akhi3030
force-pushed
the
akhi3030/cert-pool-no-epoch-stakes
branch
from
ef2061c to
155111b
Compare
| pub(crate) certificates_sent: u16, | ||
| pub(crate) certificates_dropped: u16, | ||
| pub(crate) new_finalized_slot: u16, | ||
| pub(crate) new_root: u16, |
There was a problem hiding this comment.
Maybe we should add a count for cleanup we've done....
There was a problem hiding this comment.
do you mean the number of times we have called cleanup?
There was a problem hiding this comment.
Yup, so if suddenly we don't accept any incoming votes/certs we can check whether a cleanup happened.
| } | ||
|
|
||
| #[derive(Default)] | ||
| fn get_key_and_stakes( |
There was a problem hiding this comment.
this also probably shouldn't be in certificate_pool
| let vote_type = VoteType::get_type(vote); | ||
| if let Some(conflicting_type) = | ||
| self.has_conflicting_vote(slot, vote_type, &validator_vote_key, &block_id) | ||
| self.has_conflicting_vote(vote_slot, vote_type, &validator_vote_key, &block_id) |
There was a problem hiding this comment.
this should not to be part of this PR, but why do we turn the rank into the vote key and then use the vote key to index validators? I think we should only use the rank within all of Pool
|
|
||
| fn add_certificate( | ||
| &mut self, | ||
| root_slot: Slot, |
There was a problem hiding this comment.
this should not care about the root at all, but I guess that will be part of #368
|
This PR was accidentally merged. I'll make a new one with some of the changes requested above. |
|
#384 covers the remaining review comments. |
…nza-xyz#360) Instead of caching epoch related state in the CertificatePool, pass it into the pool. This removes dependencies between banks and other related structs like the RootBankCache, and BankForks, etc. Also updates cleanup() to use the highest finalized slot instead of the root slot which is in line with the paper.
5 participants
Instead of caching epoch related state in the
CertificatePool, pass it into the pool. This removes dependencies between banks and other related structs like the RootBankCache, and BankForks, etc.