-
Notifications
You must be signed in to change notification settings - Fork 284
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
feat: Minimal globals in constant rollup data #882
Conversation
bc258c9
to
c277d1f
Compare
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.
- In
solo_block_builder.buildL2Block()
, we are now passing timestamp, but we are never setting it anywhere (for example insequencer.buildBlock()
which calls thebuildL2Block()
? - When we upgrade, it seems like there would be 2 places where we need to update
chainID
andversion
now? Insequencer-client.ts
which instantiates the soloblock builder and the AztecRPCServer. Can we move these to configs instead? Maybe we don't want to have a default param for these values so that we are sure we are passing these correctly everywhere (will be helpful for upgrades). - The archiver processes L2 block logs and increments
expectedNextL2BlockNum
. Should there be a check to ensure this matches the l2 block it decoded? @spalladino
using boolean = typename NCT::boolean; | ||
|
||
fr chain_id = 0; | ||
fr version = 0; |
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.
can we rename version
to aztecVersion
and chainId
to baseChainID
to make it more clear what these are. Otherwise, especially for noir devs this will be super super confusing
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.
Technically they are both chainIds, so could also be baseChainId
and rollupChainId
🤷. Version mainly chosen just to be different to chainId
to make it clear it was not the same thing.
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.
yea I am fine with baseChainId and rollupChainId!
@@ -213,7 +213,7 @@ export class AztecRPCServer implements AztecRPCClient { | |||
contractAddressSalt, | |||
portalContract, | |||
); | |||
const txContext = new TxContext(false, false, true, contractDeploymentData); | |||
const txContext = new TxContext(false, false, true, contractDeploymentData, Fr.ZERO, Fr.ZERO); |
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.
is this from where the kernel circuit will add these values into its public inputs (I know we add it to base circuit's constants in the solo_block_builder.ts
file)? If so, the chainID and version should be read from somewhere (like a config value), rather than us hard-coding no? Otherwise for every upgrade we might have to edit it here?
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.
See the general comment; #882 (comment).
Timestamps will be constrained based on L1 contract in #830. As this is expected to break a lot of tests and such, I delayed that task to when we are actually enforcing it, which will be a separte PR.
Similar to above, planned to address this when the values starting being constrained on L1 with #827 and #831 |
c277d1f
to
16b52b3
Compare
Description
Replaces #861 to address #826. Extends
ConstantRollupData
withchainid
fromtx_request
intotx_context
version
intotx_context
global_variables
(chain_id
,version
) toconstants
global_variables
to matchchainid
andversion
to kernels in the base rollup.block_number
andtimestamp
toglobal_variables
global_variables
topublic
input hash in the root rollup, include it in L2 blockChecklist: