-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Introduce compact block headers #1999
base: testnet3
Are you sure you want to change the base?
Conversation
// Read the compact batch header. | ||
let compact_batch_header = CompactBatchHeader::read_le(&mut reader)?; | ||
// Read the number of signatures. | ||
let num_signatures = 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 may want to introduce an additional limit here, like in https://github.com/AleoHQ/snarkVM/pull/1985
// Construct the signatures. | ||
let signatures = batch_certificate | ||
.signatures() | ||
.zip_eq(batch_certificate.timestamps()) |
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 might want to first compare lengths and return an error, zip_eq
will panic on different lengths after all
/// Returns the median timestamp of the batch ID from the committee. | ||
pub fn median_timestamp(&self) -> i64 { | ||
let mut timestamps = | ||
self.timestamps().chain([self.compact_batch_header.timestamp()].into_iter()).collect::<Vec<_>>(); |
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.
self.timestamps().chain([self.compact_batch_header.timestamp()].into_iter()).collect::<Vec<_>>(); | |
self.timestamps().chain(Some(self.compact_batch_header.timestamp())).collect::<Vec<_>>(); |
impl<N: Network> Hash for CompactBatchCertificate<N> { | ||
fn hash<H: Hasher>(&self, state: &mut H) { | ||
self.compact_batch_header.batch_id().hash(state); | ||
(self.signatures.len() as u64).hash(state); |
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.
nit: it's probably not necessary to hash the number of signatures if all of them get hashed individually as well
timestamp.write_le(&mut preimage)?; | ||
} | ||
// Hash the preimage. | ||
N::hash_bhp1024(&preimage.to_bits_le()) |
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.
not sure if all the components impl ToBits
, but it would be faster to use write_bits_le
instead of write_le
to avoid the extra conversion and allocation of to_bits_le
let timestamp = i64::read_le(&mut reader)?; | ||
|
||
// Read the number of transmission IDs. | ||
let num_transmissions = 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.
some comment as in https://github.com/AleoHQ/snarkVM/pull/1999#discussion_r1332752799
/// The timestamp. | ||
timestamp: i64, | ||
/// The set of index references for the `transmission IDs`. | ||
transmission_ids_map: IndexSet<(TransmissionType, u32)>, |
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 think I've seen this collection ever being queried or things being removed from it; if only the order of entries is important, perhaps a plain Vec<(TransmissionType, u32)>
would suffice instead?
/// The set of index references for the `transmission IDs`. | ||
transmission_ids_map: IndexSet<(TransmissionType, u32)>, | ||
/// The batch certificate IDs of the previous round. | ||
previous_certificate_ids: IndexSet<Field<N>>, |
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.
ditto, though at least here I can see the potential for things being queried for
certificate_id.write_le(&mut preimage)?; | ||
} | ||
// Hash the preimage. | ||
N::hash_bhp1024(&preimage.to_bits_le()) |
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.
// Read the round. | ||
let round = u64::read_le(&mut reader)?; | ||
// Read the number of 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.
// Initialize a set for the already ordered certificates. | ||
let mut already_ordered = HashSet::new(); | ||
// Initialize a buffer for the certificates to order, starting with the leader certificate. | ||
let mut buffer = subdag.iter().next_back().map_or(Default::default(), |(_, leader)| leader.clone()); |
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.
let mut buffer = subdag.iter().next_back().map_or(Default::default(), |(_, leader)| leader.clone()); | |
let mut buffer = subdag.iter().next_back().map_or_else(|| Default::default(), |(_, leader)| leader.clone()); |
continue; | ||
} | ||
// Insert the previous certificate into the buffer. | ||
buffer.insert(previous_certificate.clone()); |
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 wonder if the buffer
couldn't hold references instead of owned values
} | ||
|
||
/// Returns the transmission type for the given numeric identifier. | ||
pub fn from(type_id: u8) -> Result<Self> { |
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.
nit: this should instead be an impl TryFrom<u8> for TransmissionType
; From
conversions are infallible
Motivation
This PR introduces
CompactBatchHeader
,CompactBatchCertificate
, andCompactSubdag
by updating the vec of transmission ids in the batch header into a vec of index references. This is safe, because these references point to transactions, ratifications, and solutions that should already be included in the block. This will help reduce the block size by 1 TransmissionID (32 bytes) per transmission.