Skip to content

fix(api-backend): disable finalized tag when rollup verify is disabled #722

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

Jump to bottom
Merged
merged 1 commit into from
Apr 26, 2024
Merged

fix(api-backend): disable finalized tag when rollup verify is disabled #722

merged 1 commit into from
Apr 26, 2024

Conversation

colinlyguo
Copy link
Member

@colinlyguo colinlyguo commented Apr 25, 2024 •
edited
Loading

Disables the "finalized tag" when the "rollup verify" feature is disabled.

Otherwise, if one node previously enabled --rollup.verify but then disable it, when it queries a "finalized" block, it will still get a finalized block number but an incorrect one.

Related previous PR: #548

@georgehao
Copy link
Member

Disables the "finalized tag" when the "rollup verify" feature is disabled.

Otherwise, if one node previously enabled --rollup.verify but then disable it, when it queries a "finalized" block, it will still get a finalized block number but an incorrect one.

Related previous PR: #548

small question:

  1. previously enable --rollup.verify , then disable it ---> is it need restart the execution to disable?
  2. why restart the execution still get the wrong one?

@colinlyguo
Copy link
Member Author

Disables the "finalized tag" when the "rollup verify" feature is disabled.
Otherwise, if one node previously enabled --rollup.verify but then disable it, when it queries a "finalized" block, it will still get a finalized block number but an incorrect one.
Related previous PR: #548

small question:

  1. previously enable --rollup.verify , then disable it ---> is it need restart the execution to disable?
  2. why restart the execution still get the wrong one?

For 1. Yes. And usually, it's just a mistake due to config changes. e.g., migrating to other scripts but forgetting to add --rollup.verify.
For 2. After restarting the node without --rollup.verify, it will not sync the new finalized batch anymore, thus the "finalized" block height stops increasing.

@colinlyguo
Copy link
Member Author

This PR can also remind the node's owner whether --rollup.verify is open, easier to debug and detect issues.

@colinlyguo colinlyguo merged commit a860446 into develop Apr 26, 2024
7 checks passed
@colinlyguo colinlyguo deleted the fix-disable-finalized-tag-when-rollup-verify-is-disabled branch April 26, 2024 04:11
0xmountaintop pushed a commit that referenced this pull request Jun 26, 2024
@colinlyguo @0xmountaintop
fix(api-backend): disable finalized tag when rollup verify is disabled (
ecb8a92 
#722)
0xmountaintop added a commit that referenced this pull request Jun 26, 2024
@0xmountaintop @colinlyguo @Thegaram
sync rollup_sync_service fixes (#860)
a83c05c 
* feat(rollup): add fallback if TransactionByHash fails in getting commitBatch calldata (#601)

* feat(rollup): add fallback if TransactionByHash fails in getting commitBatch calldata

* Update rollup/rollup_sync_service/rollup_sync_service.go

Co-authored-by: Péter Garamvölgyi <peter@scroll.io>

* change log.Warn to log.Debug

---------

Co-authored-by: Péter Garamvölgyi <peter@scroll.io>

* fix(rollup sync service): remove syscall.Kill (#636)

* fix(rollup sync service): remove syscall.Kill

* bump version

* fix(api-backend): disable finalized tag when rollup verify is disabled (#722)

---------

Co-authored-by: colin <102356659+colinlyguo@users.noreply.github.com>
Co-authored-by: Péter Garamvölgyi <peter@scroll.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgehao georgehao georgehao approved these changes

@Thegaram Thegaram Thegaram approved these changes

@0xmountaintop 0xmountaintop 0xmountaintop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@colinlyguo @georgehao @Thegaram @0xmountaintop