-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Optimize] Store pending certificates to proposal cache file #3247
Conversation
@@ -144,15 +159,20 @@ impl<N: Network> FromBytes for ProposalCache<N> { | |||
}; | |||
// Deserialize `signed_proposals`. | |||
let signed_proposals = SignedProposals::read_le(&mut reader)?; | |||
// Read the number of pending certificates. | |||
let num_certificates = u32::read_le(&mut reader)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to safety check this number so it does not overallocate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in AleoHQ@7ed91da
for (_, certificates_for_round) in rounds.clone().sorted_by(|a, _, b, _| a.cmp(b)) { | ||
// Iterate over the certificates for the round. | ||
for (certificate_id, _, _) in certificates_for_round { | ||
// Skip the certificate if it already exists in the ledger. | ||
if self.ledger.contains_certificate(&certificate_id).unwrap_or(false) { | ||
continue; | ||
} | ||
|
||
// Add the certificate to the pending certificates. | ||
match certificates.get(&certificate_id).cloned() { | ||
Some(certificate) => pending_certificates.insert(certificate), | ||
None => continue, | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljedrz can you look into using cfg_iter
and/or cfg_sorted_by
to rayonify (conditionally with feature = 'serial'
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you implement the perf improvement here @ljedrz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snarkOS branch is ready, just needs https://github.com/AleoHQ/snarkVM/pull/2450 to be merged.
*self.propose_lock.lock().await = latest_certificate_round; | ||
|
||
// Update the storage with the pending certificates. | ||
for certificate in pending_certificates { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be sped up @ljedrz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything obvious, but I'll investigate it further.
Motivation
This PR is an extension of https://github.com/AleoHQ/snarkOS/pull/3239 that addresses https://github.com/AleoHQ/snarkOS/issues/3171.
This PR adds to the functionality of the
ProposalCache
by adding pending certificates (that have not yet been included in ledger) to the list of attributes to store to file.This should increase the robustness of reboots because nodes should now not need to rely on
PrimaryPings
to catch up to other peers in a reboot scenario. Initial testing has observed successful restarts (in the honest case) for any number of validators (including scenarios of more thanf+1
validators rebooting).Test Plan
Burn-in tests and additional reboot testing needs to be done to ensure robustness of this change.
Related PRs
Extension of https://github.com/AleoHQ/snarkOS/pull/3239