feat: update `tryFinalize()` to check finality on each L2 block #23

lesterli · 2024-07-12T01:54:06Z

Summary

This PR updates finalizer.go to check finality on each L2 block, tryFinalize() doesn't call on each L2 block b/c onPendingSafeUpdate only triggered upon a new L1 block.

add an FinalizerL2Interface interface which has the function L2BlockRefByNumber(ctx context.Context, num uint64) (eth.L2BlockRef, error)
update the function NewFinalizer to add this interface as its input parameter
update all the codes that call the function NewFinalizer and pass an instance of the FinalizerL2Interface interface

Test Plan

make lint-go
make build-go
cd op-node && make test

connect w FP e2e tests locally and see it passed

    op_test_manager.go:354: [17:55:15.448] Registered Finality Provider cceb4691002c79802720f1adb9573d66e44c18edcad2b26e59b80750eb14e2e8 for op-stack-l2-901
    base_test_manager.go:174: successfully submitted a BTC delegation
    base_test_manager.go:207: delegations are pending
    base_test_manager.go:227: delegations are active
    op_test_manager.go:430: [17:55:36.294] The test manager is running with 1 finality-provider(s)
    utils.go:100: Public randomness for fp cceb4691002c79802720f1adb9573d66e44c18edcad2b26e59b80750eb14e2e8 is successfully committed at height 64
    op_e2e_test.go:172: [17:56:31.538] Stopped the FP instance
    op_e2e_test.go:176: [17:56:31.538] last processed height 56
    op_e2e_test.go:189: [17:56:43.065] Test case 1: OP chain block finalized stuck at height 56
    op_e2e_test.go:198: [17:57:12.090] Restarted the FP instance
    op_e2e_test.go:202: [17:57:22.093] Test case 2: OP chain finality is recover, the latest finalized block height 95
    op_test_manager.go:480: Stopping test manager
--- PASS: TestFinalityStuckAndRecover (193.73s)
PASS
ok  	github.com/babylonchain/finality-provider/itest/opstackl2	201.464s

op-node/rollup/finality/finalizer.go

bap2pecs · 2024-07-16T21:52:18Z

updated to fix all comments ^

bap2pecs · 2024-07-16T22:05:02Z

@lesterli let's make sure we run all the commands in test plan since we have disabled CI for now.

I just found that cd op-node && make test fails

op-node/rollup/finality/finalizer.go

go.mod

op-node/rollup/finality/finalizer_test.go

op-node/rollup/finality/finalizer.go

bap2pecs · 2024-07-26T14:10:03Z

op-node/rollup/finality/finalizer.go

+			lastFinalizedBlock, err := fi.findFinalizedL2Block(fd.L2Block.Number, finalizedL2.Number)
+			if err != nil {


this confused me a bit b/c when there is err but we don't log it here. only after digging into the code, I see that it's already emitted inside findFinalizedL2Block

so given that we only need to check "where there is error", a better API design is to return a bool hasErr. wdyt?

also findFinalizedL2Block should be renamed to sth like findLastFinalizedL2BlockWithConsecutiveQuorom

fixed in 38f6625

bap2pecs · 2024-07-26T14:18:37Z

op-node/rollup/finality/finalizer.go

+			if err != nil {
+				if lastFinalizedBlock != (eth.L2BlockRef{}) {
+					// set finalized block(s)
+					finalizedL2, finalizedDerivedFrom = fi.updateFinalized(lastFinalizedBlock, fd.L1Block)


should we use lastFinalizedBlock.L1Origin here instead of fd.L1Block?

b/c lastFinalizedBlock might not be the same as fd.L2Block.Number

this is probably another edge case that test coverage failed. we should have a test case for this where fd.L1Block is different from lastFinalizedBlock.L1Origin

fixed in 38f6625

bap2pecs · 2024-07-26T14:34:49Z

op-node/rollup/finality/finalizer.go

+				break
+			}
+			// set finalized block(s)
+			finalizedL2, finalizedDerivedFrom = fi.updateFinalized(lastFinalizedBlock, fd.L1Block)


this is the same call as in L252. i think the overall logic from line 249-264 is not very clean

same for the if lastFinalizedBlock == (eth.L2BlockRef{}) { check. we checked it twice

we can do sth like this:

so we only need to call updateFinalized once; and check lastFinalizedBlock != (eth.L2BlockRef{}) once

fixed in 38f6625

bap2pecs · 2024-07-26T15:07:43Z

op-node/rollup/finality/finalizer.go

-			if err != nil && !errors.Is(err, sdk.ErrNoFpHasVotingPower) {
-				fi.emitter.Emit(rollup.CriticalErrorEvent{Err: fmt.Errorf("failed to check if block %d is finalized on Babylon: %w", fd.L2Block.Number, err)})
+
+			if hasErr {
 				return


thinking deeper on it. "return on err" is inappropriate now. if you see the below logic where if finalizedDerivedFrom != (eth.BlockID{}) { is checked, whenever finalizedDerivedFrom is set, we have to do some sanity checks.

so we need to:

remove the "return on error" logic

findLastFinalizedL2BlockWithConsecutiveQuorom doesn't need to return hasErr now

fixed in b28870d

bap2pecs · 2024-07-26T15:12:16Z

op-node/rollup/finality/finalizer.go

@@ -300,6 +292,80 @@ func (fi *Finalizer) tryFinalize() {
 	}
 }

+/*
+ *  findLastFinalizedL2BlockWithConsecutiveQuorom tries to find the finalized L2 block.


tries to find the last finalized L2 block with consecutive quorom

fixed in c89dd55

bap2pecs · 2024-07-26T15:22:08Z

op-node/rollup/finality/finalizer.go

+ *  for any other error types, emit an critical error event.
+ */
+func (fi *Finalizer) findLastFinalizedL2BlockWithConsecutiveQuorom(
+	fdL2BlockNumber uint64,
+	finalizedL2Number uint64,


let's add some comments to explain the return value and those arguments

fixed in c89dd55

bap2pecs · 2024-07-26T15:25:51Z

op-node/rollup/finality/finalizer.go

+		return eth.L2BlockRef{}
+	}
+
+	return l2Blocks[*lastFinalizedBlockNumber]


what if lastFinalizedBlockNumber is nil?

fixed in c89dd55

bap2pecs · 2024-07-26T15:28:49Z

op-node/rollup/finality/finalizer.go

+func (fi *Finalizer) findLastFinalizedL2BlockWithConsecutiveQuorom(
+	fdL2BlockNumber uint64,
+	finalizedL2Number uint64,
+) eth.L2BlockRef {


why not return a pointer?

i think using a pointer type will be cleaner so the check above

if lastFinalizedBlock.Number != fd.L2Block.Number {

will be changed to

if lastFinalizedBlock == nil || lastFinalizedBlock.Number != fd.L2Block.Number {

so it's more explicit

fixed in c89dd55

bap2pecs · 2024-07-26T15:41:51Z

op-node/rollup/finality/finalizer.go

+	// https://github.com/babylonchain/babylon-finality-gadget/issues/59
+	if err != nil && !errors.Is(err, sdkclient.ErrNoFpHasVotingPower) {
+		if lastFinalizedBlockNumber != nil {
+			fi.log.Warn("Received error but also had the finalized block", "error", err, "blockNumber", *lastFinalizedBlockNumber)


the logic here is not clean for the same reason that we have too many unnecessary nesting and condition checks

it can be simplified to be:

if there is error, emit

if last finalized block is not nil, return the block

else return nil

fixed in c89dd55

bap2pecs

given that this PR is already way too big. let's move all the changes in finalizer_test.go to another PR so we can start new discussion threads there

also the tests seem still failing

bap2pecs

ok I fixed all the comments above. @lesterli you can create a new PR to fix test. it's currently failing. I copied the code of finalizer_test.go to https://gist.github.com/bap2pecs/65e97276d3253e3973f138ab84878e74

I am gonna merge this PR now. I tested against FP e2e tests and all passed

let's keep the branch for now since I don't know if FP or SDK repo depends on some commits in this branch

finalize each l2 block

b514d31

lesterli marked this pull request as draft July 12, 2024 01:54

lesterli added 2 commits July 16, 2024 22:12

update for sdk client

0b364dc

update config

2870d95

lesterli requested review from gitferry, SebastianElvis, maurolacy, parketh, gusin13 and bap2pecs July 16, 2024 15:08

lesterli marked this pull request as ready for review July 16, 2024 15:08

lesterli marked this pull request as draft July 16, 2024 15:10

bap2pecs reviewed Jul 16, 2024

View reviewed changes

bap2pecs marked this pull request as ready for review July 16, 2024 18:42

bap2pecs reviewed Jul 16, 2024

View reviewed changes

op-node/rollup/finality/finalizer.go Outdated Show resolved Hide resolved

bap2pecs reviewed Jul 16, 2024

View reviewed changes

op-node/rollup/finality/finalizer.go Show resolved Hide resolved

bap2pecs reviewed Jul 16, 2024

View reviewed changes

op-node/rollup/finality/finalizer.go Outdated Show resolved Hide resolved

comments

b6d64d6

nit

3024cdb

SebastianElvis reviewed Jul 17, 2024

View reviewed changes

op-node/rollup/finality/finalizer.go Show resolved Hide resolved

lesterli added 5 commits July 17, 2024 19:59

fix some test cases

8fad1ec

nit

fa941f0

finalizer reorg test

7f7bdfd

fix bug

b89dae4

update deps

aad5f3f

bap2pecs reviewed Jul 25, 2024

View reviewed changes

op-node/rollup/finality/finalizer.go Outdated Show resolved Hide resolved

bap2pecs and others added 2 commits July 25, 2024 18:06

update deps

c21dbb6

update tryFinalizeOnConsecutiveBlockQuorom

6dca01f

fix

ea2292b

bap2pecs reviewed Jul 26, 2024

View reviewed changes

lesterli added 2 commits July 26, 2024 19:13

update and partially case

26acb0a

nit

3d5bdb9

lesterli requested a review from bap2pecs July 26, 2024 11:30

bap2pecs reviewed Jul 26, 2024

View reviewed changes

comments

38f6625

bap2pecs reviewed Jul 26, 2024

View reviewed changes

remove "return on err"

b28870d

bap2pecs reviewed Jul 26, 2024

View reviewed changes

comments

c89dd55

bap2pecs reviewed Jul 26, 2024

View reviewed changes

revert finalizer_test.go

c9aafd4

bap2pecs approved these changes Jul 26, 2024

View reviewed changes

bap2pecs merged commit 8d9427b into feat/babylon-rfc Jul 26, 2024
1 check failed

bap2pecs deleted the feat/check-each-l2-block branch July 26, 2024 23:56

bap2pecs restored the feat/check-each-l2-block branch July 26, 2024 23:57

bap2pecs mentioned this pull request Jul 27, 2024

fix tests #24

Closed

5 tasks

lesterli mentioned this pull request Jul 29, 2024

fix finalizer_test.go #25

Merged

bap2pecs mentioned this pull request Jul 31, 2024

uint tests babylonlabs-io/optimism#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update `tryFinalize()` to check finality on each L2 block #23

feat: update `tryFinalize()` to check finality on each L2 block #23

lesterli commented Jul 12, 2024 •

edited by bap2pecs

Loading

bap2pecs commented Jul 16, 2024

bap2pecs commented Jul 16, 2024

bap2pecs Jul 26, 2024 •

edited

Loading

bap2pecs Jul 26, 2024 •

edited

Loading

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024 •

edited

Loading

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs Jul 26, 2024

bap2pecs left a comment •

edited

Loading

bap2pecs left a comment •

edited

Loading

		lastFinalizedBlock, err := fi.findFinalizedL2Block(fd.L2Block.Number, finalizedL2.Number)
		if err != nil {

feat: update tryFinalize() to check finality on each L2 block #23

feat: update tryFinalize() to check finality on each L2 block #23

Conversation

lesterli commented Jul 12, 2024 • edited by bap2pecs Loading

Summary

Test Plan

bap2pecs commented Jul 16, 2024

bap2pecs commented Jul 16, 2024

bap2pecs Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

bap2pecs Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bap2pecs Jul 26, 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

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

Choose a reason for hiding this comment

bap2pecs left a comment • edited Loading

Choose a reason for hiding this comment

bap2pecs left a comment • edited Loading

Choose a reason for hiding this comment

feat: update `tryFinalize()` to check finality on each L2 block #23

feat: update `tryFinalize()` to check finality on each L2 block #23

lesterli commented Jul 12, 2024 •

edited by bap2pecs

Loading

bap2pecs Jul 26, 2024 •

edited

Loading

bap2pecs Jul 26, 2024 •

edited

Loading

bap2pecs Jul 26, 2024 •

edited

Loading

bap2pecs left a comment •

edited

Loading

bap2pecs left a comment •

edited

Loading