fix(synchroniser): Store most recent globals hash in the synchroniser, rather than fetching from the latest block #1539
Conversation
Maddiaa0
changed the title
| return Promise.resolve(); | ||
| } | ||
|
|
||
| // TODO / Reviewers note: shall i remove the tree roots and globals hash entirely and just have this? Its all the sim needs. |
There was a problem hiding this comment.
drawing attention to this comment!
There was a problem hiding this comment.
Both setters and getters? If we are not using the functions standalone in any cases anymore I think it would be fine to remove it. I don't see many cases where you would only want the globalVariablesHash neither, so think the overhead over getting the tree roots directly is fine.
|
Is it feasible to make a test that tracks this if it regresses? |
| * | ||
| * @returns A Promise that resolves to a ConstantHistoricBlockData object. | ||
| */ | ||
| getConstantHistoricBlockData(): Promise<ConstantHistoricBlockData> { |
There was a problem hiding this comment.
Could you not just use db.getHistoricBlockData directly here instead of reimplementing? Also, the naming is not fully consistent if including constant or not.
There was a problem hiding this comment.
correct this is clutter from before i implemented db.
| @@ -47,16 +34,11 @@ export async function deployAndInitializeNonNativeL2TokenContracts( | |||
| rollupRegistryAddress: EthAddress, | |||
| initialBalance = 0n, | |||
| owner = AztecAddress.ZERO, | |||
| underlyingERC20Address?: EthAddress | |||
| underlyingERC20Address?: EthAddress, | |||
There was a problem hiding this comment.
Were formatting not run here before or skipped now? Seems like quite a few formatting changes.
There was a problem hiding this comment.
Were formatting not run here before or skipped now? Seems like quite a few formatting changes.
Ah looks like formatting isnt enforced for it, but i ran the formatter for the whole repo. Ill probably turn the formatter on in another pr
I dont think it is without creating a test that has sufficiently many blocks that the node sync starts to drag behind the rest of the test. However if any one has other ideas ill take it on |
Probably going to drop the constant from it, its within the constants object in circuits, its implied that it is constant |
Sounds like a fuzzing sort of task + therefore out of scope. Something that kicks off nightly and runs for some time X (increasing period of time as we add features) would be cool though. Made #1545 |
fixes: #1537
Stores the most recent global variables hash within the synchroniser rather than fetching it from the most recent block. This created a race condition where the globals hash requested from the block was ahead of the tree roots requested from the node.
As the tree roots were being requested from the node the synchroniser was also behind.