Reject certificates older than root and add certificate pool stats #311
Conversation
votor/src/certificate_pool/stats.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn reset(&mut self) { |
There was a problem hiding this comment.
could just set to default here
votor/src/certificate_pool/stats.rs
Outdated
| ); | ||
|
|
||
| datapoint_info!( | ||
| "certificate_pool_ingested_votes_by_type", |
There was a problem hiding this comment.
could probably drop the _by_type postfix for these metrics just to reduce verbosity
votor/src/certificate_pool/stats.rs
Outdated
|
|
||
| if self.ingested_votes_by_type.len() <= index { | ||
| self.ingested_votes_by_type | ||
| .resize(index.saturating_add(1), 0); |
There was a problem hiding this comment.
don't love this.. maybe we could use something like strum to help derive number of enums and set the Vec size properly up front?
There was a problem hiding this comment.
Actually, since the type of Vote/Cert in Alpenglow will not change that fast, I can use the last_constant + 1. If we ever change this, we should see panic in tests because I was using array[index]. Does that work?
votor/src/certificate_pool/stats.rs
Outdated
| pub(crate) event_safe_to_notarize: u32, | ||
|
|
||
| pub(crate) exits: [u32; 2], | ||
| pub(crate) incoming: [u32; 2], |
There was a problem hiding this comment.
probably easier to just have incoming_votes and incoming_certs fields
votor/src/certificate_pool/stats.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn incr_u32_array(value: &mut [u32; 2], is_vote: bool) { |
There was a problem hiding this comment.
I'm assuming we could get rid of this if we get rid of the arrays
votor/src/certificate_pool/stats.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn incr_u32(value: &mut u32) { |
There was a problem hiding this comment.
Do we really need this? I think we can just call self.value = self.value.saturating_add(1); directly
There was a problem hiding this comment.
I was just being lazy, removed.
bw-solana
left a comment
bw-solana
left a comment
There was a problem hiding this comment.
Overall, LGTM!
Only thing I don't love is burying stats reporting down deep and being reliant on receiving a BLS message (although I agree this should be happening pretty much all the time)
votor/src/certificate_pool.rs
Outdated
| events: &mut Vec<VotorEvent>, | ||
| ) -> Result<(Option<Slot>, Vec<Arc<CertificateMessage>>), AddVoteError> { | ||
| // We add stats reporting here because we should almost always have a message. | ||
| self.stats.maybe_report(); |
There was a problem hiding this comment.
I'm thinking this might fit better higher up the stack (like in certificate_pool_ingest main loop). If we move it up there, we could even make this a sub-component of the CertificatePoolServiceStats reporting
There was a problem hiding this comment.
Hmm I don't think the two stats should be mixed together, they each maintain the status of the corresponding data structure.
But I can move the trigger there. Done.
| stats.maybe_report(); | ||
| if stats.maybe_report() { | ||
| // Stats were reported, report the certificate pool stats | ||
| cert_pool.report_stats(); |
There was a problem hiding this comment.
if we're going to decouple the service and pool stats, might as well fully decouple them and just call something like this in succession:
stats.maybe_report();
cert_pool.maybe_report();
…penglow into add_certificate_pool_stats
2 participants
Add CertificatePoolStats as promised in https://github.com/anza-xyz/alpenglow/pull/307/files#r2223736794
During the process, also realized we are not currently rejecting certificates older than root, which is a bug.
Fixed bug and added tests.