Conversation
| } | ||
|
|
||
| struct InterimProposeValues { | ||
| ProposedHeader header; |
There was a problem hiding this comment.
Isn't moving the entire header into memory costly in terms of gas?
There was a problem hiding this comment.
Ye, potentially. We are running into stack too deep without it. And if moving the compressed tips into it instead, the gas actually goes up, so I think we are benefitting a bit from some storage referencing since we are not updating it 😅
|
|
||
| ejectionThreshold: 100_000n * 10n ** 18n, | ||
| activationThreshold: 200_000n * 10n ** 18n, | ||
| localEjectionThreshold: 196n * 10n ** 18n, |
There was a problem hiding this comment.
Is this correct? Shouldn't it be 196_000n?
| @memoize | ||
| getLocalEjectionThreshold() { | ||
| return this.rollup.read.getLocalEjectionThreshold(); | ||
| } |
There was a problem hiding this comment.
We shouldn't memoize this value if it's updateable
There was a problem hiding this comment.
Think we are already do so with a few values that can be updated, but are only expected to be so rarely, can fix but will address a few more then as well:
- getProvingCostPerMana
- getManaTarget
- getManaLimit
- getLocalEjectionThreshold
There was a problem hiding this comment.
Or I guess at least for ignition the first 3 won't really change at all. So it is probably fine to let them sit
| bootstrapFlushSize: 32n, // will effectively be bounded by maxQueueFlushSize | ||
| normalFlushSizeMin: 32n, | ||
| bootstrapFlushSize: 48n, | ||
| normalFlushSizeMin: 1n, |
There was a problem hiding this comment.
Do we want to set the flush size to 48 as in StagingIgnition in #17364? Not saying we should, just asking.
There was a problem hiding this comment.
Do you mean the normal flush size min or the bootstrap flush size? Bootstrap is already 48 here and that matches #17364, so I would think that is fine?
| } | ||
|
|
||
| // Note that we are flushing at most `chunkSize` at each call | ||
| await deployer.l1TxUtils.sendAndMonitorTransaction( |
There was a problem hiding this comment.
Nit: this would faster if we didn't wait for each tx to be mined before sending the next one. But it's just an optimization, and I don't think it's really an issue, is it?
There was a problem hiding this comment.
Don't think its an issue, at least the speedup still much faster than before, but ye should be room to go faster.
| ? await this.validatorClient.signAttestationsAndSigners( | ||
| attestationsAndSigners, | ||
| proposerAddress ?? publisher.getSenderAddress(), | ||
| ) |
There was a problem hiding this comment.
I'm not sure if the fallback on the sender address is correct. Shouldn't we just use an empty signature if there is no proposer defined for this iteration?
There was a problem hiding this comment.
Should be able to just give it an empty ye.
| toString() { | ||
| throw new Error('Not implemented'); | ||
| } |
There was a problem hiding this comment.
Aren't we risking breaking a logging or serialization by throwing on toString?
cf70a62 to
c257d92
Compare
just-mitch
left a comment
There was a problem hiding this comment.
governance changes look good 👍
Foundry has been screaming at me whenever I bootstrap the l1-contracts with what looks like a lot of low-value lints. I've shied away from making any code changes as I don't think it would be particularly helpful so this PR mostly boils down to silencing lints coming from dependencies and disabling lints which just are just code style-guide items. One semi-meaningful thing which I've disabled is a lint calling out an opportunities to optimize keccak hashing so I've opened an issue to track this here: #16808 `forge lint` now outputs the below. This looks like we can add `/// forge-lint: disable-next-item(erc20-unchecked-transfer)` to these cases as we control the token being used in these situations - but I'll leave that up to the tmnts. ``` warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/core/libraries/rollup/RewardLib.sol:90:5 | 90 | rollupStore.config.feeAsset.transfer(_sequencer, amount); | -------------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/core/libraries/rollup/StakingLib.sol:184:5 | 184 | store.stakingAsset.transfer(exit.recipientOrWithdrawer, exit.amount); | -------------------------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/core/libraries/rollup/StakingLib.sol:285:5 | 285 | store.stakingAsset.transferFrom(msg.sender, address(this), amount); | ------------------------------------------------------------------ | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/core/libraries/rollup/StakingLib.sol:357:9 | 357 | store.stakingAsset.transfer(args.withdrawer, amount); | ---------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/core/libraries/rollup/RewardLib.sol:122:5 | 122 | rollupStore.config.feeAsset.transfer(_prover, accumulatedRewards); | ----------------------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/governance/GSE.sol:339:5 | 339 | ASSET.transferFrom(msg.sender, address(this), ACTIVATION_THRESHOLD); | ------------------------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/governance/GSE.sol:472:5 | 472 | ASSET.transferFrom(msg.sender, address(this), amount); | ----------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/src/core/libraries/rollup/RewardLib.sol:218:9 | 218 | rollupStore.config.feeAsset.transfer(BURN_ADDRESS, t.totalBurn); | --------------------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value --> /mnt/user-data/tom/aztec-packages/l1-contracts/test/portals/TokenPortal.sol:149:5 | 149 | underlying.transfer(_recipient, _amount); | ---------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer ```
Allows the vetoer to disable slashing altogether temporarily. This allows the vetoer to stop all slashes if a bug is detected that continuously triggers slashes that are not valid, so the vetoer does not need to continuously vetoed every individual slash until the issue is solved.
Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. For audit-related pull requests, please use the [audit PR template](?expand=1&template=audit.md).
Updated the `hashToPoint` function. Accompanying update is made in AztecProtocol/explorations#8
58c781f to
fc8b532
Compare
49267ab to
ee1c6d2
Compare
# v2.0.3..v2.1.0-rc.1 Notes ## Significant L1 Changes ### 1. **Rollup Contract Interface Changes** - **`propose()` function signature changed**: Now requires an additional `_attestationsAndSignersSignature` parameter - **`validateHeaderWithAttestations()` function signature changed**: Also requires the new signature parameter - This affects any code that directly calls these functions on the rollup contract ### 2. **New Required Configuration Parameters** Several new configuration parameters are now required for deployment: - `localEjectionThreshold`: Stricter ejection threshold local to specific rollup (default: 196,000 tokens) - `slashingDisableDuration`: How long slashing can be disabled in seconds (default: 5 days) ### 3. **GSE Contract Changes** - **New function**: `setProofOfPossessionGasLimit()` \- allows governance to adjust gas limits for BLS proof validation - **Gas-limited proof validation**: Proof of possession validation now has configurable gas limits (default: 200,000 gas) ### 4. **Validator Queue Management Changes** - **`flushEntryQueue()` behavior changed**: Now has an overload accepting a `_toAdd` parameter to limit validator additions - **New validator flush accounting**: System now tracks available validator flushes per epoch Significant Non-Breaking Changes -------------------------------- ### 1. **Enhanced Slashing Controls** - **Temporary slashing disable**: Vetoers can now temporarily disable slashing for the configured duration - **New function**: `setSlashingEnabled(bool)` for controlling slashing state ### 2. **Improved Validator Selection** - **Configurable lag period**: Validator sampling now uses configurable epoch lag instead of fixed 2-epoch delay - **Better bootstrapping**: Enhanced validator set bootstrapping with improved flush size calculations ### 3. **Updated Default Values** - **Coin issuer rate**: Updated to `25,000,000,000 tokens / year` (approximately 793 tokens per second) - **Local ejection threshold**: Set to 196,000 tokens (stricter than global 50,000 threshold) ## Significant Node Changes ### Fixes - Rollback world state on failed block sync – Prevents bad state persistence by rolling back uncommitted data if block sync fails. [(#17158)](github.com//pull/17158) - Early rejection of duplicate nullifiers – Detects and rejects transactions with duplicate nullifiers before inclusion. [(#17157)](github.com//pull/17157) - Watcher pruning fix – Watcher now re-executes only blocks from the relevant pruned epoch, avoiding cross-epoch slashing issues. [(#17145)](github.com//pull/17145) - Improved proposal validation – Fully validates proposal headers (including archive root derivation) and blocks attempts to reuse existing block numbers. [(#17144)](github.com//pull/17144) - L1 to L2 message sync reliability – Waits for rollup to reach the inbox block before marking L1→L2 messages as synced; adds helpers to track message readiness. [(#17132)](github.com//pull/17132) - Slashing round recovery – Executes pending slashing rounds skipped during the first executable round; adds slashExecuteRoundsLookBack to control re-check depth. [(#17125)](github.com//pull/17125) - Broker restart on rollup change – Ensures broker restarts when rollup chain changes to stay synchronized. [(#17120)](github.com//pull/17120) - Remote signer readiness check – Verifies that a remote signer is available before use. [(#17119)](github.com//pull/17119) - Orchestrator and agent retry improvements – Makes connections to the broker more robust under transient failures. [(#17117)](github.com//pull/17117) - Telemetry cleanup – Fixes incorrect or spammy telemetry warnings. [(#17155)](github.com//pull/17155) ### Features - Network configuration support – Introduces centralized configuration for network parameters. [(#17113)](github.com//pull/17113) ## Full Changelog You can generate this yourself with `./scripts/commits v2.0.3..v2.1.0-rc.1 1000 -m -g`. #### Fixes - fix: use archiveAt(0) instead of getBlock to get genesis archive tree - backport v2 ([#17447](#17447)) — spypsy, 5 days ago - fix: add keystoreDirectory option to sequencer ([#17265](#17265)) — spypsy, 13 days ago - fix: testnet archival node - v2 ([#17142](#17142)) — Aztec Bot, 3 weeks ago #### Chores - chore: bump minor version — Mitch, 4 days ago — [dbc243f](dbc243f) - chore: backport dependabot deps ([#17463](#17463)) — Aztec Bot, 5 days ago - chore: Backport slack alerts ([#17460](#17460)) — PhilWindle, 5 days ago - chore(backport-to-v2): chore: New salt for staging-ignition (#17453) ([#17453](#17453)) — Aztec Bot, 5 days ago - chore(backport-to-v2): fix: improve libp2p connection limits for network discovery (#17425) ([#17425](#17425)) — Aztec Bot, 5 days ago - chore(backport-to-v2): feat: add flushing rewarder (#17335) ([#17335](#17335)) — Aztec Bot, 6 days ago - chore(backport-to-v2): feat: add date gated relayer (#17323) ([#17323](#17323)) — Aztec Bot, 6 days ago - chore(backport-to-v2): feat: support using existing ERC20 token for fee and staking (#17413) ([#17413](#17413)) — Aztec Bot, 6 days ago - chore: Delete contract addresses from chain l2 config ([#17430](#17430)) — PhilWindle, 6 days ago - chore: More updated staging public config ([#17364](#17364)) — PhilWindle, 7 days ago - chore(backport-to-V2): L1 backports ([#17365](#17365)) — Lasse Herskind, 7 days ago - chore: Ensure DB map sizes are configured for networks ([#17383](#17383)) — PhilWindle, 7 days ago - chore: Backport of fixes into v2 ([#17206](#17206)) — PhilWindle, 8 days ago - chore: update zkpassport version ([#17339](#17339)) — saleel, 8 days ago - chore: Backport of workflow fix ([#17333](#17333)) — PhilWindle, 11 days ago - chore: Streamline staging deployments ([#17328](#17328)) — PhilWindle, 11 days ago - chore(backport-to-v2): fix: avm gracefully handles shifts (shl) with huge bit sizes (#17171) ([#17171](#17171)) — Aztec Bot, 12 days ago - chore(backport-to-v2): chore: remove unconstrained generics from trait impls (#17075) ([#17075](#17075)) — Aztec Bot, 12 days ago - chore: Backport deployment refactor ([#17280](#17280)) — PhilWindle, 12 days ago - chore(backport-to-v2): fix(docs): Update Counter contract tutorial imports and remove unnecessary sections (#17241) ([#17241](#17241)) — Aztec Bot, 13 days ago - chore: remove ACCEPT_DISABLED_AVM_VK_TREE_ROOT ([#17238](#17238)) — Alex Gherghisan, 13 days ago - chore: remove bad rollup-version default ([#17223](#17223)) — Alex Gherghisan, 2 weeks ago - chore(docs): node docs to v2 ([#17205](#17205)) — esau, 2 weeks ago - chore(backport-to-v2): chore(avm)!: Fix a misleading log in recursive verifier related to public input (#17184) ([#17184](#17184)) — Aztec Bot, 2 weeks ago - chore: Backport of ignition fix attempt 2 ([#17201](#17201)) — PhilWindle, 2 weeks ago - chore: turn on testnet compat test ([#17195](#17195)) — Alex Gherghisan, 2 weeks ago - chore: Backport fix to staging-ignition to v2 ([#17159](#17159)) — PhilWindle, 3 weeks ago - chore: kubectl ([#17140](#17140)) — Alex Gherghisan, 3 weeks ago #### Other - backport dependabots p2 ([#17488](#17488)) — mralj, 4 days ago --------- Co-authored-by: AztecBot <tech@aztecprotocol.com>

Plenty contract PR's have been implemented since the cut of V2. I'm taking a look into them to figure out if we need all of them. And can make a potential monster cherry pick.
I'll be cherry picking these with a few exceptions, namely:
as these are touching so so many files. a3e23b9 deals with the checkpoint building, which we don't really need for ignition. And the other is just spelling in the comments. And to be frank, I'm not feeling all that great about doing a 125 file commit backport just for spelling in the comments