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

Compute chain MMR root #71

Merged
merged 174 commits into from
Dec 4, 2023
Merged

Compute chain MMR root #71

merged 174 commits into from
Dec 4, 2023

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Nov 22, 2023

Closes: #68

Builds on: #64

Left to do:

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! I left just a few nits inline.

let prev_hash = witness.prev_header.prev_hash();
let block_num = witness.prev_header.block_num();
let prev_hash = witness.prev_header.hash();
let block_num = witness.prev_header.block_num() + Felt::ONE;
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: instead of using Felt::ONE we usually just use constant ONE (can be imported from miden base, i believe).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (also changed the Felt::ZERO to ZERO)

block-producer/src/block_builder/prover/block_witness.rs Outdated Show resolved Hide resolved
block-producer/src/block_builder/prover/block_witness.rs Outdated Show resolved Hide resolved
@plafer plafer marked this pull request as ready for review December 1, 2023 20:25
@plafer
Copy link
Contributor Author

plafer commented Dec 1, 2023

@bobbinth ready for final review! Created #79 as a followup that I'll work on first thing next week

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! I left a few minor comments inline.

block-producer/src/block_builder/prover/block_witness.rs Outdated Show resolved Hide resolved
pub struct BlockWitness {
pub(super) updated_accounts: BTreeMap<AccountId, AccountUpdate>,
/// (batch_index, created_notes_root) for batches that contain notes
pub(super) batch_created_notes_roots: Vec<(usize, Digest)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to using Vec<(usize, Digest)> instead of BTreeMap<usize, Digest>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to BTreeMap to be consistent with updated_accounts, but I think Vec would be better (for both) than BTreeMap, since it's more cache efficient (everything's in the same chunk of memory as opposed to be more spread out with the BTreeMap), and we don't actually need any operations from the BTreeMap.

But this is a minor detail and BTreeMap is fine for now

block-producer/src/block_builder/prover/block_witness.rs Outdated Show resolved Hide resolved
block-producer/src/test_utils/proven_tx.rs Outdated Show resolved Hide resolved
block-producer/src/block_builder/prover/mod.rs Outdated Show resolved Hide resolved
@plafer plafer merged commit 9a5e93b into main Dec 4, 2023
@plafer plafer deleted the plafer-chain-mmr-root branch December 4, 2023 14:14
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.

Block kernel: Compute chain MMR root
2 participants