Public kernel cbind errors #437

rahul-kothari · 2023-05-03T11:06:38Z

Description

Capture public kernel circuit failures and bubble up in TS (commit 1)
Fix mocks in circuits.js (commit 2)
Fix tests in sequencer, circuits! (commit 2)
PS: Ignore test.cpp -> just linting changes

Checklist:

I have reviewed my diff in github, line by line.
Every change is related to the PR description.
I have linked this pull request to the issue(s) that it resolves.
There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
The branch has been merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp

Maddiaa0

I've only commented on a small number of things, leaving out the majority of the public circuits stuff as I'm not 100% familiar

circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp

yarn-project/circuits.js/src/tests/factories.ts

PhilWindle

Looks good to me

dbanks12

LGTM

spalladino · 2023-05-03T17:18:31Z

yarn-project/sequencer-client/src/sequencer/public_processor.ts

+    // set the msgSender for each call in the call stack
+    for (let i = 0; i < publicCallStackPreimages.length; i++) {
+      const isDelegateCall = publicCallStackPreimages[i].publicInputs.callContext.isDelegateCall;
+      publicCallStackPreimages[i].publicInputs.callContext.msgSender = isDelegateCall
+        ? callStackItem.publicInputs.callContext.msgSender
+        : callStackItem.contractAddress;
+    }


Shouldn't the circuit skip these checks for empty call stack items? I'd expect that, in general, zero-items are not being checked.

Hmm, it currently doesn't, but it probably should. I wonder though if there should be a specific flag on the call stack item indicating if it is 'empty' rather than simply testing if 'xyz == 0' which seems rather arbitrary. What do you think @iAmMichaelConnor ?

I do see what you mean although there is a school of thought that that dictates we should not modify our actual code to handle testing variants

Also in a real circuit we can't skip things anyway

we should not modify our actual code to handle testing variants

Fully agree. The problem here is that these are not testing variants: it's that these empty values are used in actual code to pad to the necessary number of items in the array of call stack items. Anyhow, I think @PhilWindle was tackling this in the circuit. so we should be able to remove this once the isEmpty check is added there.

yarn-project/sequencer-client/src/sequencer/public_processor.test.ts

yarn-project/circuits.js/src/tests/factories.ts

spalladino · 2023-05-03T17:20:50Z

yarn-project/circuits.js/src/tests/factories.ts

@@ -127,7 +130,7 @@ export function makeEmptyAccumulatedData(seed = 1): CombinedAccumulatedData {
    fr(seed + 13),
    range(KERNEL_NEW_COMMITMENTS_LENGTH, seed + 0x100).map(fr),
    range(KERNEL_NEW_NULLIFIERS_LENGTH, seed + 0x200).map(fr),
-    range(KERNEL_PRIVATE_CALL_STACK_LENGTH, seed + 0x300).map(fr),
+    new Array(KERNEL_PRIVATE_CALL_STACK_LENGTH).fill(Fr.ZERO), // private call stack must be empty


Looking at the function name, shouldn't all of these fields actually be empty?

It's a badly named function (by me). Some items need to be empty and others not. It's coming back to that discussion around crafting a range of inputs in TS to satisfy circuit 'modes'.

yarn-project/circuits.js/src/tests/factories.ts

spalladino · 2023-05-03T17:23:22Z

yarn-project/circuits.js/src/structs/kernel/__snapshots__/index.test.ts.snap

@@ -838,7 +838,7 @@ current_value: 0x1514
 storage_slot: 0x1514
 current_value: 0x1515
 ]
-public_call_stack: [ 0x1611 0x1612 0x1613 0x1614 ]
+public_call_stack: [ 0x1f7ec2163f0e78b25257ca693b8606d5e6c940c36e2e77cd0aa1afbbf167e95a 0xbfa6e6e8892f68b806bca1fd20cf704dd5c449440b54ef2957a3d267342bda6 0x29618099103194faea4e2c1c2a77bbb165bfe89961c16369b797cfd38671a2ba 0x1403602a993517c978fd5ea6ea17c02280692bb4b84e38e1a92f8c6d1d7b858b ]


I'm on the fence about this change. On one hand, it's great to have "real" values for our factories. On the other, it makes it a lot more difficult to identify issues with the serialization.

I guess that troubleshooting serialization will become less important given @ludamad's upcoming changes, so we can keep going in this direction. WDYT?

yarn-project/circuits.js/src/abis/abis.ts

yarn-project/types/src/tx.ts

LHerskind

Looks good to me.

yarn-project/circuits.js/src/utils/call_wasm.ts

spalladino

Looks good, thanks for the changes! Feel free to merge.

yarn-project/sequencer-client/src/sequencer/public_processor.test.ts

rahul-kothari · 2023-05-04T14:47:04Z

Ah merging is blocked until @PhilWindle approves

sufficient reviews

* verification takes a pre-hashed message : Note: if len(hash) > 32 bytes, then bigfield will fail * use hashed_message when generating signature * modify acir structure and function to now use prehashed variant * message -> hashed_message

rahul-kothari force-pushed the rk/kernel_cbind_errors branch 5 times, most recently from 930ba00 to f6a3ea9 Compare May 3, 2023 15:41

rahul-kothari marked this pull request as ready for review May 3, 2023 15:41

rahul-kothari requested review from spalladino, PhilWindle, LHerskind, dbanks12 and Maddiaa0 May 3, 2023 15:41

rahul-kothari changed the title ~~Rk/kernel cbind errors~~ Public kernel cbind errors May 3, 2023

rahul-kothari force-pushed the rk/kernel_cbind_errors branch from f6a3ea9 to d9e1cab Compare May 3, 2023 16:13

dbanks12 reviewed May 3, 2023

View reviewed changes

circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp Outdated Show resolved Hide resolved

Maddiaa0 reviewed May 3, 2023

View reviewed changes

circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp Show resolved Hide resolved

yarn-project/circuits.js/src/tests/factories.ts Outdated Show resolved Hide resolved

yarn-project/circuits.js/src/tests/factories.ts Outdated Show resolved Hide resolved

PhilWindle approved these changes May 3, 2023

View reviewed changes

dbanks12 approved these changes May 3, 2023

View reviewed changes

spalladino reviewed May 3, 2023

View reviewed changes

PhilWindle previously requested changes May 3, 2023

View reviewed changes

yarn-project/types/src/tx.ts Outdated Show resolved Hide resolved

rahul-kothari force-pushed the rk/kernel_cbind_errors branch 2 times, most recently from 7fe95b0 to 1e6370b Compare May 4, 2023 11:29

rahul-kothari requested review from spalladino and PhilWindle May 4, 2023 11:32

rahul-kothari force-pushed the rk/kernel_cbind_errors branch from 1e6370b to ddde716 Compare May 4, 2023 11:51

LHerskind approved these changes May 4, 2023

View reviewed changes

yarn-project/circuits.js/src/utils/call_wasm.ts Show resolved Hide resolved

spalladino approved these changes May 4, 2023

View reviewed changes

yarn-project/sequencer-client/src/sequencer/public_processor.test.ts Show resolved Hide resolved

rahul-kothari added 3 commits May 4, 2023 14:35

cbinds for pub kernel + format public kernel tests

c80d491

fix tests

84a8ee4

fix per PR comments

517d98f

rahul-kothari force-pushed the rk/kernel_cbind_errors branch from ddde716 to d56ca5d Compare May 4, 2023 14:40

address PR comments

416557d

rahul-kothari force-pushed the rk/kernel_cbind_errors branch from d56ca5d to 416557d Compare May 4, 2023 14:42

rahul-kothari requested review from Maddiaa0 and PhilWindle and removed request for Maddiaa0 and PhilWindle May 4, 2023 14:43

PhilWindle approved these changes May 4, 2023

View reviewed changes

rahul-kothari merged commit 8c2810b into master May 4, 2023

rahul-kothari deleted the rk/kernel_cbind_errors branch May 4, 2023 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public kernel cbind errors #437

Public kernel cbind errors #437

rahul-kothari commented May 3, 2023 •

edited

Loading

Maddiaa0 left a comment

PhilWindle left a comment

dbanks12 left a comment

spalladino May 3, 2023 •

edited

Loading

PhilWindle May 3, 2023

rahul-kothari May 4, 2023 •

edited

Loading

spalladino May 4, 2023

spalladino May 3, 2023

PhilWindle May 3, 2023

spalladino May 3, 2023

LHerskind left a comment

spalladino left a comment

rahul-kothari commented May 4, 2023

Public kernel cbind errors #437

Public kernel cbind errors #437

Conversation

rahul-kothari commented May 3, 2023 • edited Loading

Description

Checklist:

Maddiaa0 left a comment

Choose a reason for hiding this comment

PhilWindle left a comment

Choose a reason for hiding this comment

dbanks12 left a comment

Choose a reason for hiding this comment

spalladino May 3, 2023 • edited Loading

Choose a reason for hiding this comment

PhilWindle May 3, 2023

Choose a reason for hiding this comment

rahul-kothari May 4, 2023 • edited Loading

Choose a reason for hiding this comment

spalladino May 4, 2023

Choose a reason for hiding this comment

spalladino May 3, 2023

Choose a reason for hiding this comment

PhilWindle May 3, 2023

Choose a reason for hiding this comment

spalladino May 3, 2023

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

spalladino left a comment

Choose a reason for hiding this comment

rahul-kothari commented May 4, 2023

rahul-kothari commented May 3, 2023 •

edited

Loading

spalladino May 3, 2023 •

edited

Loading

rahul-kothari May 4, 2023 •

edited

Loading