Skip to content

fix: never use cache in verifyProposer #16848

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.

Sign up for GitHub

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

Merged
LHerskind merged 1 commit into from
Sep 11, 2025
Merged

fix: never use cache in verifyProposer #16848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 0 additions & 43 deletions l1-contracts/src/core/libraries/rollup/AttestationLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
8 changes: 7 additions & 1 deletion l1-contracts/src/core/libraries/rollup/ProposeLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}

Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/src/core/libraries/rollup/RollupOperationsExtLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}

Expand Down
25 changes: 10 additions & 15 deletions l1-contracts/src/core/libraries/rollup/ValidatorSelectionLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand All @@ -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
Expand All @@ -259,6 +254,10 @@ library ValidatorSelectionLib {
bytes32 attestationsAndSignersDigest =
_attestations.getAttestationsAndSignersDigest(_signers).toEthSignedMessageHash();
SignatureLib.verify(_attestationsAndSignersSignature, proposer, attestationsAndSignersDigest);

if (_updateCache) {
setCachedProposer(_slot, proposer, proposerIndex);
}
}

/**
Expand Down Expand Up @@ -349,8 +348,6 @@ library ValidatorSelectionLib {
}
}

address proposer = stack.reconstructedCommittee[stack.proposerIndex];

require(
stack.signaturesRecovered >= stack.needed,
Errors.ValidatorSelection__InsufficientAttestations(stack.needed, stack.signaturesRecovered)
Expand All @@ -361,8 +358,6 @@ library ValidatorSelectionLib {
if (reconstructedCommitment != committeeCommitment) {
revert Errors.ValidatorSelection__InvalidCommitteeCommitment(reconstructedCommitment, committeeCommitment);
}

setCachedProposer(_slot, proposer, stack.proposerIndex);
}

/**
Expand Down
50 changes: 0 additions & 50 deletions l1-contracts/test/AttestationLib.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
39 changes: 6 additions & 33 deletions l1-contracts/test/validator-selection/ValidatorSelection.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
);
}

Expand Down Expand Up @@ -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)))));
}
Expand All @@ -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)))));
Expand Down
Loading