Fix several sync issues #583

vcastellm · 2024-02-16T16:06:25Z

What does this PR do?

Removed code that performed a check that was not actually used in real scenarios and was causing an error.
Claim transactions are now filtered by the destination network

Unified all E2E tests under a single workflow using matrix
Added support for running 2 {bridge service, zkevm node, prover} on the Makefile and docker-compose
Consolidated all the DBs under a single docker instance
Updated L1 docker image to use one with 2 rollups already attached, one with custom native gas token
Updated zkevm-node to a more recent version on docker-compose
re-structured the test config files under test/config to support having 2 services
etherman.NewL1Client receives a URL instead of the hole config struct, as this was the only used param
Changed primary key for sync.claim from (network_id, index) => (index, rollup_index, mainnet_flag) as this was giving conflicts on the test. Those conflicts could be avoided by ignoring the claims that happen on L1 from CDKs we don't sync, as long as each bridge service only syncs one CDK. Obviously we want (eventually) for the service to support syncing many CDKs
Added helper functions on etherman to support CDKs with custom native gas token
Added new mock smart contract to support how the custom native gas token used on the rollup 2 is minted
Many improvements to operartions/manager.go mostly related to using 2 networks and custom native gas token. But also cleaned a bunch of stuff... still lots of work to do here

Left a bunch of TODOs all over the place. WOuld be nice to transform them into issues before this hits develop

config/config.local.toml

vcastellm · 2024-02-16T19:10:11Z

Converting to draft as this is still WIP

Abf/fix

- Unified all E2E tests under a single workflow using matrix - Added support for running 2 {bridge service, zkevm node, prover} on the Makefile and docker-compose - Consolidated all the DBs under a single docker instance - Updated L1 docker image to use one with 2 rollups already attached, one with custom native gas token - Updated zkevm-node to a more recent version on docker-compose - re-structured the test config files under `test/config` to support having 2 services - `etherman.NewL1Client` receives a URL instead of the hole config struct, as this was the only used param - Changed primary key for `sync.claim` from `(network_id, index)` => `(index, rollup_index, mainnet_flag)` as this was giving conflicts on the test. Those conflicts could be avoided by ignoring the claims that happen on L1 from CDKs we don't sync, as long as each bridge service only syncs one CDK. Obviously we want (eventually) for the service to support syncing many CDKs - Added helper functions on etherman to support CDKs with custom native gas token - Added new mock smart contract to support how the custom native gas token used on the rollup 2 is minted - Many improvements to `operartions/manager.go` mostly related to using 2 networks and custom native gas token. But also cleaned a bunch of stuff... still lots of work to do here Left a bunch of TODOs all over the place. WOuld be nice to transform them into issues before this hits develop

arnaubennassar

Added some comments to help the reviewing process

arnaubennassar · 2024-02-29T13:49:53Z

db/pgstorage/migrations/0009.sql

+ADD COLUMN IF NOT EXISTS origin_rollup_id BIGINT DEFAULT 0;
+
+ALTER TABLE sync.claim DROP CONSTRAINT claim_pkey;
+ALTER TABLE sync.claim ADD PRIMARY KEY (index, rollup_index, mainnet_flag);


PK used to be index, metwork_id. This is wrong because network_id references destination network. Therefore there can be many claims with the same index (deposit count) and destination network. Example:

Rollup 1 does the first deposit to L1

Rollup 2 does the first deposit to L1
Once both deposits are claimed on L1, they will same index and and network id.

OTOH, index is unique per origin network, it's a counter (deposit count). And the way to index the origin network is with rollup index and mainnet flag, as explained in this comment:

// origin rollup ID is calculated as follows: // // if mainnet_flag: 0 // // else: rollup_index + 1 // destination rollup ID == network_id: network that has received the claim, therefore, the destination rollupID of the claim

arnaubennassar · 2024-02-29T13:50:31Z

db/pgstorage/pgstorage.go

-	const addDepositSQL = "INSERT INTO sync.deposit (leaf_type, network_id, orig_net, orig_addr, amount, dest_net, dest_addr, block_id, deposit_cnt, tx_hash, metadata) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) RETURNING id"
+	const addDepositSQL = `
+	INSERT INTO sync.deposit (
+		leaf_type, network_id, orig_net, orig_addr, amount, dest_net, dest_addr, block_id, deposit_cnt, tx_hash, metadata, origin_rollup_id


Added origin_rollup_id

arnaubennassar · 2024-02-29T13:52:32Z

db/pgstorage/pgstorage.go

@@ -198,13 +216,50 @@ func (p *PostgresStorage) AddTrustedGlobalExitRoot(ctx context.Context, trustedE
 }

 // GetClaim gets a specific claim from the storage.
-func (p *PostgresStorage) GetClaim(ctx context.Context, depositCount, networkID uint, dbTx pgx.Tx) (*etherman.Claim, error) {
+func (p *PostgresStorage) GetClaim(ctx context.Context, depositCount, originNetworkID, destNetworkID uint, dbTx pgx.Tx) (*etherman.Claim, error) {


This function now receives origin and destination network, to make sure we retrieve the correct calim. Altho now I realise that origin network and index (deposit count) would be enough. The code is still correct, and it used to be incorrect as it was asking for {index, network_id}

arnaubennassar · 2024-02-29T13:53:11Z

db/pgstorage/pgstorage.go

@@ -218,8 +273,28 @@ func (p *PostgresStorage) GetDeposit(ctx context.Context, depositCounterUser uin
 		deposit etherman.Deposit
 		amount  string
 	)
-	const getDepositSQL = "SELECT leaf_type, orig_net, orig_addr, amount, dest_net, dest_addr, deposit_cnt, block_id, b.block_num, d.network_id, tx_hash, metadata, ready_for_claim FROM sync.deposit as d INNER JOIN sync.block as b ON d.network_id = b.network_id AND d.block_id = b.id WHERE d.network_id = $1 AND deposit_cnt = $2"
-	err := p.getExecQuerier(dbTx).QueryRow(ctx, getDepositSQL, networkID, depositCounterUser).Scan(&deposit.LeafType, &deposit.OriginalNetwork, &deposit.OriginalAddress, &amount, &deposit.DestinationNetwork, &deposit.DestinationAddress, &deposit.DepositCount, &deposit.BlockID, &deposit.BlockNumber, &deposit.NetworkID, &deposit.TxHash, &deposit.Metadata, &deposit.ReadyForClaim)
+	const getDepositSQL = `


Modified to also select origin_rollup_id

arnaubennassar · 2024-02-29T13:53:32Z

db/pgstorage/pgstorage.go

@@ -516,7 +591,11 @@ func (p *PostgresStorage) GetClaims(ctx context.Context, destAddr string, limit

 // GetDeposits gets the deposit list which be smaller than depositCount.
 func (p *PostgresStorage) GetDeposits(ctx context.Context, destAddr string, limit uint, offset uint, dbTx pgx.Tx) ([]*etherman.Deposit, error) {
-	const getDepositsSQL = "SELECT leaf_type, orig_net, orig_addr, amount, dest_net, dest_addr, deposit_cnt, block_id, b.block_num, d.network_id, tx_hash, metadata, ready_for_claim FROM sync.deposit as d INNER JOIN sync.block as b ON d.network_id = b.network_id AND d.block_id = b.id WHERE dest_addr = $1 ORDER BY d.block_id DESC, d.deposit_cnt DESC LIMIT $2 OFFSET $3"
+	const getDepositsSQL = `


Modified to also select origin_rollup_id

arnaubennassar · 2024-02-29T13:55:37Z

etherman/etherman.go

@@ -434,6 +441,7 @@ func (etherMan *Client) depositEvent(ctx context.Context, vLog types.Log, blocks
 	deposit.TxHash = vLog.TxHash
 	deposit.Metadata = d.Metadata
 	deposit.LeafType = d.LeafType
+	deposit.OriginRollupID = etherMan.GetRollupID()


The only reliable way to know where a deposit comes from is contextual: since we are doing queries to a RPC, we know that a deposit event comes from that RPC, and we know the Rollup ID associated to that RPC

arnaubennassar · 2024-02-29T13:57:55Z

synchronizer/synchronizer.go

@@ -527,46 +527,12 @@ func (s *ClientSynchronizer) checkReorg(latestBlock *etherman.Block) (*etherman.
 }

 func (s *ClientSynchronizer) processVerifyBatch(verifyBatch etherman.VerifiedBatch, blockID uint64, dbTx pgx.Tx) error {
-	if verifyBatch.RollupID == s.etherMan.GetRollupID()-1 {


Please see this issue #586

vcastellm · 2024-02-29T14:15:23Z

db/pgstorage/migrations/0009.sql

+
+-- +migrate Down
+
+ALTER TABLE sync.claim DROP CONSTRAINT claim_pkey;


Should this be ADD PRIMARY KEY?

sonarqubecloud · 2024-03-06T09:37:25Z

Quality Gate passed

Issues
14 New issues
2 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.7% Duplication on New Code

See analysis details on SonarCloud

ARR552 · 2024-09-07T20:08:25Z

too old

vcastellm and others added 2 commits February 16, 2024 13:54

Remove unused check

a79d440

Fix the claim transaction

a48ab4f

vcastellm requested review from ARR552, tclemos and joanestebanr February 16, 2024 16:06

begmaroman reviewed Feb 16, 2024

View reviewed changes

config/config.local.toml Outdated Show resolved Hide resolved

vcastellm marked this pull request as draft February 16, 2024 19:09

arnaubennassar and others added 11 commits February 19, 2024 16:48

Fix wrong relation of claim_tx_hash

edbac37

WIP

355b9ab

looks to be working

bcf26f3

Merge pull request #585 from 0xPolygonHermez/abf/fix

f2c1706

Abf/fix

Remove commented Local Exit root existance check

a3c1bdb

Restore previous

118699a

typo

7d63f89

Rename

bcba97f

Merge remote-tracking branch 'origin' into vcastellm/fix-sync

f04e8a6

Refactor logic

fc9ff4b

Refactor claim.RollupIndex

ff6003f

0xPolygonHermez deleted a comment from cla-bot bot Feb 20, 2024

vcastellm and others added 3 commits February 20, 2024 15:00

Fix test

a7a1162

Pass ref

9921711

Fix UTs (#587)

726fed7

0xPolygonHermez deleted a comment from cla-bot bot Feb 29, 2024

cla-bot bot added the cla-signed label Feb 29, 2024

arnaubennassar added 2 commits February 29, 2024 13:33

Remove _XXX from Makefile

4163e21

Fix conflicts

ac15bcc

arnaubennassar reviewed Feb 29, 2024

View reviewed changes

vcastellm commented Feb 29, 2024

View reviewed changes

arnaubennassar added 12 commits February 29, 2024 15:31

add migration test

d60edb2

add migration test

4eed41e

Fix linter

8b1a5d7

Split migration file

7aa650c

TODOs for Monday :)

c00878c

Fix getting claims from deposits

1481244

Fix UT

762ea6f

Fix e2e

7516f21

Fix lint

2e78d3c

network_id is bigint

e7656e4

network_id is bigint

e7eba0e

network_id is bigint

21cf8cd

ARR552 closed this Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several sync issues #583

Fix several sync issues #583

vcastellm commented Feb 16, 2024 •

edited

Loading

vcastellm commented Feb 16, 2024

arnaubennassar left a comment

arnaubennassar Feb 29, 2024

arnaubennassar Feb 29, 2024

arnaubennassar Feb 29, 2024

arnaubennassar Feb 29, 2024

arnaubennassar Feb 29, 2024

arnaubennassar Feb 29, 2024

arnaubennassar Feb 29, 2024

vcastellm Feb 29, 2024

sonarqubecloud bot commented Mar 6, 2024

ARR552 commented Sep 7, 2024


		-- +migrate Down

		ALTER TABLE sync.claim DROP CONSTRAINT claim_pkey;

Fix several sync issues #583

Fix several sync issues #583

Conversation

vcastellm commented Feb 16, 2024 • edited Loading

What does this PR do?

vcastellm commented Feb 16, 2024

arnaubennassar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 6, 2024

Quality Gate passed

ARR552 commented Sep 7, 2024

vcastellm commented Feb 16, 2024 •

edited

Loading