feat: allow changing L1 synced height via admin RPC/CLI #1044

colinlyguo · 2024-09-17T20:04:54Z

1. Purpose or design rationale of this PR

For two typical errors in the logs:

"failed to get batch chunk ranges, empty chunk block ranges": revert to a "safe" height, the updates are idempotent.
"Unexpected queue index in SyncService expected=805,224 got=805,329": revert to a height that contains the last synced L1 message.

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

feat: A new feature

3. Deployment tag versioning

Has the version in params/version.go been updated?

Yes

4. Breaking change label

Does this PR have the breaking-change label?

This PR is not a breaking change

HAOYUatHZ · 2024-09-18T03:47:35Z

How about we check if the new height is lower than the previously stored height?
If the new height is higher, we won't allow it to overwrite the previous value.

colinlyguo · 2024-09-18T04:46:32Z

41ab50d

yeah. it's a reasonable defense. added in 41ab50d.

internal/web3ext/web3ext.go

eth/api.go

colinlyguo · 2024-09-18T12:27:29Z

How about we check if the new height is lower than the previously stored height? If the new height is higher, we won't allow it to overwrite the previous value.

I found this brings up another issue, if someone updates the height to a very small height (e.g. block 1, which is << the deployment height), the height cannot be increased through this rpc.

georgehao · 2024-09-19T01:48:30Z

eth/api.go

+
+// SetRollupEventSyncedL1Height sets the synced L1 height for rollup event synchronization
+func (api *ScrollAPI) SetRollupEventSyncedL1Height(height uint64) error {
+	rollupSyncService := api.eth.GetRollupSyncService()


I think don't expose it to api, it's too dangerous, just cmd. Because any internal staff can connect to API and reset the height by API.

nice catch. currently moved to "admin" namespace in e39cdb1.

RPC providers do not expose the scroll_ namespace: https://bit.ly/scroll-l2geth-rpc. But I agree it is better to have this in the admin_ namespace.

rollup/sync_service/sync_service.go

colinlyguo · 2024-09-19T14:26:38Z

local test:
setL1MessageSyncedL1Height: success case.

> admin.setL1MessageSyncedL1Height(20785078);
null

INFO [09-19|14:12:25.368|eth/api.go:278]                                        Setting L1 message synced L1 height      height=20,785,078
INFO [09-19|14:12:25.369|rollup/sync_service/sync_service.go:155]               Reset sync service                       height=20,785,078
INFO [09-19|14:12:25.369|rollup/sync_service/sync_service.go:101]               Starting L1 message sync service         latestProcessedBlock=20,785,078
TRACE[09-19|14:12:26.332|rollup/sync_service/sync_service.go:173]               Sync service fetchMessages               latestProcessedBlock=20,785,078 latestConfirmed=20,785,074
TRACE[09-19|14:12:36.332|rollup/sync_service/sync_service.go:173]               Sync service fetchMessages               latestProcessedBlock=20,785,078 latestConfirmed=20,785,074

> scroll.syncStatus
{
  l1MessageSyncHeight: 20785106, -> a new height which > 20785078
  l1RollupSyncHeight: 18393700,
  l2BlockSyncHeight: 517679,
  l2FinalizedBlockHeight: 165610
}

setL1MessageSyncedL1Height: a wrong height.

DEBUG[09-19|14:09:35.352|rollup/sync_service/sync_service.go:241]               Received new L1 events                   fromBlock=20,670,001 toBlock=20,670,100 count=7
ERROR[09-19|14:09:35.353|rollup/sync_service/sync_service.go:252]               Unexpected queue index in SyncService    expected=937,164 got=932,518 msg="{QueueIndex:932518 Gas:168000 To:0x781e90f1c8Fc4611c9b7497C3B47F99Ef6969CbC Value:+0 Data:[142 241 51 46 0 0 0 0 0 0 0 0 0 0 0 0 83 163 156 144 242 176 244 166 135 92 196 20 104 205 235 5 64 68 146 215 0 0 0 0 0 0 0 0 0 0 0 0 83 163 156 144 242 176 244 166 135 92 196 20 104 205 235 5 64 68 146 215 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 11 212 158 11 26 32 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 14 58 166 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 160 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0] Sender:0x7885BcBd5CeCEf1336b5300fb5186A12DDD8c478}"

setRollupEventSyncedL1Height: success case.

> admin.setRollupEventSyncedL1Height(18306000);
null

INFO [09-19|13:50:15.749|eth/api.go:278]                                        Setting L1 message synced L1 height      height=18,306,000
INFO [09-19|13:50:15.750|rollup/sync_service/sync_service.go:155]               Reset sync service                       height=18,306,000
INFO [09-19|13:50:15.750|rollup/sync_service/sync_service.go:101]               Starting L1 message sync service         latestProcessedBlock=18,306,000
TRACE[09-19|13:50:16.693|rollup/sync_service/sync_service.go:173]               Sync service fetchMessages               latestProcessedBlock=18,306,000 latestConfirmed=20,784,978

> scroll.syncStatus
{
  l1MessageSyncHeight: 20784946,
  l1RollupSyncHeight: 18309600, -> a new height which > 18306000
  l2BlockSyncHeight: 508165,
  l2FinalizedBlockHeight: 443195
}

setRollupEventSyncedL1Height: a wrong height.

ERROR[09-19|13:41:21.751|rollup/rollup_sync_service/rollup_sync_service.go:206] failed to parse and update rollup event logs err="failed to get local node info, batch index: 8518, err: failed to get batch chunk ranges, empty chunk block ranges"

Thegaram · 2024-09-20T14:49:16Z

rollup/rollup_sync_service/rollup_sync_service.go

+	s.resetMu.Lock()
+	defer s.resetMu.Unlock()
+
+	s.Stop()


Why do we need to stop and restart?

Instead, we should just project latestProcessedBlock with a lock (i.e. lock in fetchMessages and ResetToHeight.

Then we also don't need to reinitialize the context here.

aha, it makes sense. changed in c588232, currently, there are some read/write ops related latestProcessedBlock and db, thus added a function level write lock.

refactored to use atomic.Uint64 for latestProcessedBlock in 75413e6.

omerfirmak · 2024-09-23T12:12:47Z

rollup/sync_service/sync_service.go

+	s.stateMu.Lock()
+	defer s.stateMu.Unlock()
+
+	rawdb.WriteSyncedL1BlockNumber(s.db, height)


I don't think you need to do this, no? flush() in fetchMessageswill do that anyways.

Probably same applies to (s *RollupSyncService) ResetToHeight as well

changed in 75413e6.

omerfirmak · 2024-09-23T12:15:02Z

rollup/sync_service/sync_service.go

@@ -50,6 +51,8 @@ type SyncService struct {
 	pollInterval         time.Duration
 	latestProcessedBlock uint64
 	scope                event.SubscriptionScope
+
+	stateMu sync.Mutex // protects the service state, e.g. db and latestProcessedBlock updates


I would expect simply making latestProcessedBlock an atomic.Uint64 would work just fine instead of adding a mutex here.

changed in 75413e6.

feat: allow changing L1 synced height via RPC/CLI

ea1d2fd

colinlyguo added the bump-version label Sep 17, 2024

colinlyguo and others added 2 commits September 17, 2024 20:05

chore: auto version bump [bot]

47fb5c5

fix typos

0751c86

add height validity check

41ab50d

colinlyguo requested review from HAOYUatHZ, omerfirmak, georgehao and Thegaram September 18, 2024 05:24

omerfirmak previously approved these changes Sep 18, 2024

View reviewed changes

Thegaram reviewed Sep 18, 2024

View reviewed changes

internal/web3ext/web3ext.go Outdated Show resolved Hide resolved

eth/api.go Outdated Show resolved Hide resolved

change implementation

d38f656

colinlyguo dismissed omerfirmak’s stale review via d38f656 September 18, 2024 19:43

georgehao reviewed Sep 19, 2024

View reviewed changes

colinlyguo added 2 commits September 19, 2024 14:35

moved hotfix apis from ScrollAPI to PrivateAdminAPI

e39cdb1

change namespace from scroll to admin

e124dce

colinlyguo commented Sep 19, 2024

View reviewed changes

rollup/sync_service/sync_service.go Outdated Show resolved Hide resolved

fix bugs

dc03c12

colinlyguo requested review from georgehao, omerfirmak and Thegaram September 19, 2024 14:28

add locks

b6670b5

colinlyguo changed the title ~~feat: allow changing L1 synced height via RPC/CLI~~ feat: allow changing L1 synced height via admin RPC/CLI Sep 19, 2024

Thegaram reviewed Sep 20, 2024

View reviewed changes

colinlyguo and others added 2 commits September 23, 2024 19:35

add a lock to protect latestProcessedBlock and db state

c588232

Merge branch 'develop' into feat-allow-changing-l1-synced-height

e0fc1e5

revert some changes

eaa6eeb

omerfirmak reviewed Sep 23, 2024

View reviewed changes

colinlyguo added 3 commits September 23, 2024 20:49

remove lock and use atomic in latestProcessedBlock

75413e6

fix CI

bc40b7a

tweak

2662c6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow changing L1 synced height via admin RPC/CLI #1044

feat: allow changing L1 synced height via admin RPC/CLI #1044

colinlyguo commented Sep 17, 2024 •

edited

Loading

HAOYUatHZ commented Sep 18, 2024 •

edited

Loading

colinlyguo commented Sep 18, 2024 •

edited

Loading

colinlyguo commented Sep 18, 2024 •

edited

Loading

georgehao Sep 19, 2024 •

edited

Loading

colinlyguo Sep 19, 2024

Thegaram Sep 20, 2024

colinlyguo commented Sep 19, 2024 •

edited

Loading

Thegaram Sep 20, 2024

colinlyguo Sep 23, 2024

colinlyguo Sep 23, 2024

omerfirmak Sep 23, 2024

omerfirmak Sep 23, 2024

colinlyguo Sep 23, 2024

omerfirmak Sep 23, 2024

colinlyguo Sep 23, 2024

feat: allow changing L1 synced height via admin RPC/CLI #1044

Are you sure you want to change the base?

feat: allow changing L1 synced height via admin RPC/CLI #1044

Conversation

colinlyguo commented Sep 17, 2024 • edited Loading

1. Purpose or design rationale of this PR

2. PR title

3. Deployment tag versioning

4. Breaking change label

HAOYUatHZ commented Sep 18, 2024 • edited Loading

colinlyguo commented Sep 18, 2024 • edited Loading

colinlyguo commented Sep 18, 2024 • edited Loading

georgehao Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinlyguo commented Sep 19, 2024 • edited Loading

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

colinlyguo commented Sep 17, 2024 •

edited

Loading

HAOYUatHZ commented Sep 18, 2024 •

edited

Loading

colinlyguo commented Sep 18, 2024 •

edited

Loading

colinlyguo commented Sep 18, 2024 •

edited

Loading

georgehao Sep 19, 2024 •

edited

Loading

colinlyguo commented Sep 19, 2024 •

edited

Loading