fix(sync): Sync latest globals within merkle tree ops #1612
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ludamad
reviewed
Aug 17, 2023
| @@ -348,7 +346,7 @@ export class AztecNodeService implements AztecNode { | |||
| await getTreeRoot(MerkleTreeId.BLOCKS_TREE), | |||
| Fr.ZERO, | |||
| await getTreeRoot(MerkleTreeId.PUBLIC_DATA_TREE), | |||
| globalsHash, | |||
| await this.merkleTreeDB.getLatestGlobalVariablesHash(), | |||
There was a problem hiding this comment.
hmm is there any chance this would advance such that the global variable hash is too new? Like if the system moves on after await getTreeRoot(MerkleTreeId.PUBLIC_DATA_TREE),
There was a problem hiding this comment.
I don't fully understand the code, but if we're fetching data it would be good to fetch it all at once representing one snapshot, 'latest' makes me nervous when used with other async calls
There was a problem hiding this comment.
These changes have unfortunately broken the p2p tests, this could be the culprit
ludamad
approved these changes
Aug 17, 2023
|
Revoking acceptance given the comment |
|
Only nit is a future comment would be nice |
Where should I place the comment and what should I highlight, ill incorporate it into a coming pr |
|
Would be nice to comment the promise all |
codygunton
pushed a commit
that referenced
this pull request
Jan 23, 2024
* dynamic reference to redis cluster subnet group (#1594) Co-authored-by: PhilWindle <philip.windle@gmail.com> * Created generic swapping agent for both uniswap and lido-curve bridges (#1265) * JB/Website changes (#1554) * Bruno changes * Grants changes * Update grants page, edit team page * Fix typos, change header, footer links; noir links * Fix sliders * Copy changes * Adds two RFPs * Add Michael to Careers, typos * Fix titles of roles * JB/Website changes (#1612) * Bruno changes * Grants changes * Update grants page, edit team page * Fix typos, change header, footer links; noir links * Fix sliders * Copy changes * Adds two RFPs * Add Michael to Careers, typos * Fix titles of roles * Fix lint, replace images * Pass the uint context to the return value of uint::at (#1593) * Construct return value from current context in uint::at Fixes AztecProtocol/aztec2-internal#1433 * Add uint test which asserts context inheritance * Move to yarn 3 plug 'n play and workspaces, pure ESM modules. (#1599) * Transferrables and webpack to build a worker.js until we have loader chaining... * Prod/dev conf * Update bb imports. * bb tests passing. * Integrate wasm back in bb.js. Passing tests. Reduce wasm memory footprint. * Can run halloumi and falafel. * Falafel tests pass. * Sdk tests pass. * e2e.test passes. * webpack * bb.js yarn 3.2.2 * fix mem align * fix bitwise warning on newer clang * Tweaks towards wasi 16 (more to figure out) * yarn 3.2.2 * remove encoding headers from block source client * bb.js updates * Blockchain test transpile to cjs for hardhat. * Fix blockchain. * SDK alternates proving keys. * Falafel/halloumi start_e2e scripts args. * Shared linting, formatting for bb.js and blockchain. * explorer import changes. build needs fixing. * end-to-end, falafel, halloumi linting * Hummus just a terminal. Linting. Version upgrades. * Linting. Bootstrapping. Wallet. * prettier config. * Basic working wallet. * Fix apollo server stuff in falafel... * account-migrator. explorer. * Cleanup JSON stuff. * wasabi. * Revert jest->mocha * Merge fixes. * Initial zk-money. * - Reconnect bootstrap scripts - set prettier as js default * - es-module style imports - upgrade typescript - copy wasm * zk-money builds. * Linting * Dockerfile and build stuff. * Move yarn project into own folder. * explorer build. min cci yml. * Introduce a build manifest to compute rebuild patterns. * Addition of a script for auto launching tmux and running a test. * Streamline bootstrapping. * Add bash to falafel image. * build image * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * Attempt to fix deploy. * When scanning for rollups, we only cache the last block number where … (#1611) * When scanning for rollups, we only cache the last block number where we actually found a rollup * Fixed bug * fix typo in zk-money app obs intial -> initial (#1633) * Adress comments about docs (#1500) * Adress comments about docs * Update schnorr.md Co-authored-by: Michael Connor <mike@aztecprotocol.com> * Move genesis data from bb.js to falafel (#1640) * moved init/genesis data from bb.js to falafel along with helper functions for reading the init data. * data files no longer need to be moved as part of bb.js webpack * obsolete comment in bb.js env * Moved some init helper functions back to bb.js. Renamed helper class in falafel. Fixed missing 'any' type in falafel env init.ts * copy init account data to dest in falafel Co-authored-by: spypsy <spypsy@users.noreply.github.com> Co-authored-by: Jonathan Bursztyn <jobur93@gmail.com> Co-authored-by: Guido Vranken <guidovranken@users.noreply.github.com> Co-authored-by: Charlie Lye <charlie@aztecprotocol.com> Co-authored-by: Charlie Lye <karl.lye@gmail.com> Co-authored-by: David Banks <47112877+dbanks12@users.noreply.github.com> Co-authored-by: Adrian Hamelink <adrian.hamelink@gmail.com> Co-authored-by: Michael Connor <mike@aztecprotocol.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Ci has been failing due to a race condition where world state operations were not being synchronised.
The offending value was that the world latestGlobalVariablesHash was not stored with the rest of the merkle trees, and such could not use the same serial queue to sync their update operations. This lead to the trees being out of sync with the globals value on some occasions. (I think)
This pr may be slightly contraversial as it stores non merkle tree data within the merkle_trees abstraction. However think the naming of the merkle_trees abstraction is the limiting factor. The file just stores and synchronises the world state, it just so happens that until now this state was all trees. (although this could be me bending reality to suit my current needs)
Adds a Committable type which allows us to add rollback functionality to anything