diff --git a/l1-contracts/src/core/libraries/rollup/AttestationLib.sol b/l1-contracts/src/core/libraries/rollup/AttestationLib.sol index 85cc32e6b9cd..7c07b3b878fc 100644 --- a/l1-contracts/src/core/libraries/rollup/AttestationLib.sol +++ b/l1-contracts/src/core/libraries/rollup/AttestationLib.sol @@ -129,49 +129,6 @@ library AttestationLib { return addr; } - /** - * @notice Assert that the size of `_attestations` is as expected, throw otherwise - * - * @custom:reverts SignatureIndicesSizeMismatch if the signature indices have a wrong size - * @custom:reverts SignaturesOrAddressesSizeMismatch if the signatures or addresses object has wrong size - * - * @param _attestations - The attestation struct - * @param _expectedCount - The expected size of the validator set - */ - function assertSizes(CommitteeAttestations memory _attestations, uint256 _expectedCount) internal pure { - // Count signatures (1s) and addresses (0s) from bitmap - uint256 signatureCount = 0; - uint256 addressCount = 0; - uint256 bitmapBytes = (_expectedCount + 7) / 8; // Round up to nearest byte - require( - bitmapBytes == _attestations.signatureIndices.length, - Errors.AttestationLib__SignatureIndicesSizeMismatch(bitmapBytes, _attestations.signatureIndices.length) - ); - - for (uint256 i = 0; i < _expectedCount; i++) { - uint256 byteIndex = i / 8; - uint256 bitIndex = 7 - (i % 8); - uint8 bitMask = uint8(1 << bitIndex); - - if (uint8(_attestations.signatureIndices[byteIndex]) & bitMask != 0) { - signatureCount++; - } else { - addressCount++; - } - } - - // Calculate expected size - uint256 sizeOfSignaturesAndAddresses = (signatureCount * SIGNATURE_LENGTH) + (addressCount * ADDRESS_LENGTH); - - // Validate actual size matches expected - require( - sizeOfSignaturesAndAddresses == _attestations.signaturesOrAddresses.length, - Errors.AttestationLib__SignaturesOrAddressesSizeMismatch( - sizeOfSignaturesAndAddresses, _attestations.signaturesOrAddresses.length - ) - ); - } - /** * Recovers the committee from the addresses in the attestations and signers. * diff --git a/l1-contracts/src/core/libraries/rollup/ProposeLib.sol b/l1-contracts/src/core/libraries/rollup/ProposeLib.sol index aa31bb445564..630a2fd4a891 100644 --- a/l1-contracts/src/core/libraries/rollup/ProposeLib.sol +++ b/l1-contracts/src/core/libraries/rollup/ProposeLib.sol @@ -237,7 +237,13 @@ library ProposeLib { { // Verify that the proposer is the correct one for this slot by checking their signature in the attestations ValidatorSelectionLib.verifyProposer( - v.header.slotNumber, v.currentEpoch, _attestations, _signers, v.payloadDigest, _attestationsAndSignersSignature + v.header.slotNumber, + v.currentEpoch, + _attestations, + _signers, + v.payloadDigest, + _attestationsAndSignersSignature, + true ); } diff --git a/l1-contracts/src/core/libraries/rollup/RollupOperationsExtLib.sol b/l1-contracts/src/core/libraries/rollup/RollupOperationsExtLib.sol index 2e3ee1152756..69614e43bf02 100644 --- a/l1-contracts/src/core/libraries/rollup/RollupOperationsExtLib.sol +++ b/l1-contracts/src/core/libraries/rollup/RollupOperationsExtLib.sol @@ -53,7 +53,7 @@ library RollupOperationsExtLib { Epoch epoch = slot.epochFromSlot(); ValidatorSelectionLib.verifyAttestations(slot, epoch, _attestations, _args.digest); ValidatorSelectionLib.verifyProposer( - slot, epoch, _attestations, _signers, _args.digest, _attestationsAndSignersSignature + slot, epoch, _attestations, _signers, _args.digest, _attestationsAndSignersSignature, false ); } diff --git a/l1-contracts/src/core/libraries/rollup/ValidatorSelectionLib.sol b/l1-contracts/src/core/libraries/rollup/ValidatorSelectionLib.sol index aea12bad09a1..78948e616ea0 100644 --- a/l1-contracts/src/core/libraries/rollup/ValidatorSelectionLib.sol +++ b/l1-contracts/src/core/libraries/rollup/ValidatorSelectionLib.sol @@ -193,6 +193,7 @@ library ValidatorSelectionLib { * 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 _digest The digest of the block being proposed + * @param _updateCache Flag to identify that the proposer should be written to transient cache. * @custom:reverts Errors.ValidatorSelection__InvalidCommitteeCommitment if reconstructed committee doesn't match * stored commitment * @custom:reverts Errors.ValidatorSelection__MissingProposerSignature if proposer hasn't signed their attestation @@ -204,13 +205,13 @@ library ValidatorSelectionLib { CommitteeAttestations memory _attestations, address[] memory _signers, bytes32 _digest, - Signature memory _attestationsAndSignersSignature + Signature memory _attestationsAndSignersSignature, + bool _updateCache ) internal { - // Try load the proposer from cache - (address proposer, uint256 proposerIndex) = getCachedProposer(_slot); + uint256 proposerIndex; + address proposer; - // If not in cache, grab from the committee, reconstructed from the attestations and signers - if (proposer == address(0)) { + { // Load the committee commitment for the epoch (bytes32 committeeCommitment, uint256 committeeSize) = getCommitteeCommitmentAt(_epochNumber); @@ -234,12 +235,6 @@ library ValidatorSelectionLib { uint256 sampleSeed = getSampleSeed(_epochNumber); proposerIndex = computeProposerIndex(_epochNumber, _slot, sampleSeed, committeeSize); proposer = committee[proposerIndex]; - - setCachedProposer(_slot, proposer, proposerIndex); - } else { - // Assert that the size of the attestations is as expected, to avoid memory abuse on sizes. - // These checks are also performed inside `reconstructCommitteeFromSigners`. - _attestations.assertSizes(getStorage().targetCommitteeSize); } // We check that the proposer agrees with the proposal by checking that he attested to it. If we fail to get @@ -259,6 +254,10 @@ library ValidatorSelectionLib { bytes32 attestationsAndSignersDigest = _attestations.getAttestationsAndSignersDigest(_signers).toEthSignedMessageHash(); SignatureLib.verify(_attestationsAndSignersSignature, proposer, attestationsAndSignersDigest); + + if (_updateCache) { + setCachedProposer(_slot, proposer, proposerIndex); + } } /** @@ -349,8 +348,6 @@ library ValidatorSelectionLib { } } - address proposer = stack.reconstructedCommittee[stack.proposerIndex]; - require( stack.signaturesRecovered >= stack.needed, Errors.ValidatorSelection__InsufficientAttestations(stack.needed, stack.signaturesRecovered) @@ -361,8 +358,6 @@ library ValidatorSelectionLib { if (reconstructedCommitment != committeeCommitment) { revert Errors.ValidatorSelection__InvalidCommitteeCommitment(reconstructedCommitment, committeeCommitment); } - - setCachedProposer(_slot, proposer, stack.proposerIndex); } /** diff --git a/l1-contracts/test/AttestationLib.t.sol b/l1-contracts/test/AttestationLib.t.sol index bdfba23a0a82..bff11bc49727 100644 --- a/l1-contracts/test/AttestationLib.t.sol +++ b/l1-contracts/test/AttestationLib.t.sol @@ -17,10 +17,6 @@ contract AttestationLibWrapper { return AttestationLib.isEmpty(_attestations); } - function assertSizes(CommitteeAttestations memory _attestations, uint256 _expectedCount) external pure { - AttestationLib.assertSizes(_attestations, _expectedCount); - } - function reconstructCommitteeFromSigners( CommitteeAttestations memory _attestations, address[] memory _signers, @@ -78,52 +74,6 @@ contract AttestationLibTest is TestBase { _; } - function test_assertSizes(uint256 _signatureCount) public createValidAttestations(_signatureCount) { - attestationLibWrapper.assertSizes($attestations, SIZE); - } - - function test_assertSizes_wrongBitmapSize(uint256 _signatureCount, bool _over) - public - createValidAttestations(_signatureCount) - { - uint256 bitmapSize = $attestations.signatureIndices.length; - if (_over) { - $attestations.signatureIndices = new bytes(bitmapSize + 1); - } else { - $attestations.signatureIndices = new bytes(bitmapSize - 1); - } - - vm.expectRevert( - abi.encodeWithSelector( - Errors.AttestationLib__SignatureIndicesSizeMismatch.selector, bitmapSize, $attestations.signatureIndices.length - ) - ); - - attestationLibWrapper.assertSizes($attestations, SIZE); - } - - function test_assertSizes_wrongSignaturesOrAddressesSize(uint256 _signatureCount, bool _over) - public - createValidAttestations(_signatureCount) - { - uint256 dataSize = $attestations.signaturesOrAddresses.length; - if (_over) { - $attestations.signaturesOrAddresses = new bytes(dataSize + 1); - } else { - $attestations.signaturesOrAddresses = new bytes(dataSize - 1); - } - - vm.expectRevert( - abi.encodeWithSelector( - Errors.AttestationLib__SignaturesOrAddressesSizeMismatch.selector, - dataSize, - $attestations.signaturesOrAddresses.length - ) - ); - - attestationLibWrapper.assertSizes($attestations, SIZE); - } - function test_reconstructCommitteeFromSigners(uint256 _signatureCount) public createValidAttestations(_signatureCount) diff --git a/l1-contracts/test/validator-selection/ValidatorSelection.t.sol b/l1-contracts/test/validator-selection/ValidatorSelection.t.sol index a959a41bba69..d62027a4d852 100644 --- a/l1-contracts/test/validator-selection/ValidatorSelection.t.sol +++ b/l1-contracts/test/validator-selection/ValidatorSelection.t.sol @@ -374,24 +374,12 @@ contract ValidatorSelectionTest is ValidatorSelectionTestBase { } function testInvalidAddressAttestation() public setup(4, 4) progressEpochs(2) { - ProposeTestData memory ree = - _testBlock("mixed_block_1", NO_REVERT, 3, 4, TestFlagsLib.empty().invalidateAddressAttestation()); - - // We try to invalidate the count, but it got sufficient, so tx should revert - _invalidateByAttestationCount(ree, Errors.ValidatorSelection__InsufficientAttestations.selector); - - // We now invalidate the wrong attestation, no revert - // https://www.youtube.com/watch?v=glN0W8WogK8 - _invalidateByAttestationSig(ree, ree.invalidAddressAttestationIndex, NO_REVERT); - - // Try to prove to show that it can explode at this point, and we could not do anything before it. - // This should revert but won't if we did not invalidate - _proveBlocks( - "mixed_block_", - 1, - 1, - AttestationLibHelper.packAttestations(ree.attestations), - Errors.Rollup__InvalidBlockNumber.selector + _testBlock( + "mixed_block_1", + Errors.ValidatorSelection__InvalidCommitteeCommitment.selector, + 3, + 4, + TestFlagsLib.empty().invalidateAddressAttestation() ); } @@ -592,8 +580,6 @@ contract ValidatorSelectionTest is ValidatorSelectionTestBase { } } - // @todo figure out the attestationsAndSignersSignature - if (_flags.senderIsNotProposer) { ree.sender = address(uint160(uint256(keccak256(abi.encode("invalid", ree.proposer))))); } @@ -615,19 +601,6 @@ contract ValidatorSelectionTest is ValidatorSelectionTestBase { ) ).signature; } - - // By using this function we end up caching the correct proposer so we can skip the check in the real submission - // Only works in the same tx. - rollup.validateHeaderWithAttestations( - ree.proposeArgs.header, - AttestationLibHelper.packAttestations(ree.attestations), - ree.signers, - ree.attestationsAndSignersSignature, - digest, - bytes32(0), - BlockHeaderValidationFlags({ignoreDA: true}) - ); - // Change the last element in the committee (since it don't need a sig as we have enough earlier) // to be a random address instead of the expected one. address invalidAddress = address(uint160(uint256(keccak256(abi.encode("invalid", block.timestamp)))));