Skip to content

Conversation

@Stefan-Ethernal
Copy link
Contributor

@Stefan-Ethernal Stefan-Ethernal commented Oct 2, 2025

🔄 Changes Summary

  • In case the block finality is set to Empty, which is case for the BridgeL1Sync component, due to config BridgeL1Sync.BlockFinality being deprecated, we are hardcoding the block finality to the FinalizedBlock.
  • Also in case, Empty block finality is received to the EVMDownloader, we are returning an error.
  • Remove the BlockFinality from BridgeSync, as it was not being used anywhere, except the unit tests.
  • Rename BlockNumberFinality.GreaterThan to BlockNumberFinality.LessFinalThan, and make it slightly more understandable.
  • Remove redundant check when declaring block as finalized in evm downloader. It is enough checking whether the given block is less or equal to the last finalized block (whatever we have configured as the finalized commitment).

⚠️ Breaking Changes

N/A

📋 Config Updates

N/A

✅ Testing

  • 🤖 Automatic: Test_ResolveBlockFinality unit test
  • 🖱️ Manual:
  1. Spin up the aggkit with bridge service
  2. Note that there is not the following log entry in the aggkit log for L1BridgeSyncer, since it is now initialized with the finalized keyword for block finality:
"finalized block type FinalizedBlock is greater than block finality , setting finalized block type to "

🐞 Issues

🔗 Related PRs

  • [Optional: Enumerate related pull requests]

📝 Notes

  • [Optional: design decisions, tradeoffs, or TODOs]

@Stefan-Ethernal Stefan-Ethernal self-assigned this Oct 2, 2025
@Stefan-Ethernal Stefan-Ethernal marked this pull request as ready for review October 2, 2025 13:50
Copy link
Contributor

@joanestebanr joanestebanr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the case that BlockFinality is Empty?

@Stefan-Ethernal
Copy link
Contributor Author

Stefan-Ethernal commented Oct 3, 2025

What is the case that BlockFinality is Empty?

In the BridgeL1Sync config, the BlockFinality is omitted (it was supposed to be FinalizedBlock, but it isn't the case, since it is omitted and an empty string implicitly):

[BridgeL1Sync]
DBPath = "{{PathRWData}}/bridgel1sync.sqlite"
InitialBlockNum = 0
BridgeAddr = "{{polygonBridgeAddr}}"
SyncBlockChunkSize = 100
RetryAfterErrorPeriod = "1s"
MaxRetryAttemptsAfterError = -1
WaitForNewBlocksPeriod = "3s"
RequireStorageContentCompatibility = {{RequireStorageContentCompatibility}}
DBQueryTimeout = "{{defaultDBQueryTimeout}}"

We could provide here the aggkittypes.FinalizedBlock for sure, but with the fix I made we are sure that we are not going to set the Empty block finality.

@Stefan-Ethernal Stefan-Ethernal changed the title fix: override block finality in evm downloader fix: hardcode finalized block for bridge l1 syncer Oct 3, 2025
@Stefan-Ethernal Stefan-Ethernal force-pushed the fix/overriding-block-finality-in-evm-downloader branch from a534431 to e954b19 Compare October 3, 2025 09:15
@Stefan-Ethernal Stefan-Ethernal changed the base branch from feat/fix_e2e_fep_v2 to feat/aggsender-multisig October 3, 2025 10:12
@Stefan-Ethernal Stefan-Ethernal force-pushed the fix/overriding-block-finality-in-evm-downloader branch from d3ec811 to 0c89280 Compare October 4, 2025 06:47
@Stefan-Ethernal Stefan-Ethernal requested a review from a team October 6, 2025 09:00
Copy link
Contributor

@rachit77 rachit77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 7, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@Stefan-Ethernal Stefan-Ethernal merged commit ecdcd43 into feat/aggsender-multisig Oct 7, 2025
16 of 18 checks passed
@Stefan-Ethernal Stefan-Ethernal deleted the fix/overriding-block-finality-in-evm-downloader branch October 7, 2025 09:54
Stefan-Ethernal added a commit that referenced this pull request Oct 10, 2025
## 🔄 Changes Summary

Enable the AggSender to work with multiple validator nodes in a
committee-based validation system. The implementation includes validator
services, multisig committee management, certificate validation
improvements, and enhanced gRPC communication protocols.

**Multisig Committee Support:** 
- Added `MultisigCommittee` type to manage signer sets and enforce
signature thresholds.
- Signers are represented as `SignerInfo` structs with both address and
URL for improved context and error reporting.
- Committee construction validates non-empty membership and non-zero
threshold, preventing misconfiguration.
- Dynamic signer management: methods for adding signers, duplicate
checks by address and URL.

**Aggsender Validator Refactor:**
- The Aggsender certificate validation flow was refactored to integrate
multisig logic.
- The multisig validation logic is applicable to both `PP` and `FEP`
certificates.
- Certificate validation now checks for contiguous certificates, last L2
block, and settlement status using new queries.
- Import bridge exit proof verification is handled via new logic using
`verifyClaimProofs`, ensuring only valid proofs pass.

**Certificate Metadata removal:**
- It is gone from the agglayer and therefore it is not sent anymore from
the aggsender either.
- Only thing worth noting is that, when calculating `CertificateID`,
instead of metadata field, which was used previously, we now use
`ZeroHash`.

**Smart contracts integration:**
- **AggchainFEP contract:** Removed querying of `TrustedSequencer`
address and rely on the signers committee instead
- **AggchainBase contract:** Retrieve the multisig committee from the
`AggchainBase` contract

**Agglayer integration:**
- Invoke the `GetNetworkState` API from agglayer to get the latest
settled imported bridge exit info.
- Multisig is populated into the certificate and sent to the Agglayer's
`SendCertificate` gRPC endpoint

## ⚠️ Breaking Changes
- 🛠️ **Config**: Make sure that `Mode` on the `Validator` and
`AggSender` are the same.
- 🔌 **API/CLI**: `aggkit` version (`v0.7.0`) that supports `multisig`
will now require updated contracts to run. At least version
`v12.1.0-rc.3` of `agglayer-contracts`, and a new version of `agglayer`
which supports `multisig`, which is the `v0.4.0` of `agglayer`.
- 🗑️ **Deprecated Features**: Aggsender Phase II validator signing logic

## 📋 Config Updates
- Added `AggSender.RequireCommitteeMembershipCheck = false` parameter,
which defines if a check on `aggsender proposer` startup will be
performed to see if the proposer is in the `multisig` committee.
- Added `Validator.RequireCommitteeMembershipCheck =
{{AggSender.RequireCommitteeMembershipCheck}}` parameter, which defines
if a check on `aggsender validator` startup will be performed to see if
the validator is in the `multisig` committee.
- Added `Validator.Mode = "PessimisticProof"` parameter, which acts the
same as the `AggSender.Mode`. It tells the validator that the network is
a `PP` network or an `FEP` network. It has to be the same as on
`aggsender proposer`.
- Added `Validator.FEPConfig.SovereignRollupAddr =
"{{AggSender.SovereignRollupAddr}}" parameter which is the address of
the `AggchainFEP` rollup on L1 for given network for which validator is
running.
- Added `Validator.FEPConfig.RequireNoBlockGap =
{{AggSender.RequireNoFEPBlockGap}}, which acts the same as the given
paremeter on `AggSender` (proposer) config, and tells the validator if
gaps in blocks in certificates are allowed in `FEP` network.

```toml
[AggSender]
RequireCommitteeMembershipCheck = false

[Validator]
# PessimisticProof or AggchainProof
Mode = "PessimisticProof"
RequireCommitteeMembershipCheck = {{AggSender.RequireCommitteeMembershipCheck}}
[Validator.FEPConfig]
	SovereignRollupAddr = "{{AggSender.SovereignRollupAddr}}"
	RequireNoBlockGap = "{{AggSender.RequireNoFEPBlockGap}}"
```

## ✅ Testing
- 🤖 **Automatic**: `aggkit` CI
- 🖱️ **Manual**: [Optional: Steps to verify]

## 🐞 Issues
- Closes #792 
## 🔗 Related PRs
- #814
- #832
- #838
- #839
- #843
- #842
- #846
- #858
- #847
- #865
- #861
- #863
- #875
- #876
- #881
- #877
- #898
- #920
- #913
- #926
- #945
- #951
- #954
- #957
- #955
- #974
- #978
- #985
- #989
- #984
- #998
- #1017
- #1028
- #1034
- #1024
- #1052
- #1067
- #1068
- #1050
- #1071
- #1072
- #1060
- #1087
- #1077
- #1073

---------

Co-authored-by: Goran Rojovic <[email protected]>
Co-authored-by: Goran Rojovic <[email protected]>
Co-authored-by: Joan Esteban <[email protected]>
Co-authored-by: Rachit Sonthalia <[email protected]>
Co-authored-by: Arpit Temani <[email protected]>
Co-authored-by: Copilot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants