fix: various low-impact improvements to core/ contracts#16268
Conversation
benesjan
force-pushed
the
08-08-fix_minor_touches_to_core_contracts
branch
2 times, most recently
from
5570936 to
625f7f7
Compare
benesjan
requested review from
LHerskind,
Maddiaa0 and
just-mitch
as code owners
benesjan
force-pushed
the
08-08-fix_minor_touches_to_core_contracts
branch
2 times, most recently
from
03bf712 to
e6af424
Compare
✅ Deploy Preview for aztec-docs-temp-you-can-delete canceled.
|
benesjan
force-pushed
the
08-08-fix_minor_touches_to_core_contracts
branch
from
94f1931 to
747d868
Compare
| * - Each slot has one designated proposer from the validator set | ||
| * - Each block is expected to include attestations from committee members | ||
| * - Committees remain stable throughout an epoch | ||
| * - There is one committee per epoch |
There was a problem hiding this comment.
Was a bit confused by the meaning of stable here. Think this is clearer.
|
|
||
| /** | ||
| * @notice Sets up validator selection for the current epoch | ||
| * @dev Can be called by anyone at the start of an epoch. Samples the committee |
There was a problem hiding this comment.
Just added more context here and aligned to 120 chars line width.
| * generation impractical. | ||
| */ | ||
| function setupEpoch() public override(IValidatorSelectionCore) { | ||
| function setupEpoch() external override(IValidatorSelectionCore) { |
There was a problem hiding this comment.
This was never called within the contract.
| // duplicate cases, so `sampleIndices` is a superset of the `sampleIndex` values that have been drawn | ||
| // (to account for duplicates). Therefore, clearing `sampleIndices` clears everything. | ||
| // Due to the cost of `tstore` and `tload` operations, it is cheaper to overwrite all values | ||
| // rather than checking if there is anything to override. |
There was a problem hiding this comment.
Just fixed english here.
| // Committee members that have attested will produce a signature, and if they have not attested, the signature will be | ||
| // empty and | ||
| // an address provided. | ||
| // empty and an address provided. |
| * to recover them from their attestations' signatures (and hence save gas). The addresses of the non-signing | ||
| * committee members are directly included in the attestations. | ||
| * @param _length - The number of addresses to return, should match the number of committee members | ||
| * @return The addresses of the committee members. |
There was a problem hiding this comment.
Got confused here why do we pass in the signers + this:
"Indices with signatures will have a zero address."
was a lie so updated the docs here.
| using AttestationLib for CommitteeAttestations; | ||
| using CompressedTimeMath for CompressedSlot; | ||
|
|
||
| // This is a temporary struct to avoid stack too deep errors |
There was a problem hiding this comment.
This was no longer used.
| ) internal view returns (bytes32[] memory) { | ||
| RollupStore storage rollupStore = STFLib.getStorage(); | ||
|
|
||
| // TODO(#7373): Public inputs are not fully verified |
There was a problem hiding this comment.
Issue already closed.
|
|
||
| // Verify attestations for the last block in the epoch | ||
| // -> This serves as training wheels for the public part of the system (proving systems used in public and AVM) | ||
| // ensuring committee consensus on the epoch's validity alongside the cryptographic proof verification below. |
There was a problem hiding this comment.
Just added more context here as it led me to look around for what is the meaning of the training wheels. I know it's described in the docs of RollupCore but it's hard to keep all these docs in mind when not knowing the code well.
| * roundaboutSize). | ||
| * This reuses storage slots for old blocks that have been proven or pruned. | ||
| * The roundabout size is calculated as maxPrunableBlocks() + 1 to ensure at least the last provable block | ||
| * The roundabout size is calculated as maxPrunableBlocks() + 1 to ensure at least the last proven block |
There was a problem hiding this comment.
Small difference in the word but a big difference in meaning.
| * Uses transient storage caching to avoid recomputation within the same transaction. | ||
| * Uses transient storage caching to avoid recomputation within the same transaction. (This caching mechanism is | ||
| * commonly used when a proposer signals in governance and submits a proposal within the same transaction - then | ||
| * `getProposerAt` function is called). |
There was a problem hiding this comment.
Lasse mentioned here that the getProposer function is called when signalling.
I assume Lasse meant getProposerAt.
I actually failed to find an example where we have 1 tx signalling and proposing (will this be done via multicall?) so please correct me here if getProposerAt is actually not the function.
There was a problem hiding this comment.
Yes it is done by multicall, and yes it is getProposerAt 👍
| // If the target committee size is 0, we skip the validation | ||
| // If the rollup is *deployed* with a target committee size of 0, we skip the validation. | ||
| // Note: This generally only happens in test setups; In production, the target committee is non-zero, | ||
| // and one can see in `sampleValidators` that we will revert if the target committee size is not met. |
There was a problem hiding this comment.
Copied this comment from below in the file as this was quite confusing before.
| RollupStore storage rollupStore = STFLib.getStorage(); | ||
|
|
||
| // Pending chain tip | ||
| uint256 pendingBlockNumber = STFLib.getEffectivePendingBlockNumber(_ts); |
There was a problem hiding this comment.
Was not immediately clear to me that it's the latest block of the pending chain so added a comment here.
| * @dev Uses keccak256 hash of epoch, slot, and seed to deterministically select a committee member. | ||
| * The result is modulo committee size to ensure valid index. | ||
| * The result being modulo biased is not a problem here as the validators in the committee were chosen randomly | ||
| * and are not ordered. |
There was a problem hiding this comment.
Added a note here as it was not immediately clear to me that it's not a problem since I was not intimate yet with how the sampling works.
benesjan
force-pushed
the
08-08-fix_minor_touches_to_core_contracts
branch
from
747d868 to
f2601f2
Compare
benesjan
changed the title
core/ contracts
just-mitch
left a comment
just-mitch
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for having such a thorough look @benesjan .
just-mitch
force-pushed
the
08-08-fix_minor_touches_to_core_contracts
branch
from
f2601f2 to
037b242
Compare
benesjan
force-pushed
the
08-08-fix_minor_touches_to_core_contracts
branch
from
037b242 to
ea7a316
Compare
2 participants
When working on my internal review of internal contracts I stumbled upon a few minor issues that are not worth describing in my review doc since they are relatively low impact (e.g. typos, formatting, docs etc.) so to save everyone's time I just submitted this PR.
If some change I did is misleading please just go ahead and correct it.