Skip to content
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.

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

feat: agg-sender #22

Merged
merged 88 commits into from
Oct 31, 2024
Merged

feat: agg-sender #22

merged 88 commits into from
Oct 31, 2024

Conversation

goran-ethernal
Copy link
Collaborator

@goran-ethernal goran-ethernal commented Jul 26, 2024

Description

This PR implements the Aggsender component which is tasked of sending certificates to the agglayer using the interop_sendCeritificate rpc endpoint.

Config

Next code is an example of the Aggsender configuration:

[AggSender]
DBPath = "/tmp/aggsender" // path of the SQLite DB for the aggsender
AggLayerURL = "http://zkevm-agglayer" // URL to the agglayer
AggsenderPrivateKey = {Path = "/pk/sequencer.keystore", Password = "testonly"} // Aggsender private key that will sign the certificates before sending them to the agglayer
BlockGetInterval = "2s" // Interval for getting L1 blocks
URLRPCL2="http://test-aggoracle-l2:8545" // L2 node RPC URL
CheckSettledInterval = "2s" // Interval in which aggsender will check if sent certificates are settled
SaveCertificatesToFiles = false // indicates if sent certificates should be save to json files (for debugging purposes)

Required components (services)

The aggsender utilizes next components in the cdk repo to build a certificate:

  1. L1 client to get L1 blocks - certificates are settled on agglayer one by CDK in a single L1 epoch (configured by the EpochSize in the config), so we need to know how many L1 blocks have passed so we can send another certificate.
  2. L2 client - to get the last block on the L2. This is needed to know to which block we get claims and bridges on L2 to send them in a certificate.
  3. L2 bridge syncer - to get bridges, claims and blocks on L2, needed for a certificate.
  4. L1 info tree syncer - to build inclusion proofs.
  5. Agglayer client - needed to send certificates to the agglayer, and check if sent certificates were settled on the agglayer.

DB

Aggsender saves sent certificates in its local SQLite db. It has one table called certificate_info in which it saves this data:

type CertificateInfo struct {
	Height           uint64                     `meddler:"height"`
	CertificateID    common.Hash                `meddler:"certificate_id"`
	NewLocalExitRoot common.Hash                `meddler:"new_local_exit_root"`
	FromBlock        uint64                     `meddler:"from_block"`
	ToBlock          uint64                     `meddler:"to_block"`
	Status           agglayer.CertificateStatus `meddler:"status"`
}

Height - is a simple counter of this cdk's certificates. We can look at it as a nonce. It gets incremented by sending and settling certificates.
CertificateID - certificates Hash.
NewLocalExitRoot - new local exit root posted through the certificate (gotten from l2 bridge syncer).
FromBlock - block from which we got bridges and claims on L2.
ToBlock - block to which we got bridges and claims on L2.
Status - certificate status on agglayer. It can be Pending (pending to be settled), Settled, or InError (if some error happens on certificate settling).

Flow

image

@goran-ethernal goran-ethernal changed the title Feat: agg-sender feat: agg-sender Jul 26, 2024
@goran-ethernal goran-ethernal force-pushed the feat/agg-sender branch 2 times, most recently from c4fec21 to d5154cf Compare July 29, 2024 09:31
@goran-ethernal goran-ethernal force-pushed the feat/agg-sender branch 2 times, most recently from 8630c9e to b4cafa4 Compare August 9, 2024 12:04
@goran-ethernal goran-ethernal self-assigned this Aug 23, 2024
@goran-ethernal goran-ethernal force-pushed the feat/agg-sender branch 4 times, most recently from aae1445 to 746f647 Compare September 10, 2024 12:15
@goran-ethernal goran-ethernal force-pushed the feat/agg-sender branch 7 times, most recently from e7c7d70 to cd50970 Compare September 19, 2024 10:12
aggsender/aggsender.go Outdated Show resolved Hide resolved
aggsender/aggsender.go Show resolved Hide resolved
aggsender/aggsender.go Outdated Show resolved Hide resolved
aggsender/aggsender.go Outdated Show resolved Hide resolved
@goran-ethernal goran-ethernal force-pushed the feat/agg-sender branch 9 times, most recently from 651a5c4 to 489a89f Compare October 4, 2024 11:10
@vcastellm
Copy link
Contributor

This is being kept hidden by the GithubUI but it's important #22 (comment)

Copy link
Contributor

@arnaubennassar arnaubennassar left a comment

Choose a reason for hiding this comment

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

plz revert the changes done in #140 before merging

@joanestebanr
Copy link
Contributor

joanestebanr commented Oct 29, 2024

plz revert the changes done in #140 before merging

If we revert this PR we are going to have a bug if somebody calls bridgeAsset of Bridge Contract with param forceUpdateGlobalExitRoot=false.
I suggest:

  • Keep this code until we have a better solution (think in that as a workarround) until contracts are updated as you suggest
  • If we decide to delete this code we can implement a less efficient solution:
    • add a check to the synced Bridge and if have forceUpdateGlobalExitRoot=false prevent syncing this and keep in a 'waiting assets' lists on DB until appears a new bridgeAsset with forceUpdateGlobalExitRoot=true that then we can include the 'waiting assets list' .This suppose that in that in some moment there are going to be this call because the call updateGlobalExitRoot(), that is supported in feat: allow bridgeAsset with forceUpdateGlobalExitRoot=false #140 solution, is not going to allow to include this 'waiting asssets'.
    • Implement a polling pattern over the field LastUpdatedDepositCount that is a less efficent solution that the one on PR feat: allow bridgeAsset with forceUpdateGlobalExitRoot=false #140
  • Don't implement any solution to the bug and just detect it showing an error and stopping syncing: it require an extra call because the flag is in calldata and we are not retreiving it, so the previous solution is better. Also require an extra call but then it works

We have to decide this point, reverting without any other solution (so introduce a bug) i think that is not acceptable.

@vcastellm vcastellm changed the base branch from develop to release/0.4.0 October 29, 2024 09:30
@vcastellm
Copy link
Contributor

add a check to the synced Bridge and if have forceUpdateGlobalExitRoot=false prevent syncing this and keep in a 'waiting assets' lists on DB until appears a new bridgeAsset with forceUpdateGlobalExitRoot=true that then we can include the 'waiting assets list' .This suppose that in that in some moment there are going to be this call because the call updateGlobalExitRoot(), that is supported in #140 solution, is not going to allow to include this 'waiting asssets'.

I like this solution, isn't this how it's supposed to work?

goran-ethernal and others added 3 commits October 29, 2024 11:42
* use l1info root insetead of ger to generate merkleproof

* fix: uts

* fix: different l1 info trees test

* fix: ut

---------

Co-authored-by: Goran Rojovic <[email protected]>
@vcastellm vcastellm changed the base branch from release/0.4.0 to develop October 30, 2024 09:06
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Copy link

sonarcloud bot commented Oct 31, 2024

@joanestebanr joanestebanr dismissed arnaubennassar’s stale review October 31, 2024 09:47

The PR have been reverted

@joanestebanr joanestebanr merged commit 2a76deb into develop Oct 31, 2024
10 of 11 checks passed
@joanestebanr joanestebanr deleted the feat/agg-sender branch October 31, 2024 10:04
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.

5 participants