Skip to content
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

Allow blocks with empty batches #1172

Merged
merged 10 commits into from
Feb 24, 2025
Merged

Allow blocks with empty batches #1172

merged 10 commits into from
Feb 24, 2025

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Feb 20, 2025

Allow blocks with empty batches so we can build new blocks in the node even if no transactions or batches are available.

Also clarifies comments in the proposed batch.

Consistently name created_nullifiers method across ProposedBatch, ProposedBlock and ProvenBlock.

Base automatically changed from pgackst-batch-expiration to next February 20, 2025 19:35
@@ -130,13 +130,9 @@ impl ProposedBlock {
batches: Vec<ProvenBatch>,
timestamp: u32,
) -> Result<Self, ProposedBlockError> {
// Check for empty or duplicate batches.
// Check for duplicate and max number of batches.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-PR question, but why does the protocol care about defining maximum counts? Feels like something that should be configurable - and while the node can enforce its own limits, this still sets the upper cap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good question. Some kind of ProtocolConfig would be better I guess. I guess right now it makes it easier to verify consistency, for example: The max number of inputs per batch and the max number of batches per block (1024 * 64 = 2^16) guarantees we can use a BlockNoteTree of depth 16 to represent all notes created in a block.

But we could verify such invariants in a ProtocolConfig::new constructor, and then miden-base could assume it is consistent.

// --------------------------------------------------------------------------------------------

for tx in transactions.iter() {
if block_header.block_num() != tx.block_num()
if reference_block_header.block_num() != tx.block_num()
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be better :)

transactions
  .iter()
  .filter(|tx| tx.block_num != reference_block_header.block_num())
  .find_map(|tx| chain_mmr.contains_block(tx.block_num()).not().then_some(tx))
  .map_or(|tx| Err(ProposedBatcError:: ...)?;

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@PhilippGackstatter
Copy link
Contributor Author

I pushed one more change related to this comment: 0xPolygonMiden/miden-node#709 (comment), which is to make BlockNoteIndex::new returns Option<Self> instead of panicking, so that users have control over the panic and it's more obvious that it panics since we have to unwrap or expect.

@PhilippGackstatter PhilippGackstatter merged commit f2d50bf into next Feb 24, 2025
12 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-empty-blocks branch February 24, 2025 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants