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

BlockWitness: pass inputs to the VM through the advice provider #516

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Oct 10, 2024

This PR refactors the BlockWitness: it no longer uses the operand stack to provide the inputs to the VM, it uses only advice inputs instead.

Closes: #78.

@Fumuran Fumuran requested review from bobbinth and plafer October 10, 2024 23:15
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

Code LGTM but I can't really comment on the masm changes. Time for me to learn some masm I suppose :D

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! Not a super deep review from me - but I did leave a couple of comments inline. The main one is about crating issues in miden-base to flash out inputs/outputs of batch and block kernels. If you create some basic issues, i'll try to fill in the details a bit later.

let num_produced_nullifiers: Felt = (self.produced_nullifiers.len() as u64)
.try_into()
.expect("nullifiers number is greater than or equal to the field modulus");
pub(super) fn into_program_inputs(self) -> Result<AdviceInputs, BlockProverError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably still keep the old interface (i.e., return (AdviceInputs, StackInputs)) even if we currently return StackInput::default() for stack inputs.

Comment on lines +216 to 224
#! Inputs:
#! Operand stack: []
#! Advice stack: [<account root inputs>, <note root inputs>, <nullifier root inputs>, <chain mmr root inputs>]
#! Advice map: {
#! PREV_CHAIN_MMR_HASH: [NUM_LEAVES, [peak_i], <maybe padding>]
#! }
#! Outputs:
#! Operand stack: [ACCOUNT_ROOT, NOTE_ROOT, NULLIFIER_ROOT, CHAIN_MMR_ROOT]
begin
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but we should be providing some inputs via the operand stack - otherwise this can be used to provide any arbitrary batch inputs.

Let's create 2 issues in miden-base for this: one issue for defining batch kernel inputs/outputs, and the other one for defining block kernel inputs/outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobbinth I created two issue templates: 0xPolygonMiden/miden-base#919 and 0xPolygonMiden/miden-base#920, although didn't manage to fill them properly, I'm not sure I understand what should be done.

@Mirko-von-Leipzig Mirko-von-Leipzig merged commit 0e352bd into next Oct 14, 2024
8 checks passed
@Mirko-von-Leipzig Mirko-von-Leipzig deleted the andrew-block-witness-change-inputs-to-vm branch October 14, 2024 16:17
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