-
Notifications
You must be signed in to change notification settings - Fork 234
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
chore: Debug log oracle calls return nothing #6209
Conversation
return [result]; | ||
return Array.isArray(result) && result.length === 0 ? [] : [result]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change, ACVM complained with Assertion failed: 1 output values were provided as a foreign call result for 0 destination slots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm seems like this breaks other calls:
+e2e-tests *failed* | Simulation error: Assertion failed: 0 output values were provided as a foreign call result for 1 destination slots 'call_public_function_oracle(
+e2e-tests *failed* | contract_address,
+e2e-tests *failed* | function_selector,
+e2e-tests *failed* | args_hash,
+e2e-tests *failed* | side_effect_counter,
+e2e-tests *failed* | is_static_call,
+e2e-tests *failed* | is_delegate_call
+e2e-tests *failed* | )'
Any tips on how to fix? I don't have a firm grasp on (ie I have no idea) how ACVM expects the callback return values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that our oracle resolver doesn't handle very well returning more/less than 1 result (that result being a value or an array).
ACVM oracles expect returns to be an array of ValueOrArray. So for example if you are returning an array of fields, the ACVM expects [array]. If you are returning a struct with a field and an array, it'd be [field, array]. I think our typed oracle stuff assumes always returning one item, it being a field or an array.
It needs some updating!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our typed oracle stuff assumes always returning one item, it being a field or an array.
Got it, so the issue is exclusively on the side of aztec-packages then, right? I can give it a shot.
After noir-lang/noir#4959 lands in aztec-packages, we should be able to keep debug_log calls in protocol-circuits without breaking nargo test there. A prerequisite for that is ensuring that debug log calls do not return anything. I understand they were returning zero because of legacy issues, but according to @TomFrench that should not be needed. This PR updates debug calls so they return no values.
7edeaf2
to
cf815ad
Compare
@sirasistant the issue is now fixed and CI is green. Could I ask for a review..? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks! 🥳
After noir-lang/noir#4959 lands in aztec-packages, we should be able to keep debug_log calls in protocol-circuits without breaking nargo test there. A prerequisite for that is ensuring that debug log calls do not return anything. I understand they were returning zero because of legacy issues, but according to @TomFrench that should not be needed.
This PR updates debug calls so they return no values.