Introduce compact block headers #1999
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.
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.
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.
| 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.
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.
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.
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.
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.
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.
| // 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.
| // 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.
| 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.
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.
nit: this should instead be an impl TryFrom<u8> for TransmissionType; From conversions are infallible
Motivation
This PR introduces
CompactBatchHeader,CompactBatchCertificate, andCompactSubdagby 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.