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

fix: use synched block number to sync notes #10545

Closed
wants to merge 18 commits into from
Closed

Conversation

Thunkar
Copy link
Contributor

@Thunkar Thunkar commented Dec 9, 2024

Closes: #10296

Testing out a theory: simulation was using more notes than were available to the kernels (aztecNode.getBlockNumber vs. synchronizer.getSynchedBlockNumber). That might have led to a situation in which the kernel fails even if simulation succeeds (as @spalladino found in the linked issue)

@Thunkar Thunkar requested review from ludamad and spalladino December 9, 2024 16:42
@Thunkar Thunkar self-assigned this Dec 9, 2024
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

I'm not sure about this as a fix. Given it's a Circuit execution failed we're seeing, I'd expect the fix to be part of the pxe itself, rather than having the caller wait for the sync to finish.

If we want to throw this in as a temporary measure so we can reenable the 2_pxes tests, then I'm fine, but if we do I'd open an issue to investigate the Circuit execution failed.

@@ -37,6 +37,17 @@ export async function expectTokenBalance(
expectedBalance: bigint,
logger: DebugLogger,
) {
// Check if the wallet
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the wallet... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, didn't save the file properly.

To address the overall comment, the Circuit execution failed error seems to happen when validating a read request in the kernel, so I assumed it's because we haven't synched the balances properly (kernel verification happens with a stale header). Even if PXE

You made me think, hope you're happy. The problem is in a misalignment between what's read in the simulation (that uses notes seen up to aztecNode.getBlockNumber()) and the kernels (that checks read requests up to synchronizer.getSynchedBlockNumber). I'm gonna test this theory by forcing simulation to use only the synched blockNumber!

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Sorry, I had missed the description of the PR. Let's merge this then, but not mark #10296 as fixed (or open a new one instead).

@Thunkar Thunkar changed the title bug: wait for pxe to synch before checking balances fix: use synched block number to sync notes Dec 9, 2024
@Thunkar Thunkar added e2e-all CI: Enables this CI job. and removed e2e-2-pxes labels Dec 9, 2024
@Thunkar
Copy link
Contributor Author

Thunkar commented Dec 11, 2024

Closing in favor of: #10613

@Thunkar Thunkar closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix e2e_2_pxes
2 participants