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

fix: circular deps finalized head #35

Merged
merged 44 commits into from
Nov 5, 2024
Merged

Conversation

parketh
Copy link
Contributor

@parketh parketh commented Oct 31, 2024

Summary

This PR addresses various circular dependency issues with the current FG implementation.

Problems

  • FG currently only processes blocks above the finalized head. However, the finalized head must be both btc+eth finalized. This means the FG never advances past the eth finalized block (it never achieves fast finality).
  • Basing the block processing logic on the RPC also introduces potential circular dependencies
  • block processing is currently very slow as it is sequential. if the FG falls behind for any reason (e.g. crash in another system service), it may never catch up (we observe this in our devnet)

Proposed fixes

  1. Decouple the startup loop from the block processing loop. Only use the eth finalized block on startup, not anywhere else in the code.
  2. Implement batch insert by parallelising calls
  3. Update QueryBlockRangeBabylonFinalized to query the internal DB rather than call the Babylon node

Test plan

make lint
make test

@parketh parketh requested review from bap2pecs and lesterli and removed request for bap2pecs October 31, 2024 20:12
@lesterli
Copy link
Contributor

lesterli commented Nov 2, 2024

for this change, we need to update the FP e2e test.
I just ran make test-e2e-op with this commit and new rpc QueryIsBlockBabylonFinalizedFromBabylon, got this error:

2024-11-02T12:58:34.164+0800    ERROR   finalitygadget/finalitygadget.go:275    Failed to get activated timestamp from database {"error": "database not open"}
github.com/babylonlabs-io/finality-gadget/finalitygadget.(*FinalityGadget).QueryBtcStakingActivatedTimestamp
        /Users/lyy/go/pkg/mod/github.com/babylonlabs-io/[email protected]/finalitygadget/finalitygadget.go:275
github.com/babylonlabs-io/finality-gadget/server.(*rpcServer).QueryBtcStakingActivatedTimestamp
        /Users/lyy/go/pkg/mod/github.com/babylonlabs-io/[email protected]/server/rpcserver.go:80
github.com/babylonlabs-io/finality-gadget/proto._FinalityGadget_QueryBtcStakingActivatedTimestamp_Handler
        /Users/lyy/go/pkg/mod/github.com/babylonlabs-io/[email protected]/proto/finalitygadget_grpc.pb.go:218
google.golang.org/grpc.(*Server).processUnaryRPC
        /Users/lyy/go/pkg/mod/google.golang.org/[email protected]/server.go:1379
google.golang.org/grpc.(*Server).handleStream
        /Users/lyy/go/pkg/mod/google.golang.org/[email protected]/server.go:1790
google.golang.org/grpc.(*Server).serveStreams.func2.1
        /Users/lyy/go/pkg/mod/google.golang.org/[email protected]/server.go:1029

@bap2pecs
Copy link
Contributor

bap2pecs commented Nov 2, 2024

I just ran make test-e2e-op with this commit and new rpc QueryIsBlockBabylonFinalizedFromBabylon, got this error:

I also met with this error testing my PR. the problem happens without this PR. i think it's likely caused by one of the commit here:

image

I guess this also means whenever we have a FG PR, we should test it with FP or it will be hard to bisect :)

@bap2pecs bap2pecs mentioned this pull request Nov 2, 2024
db/bbolt.go Show resolved Hide resolved
db/bbolt.go Show resolved Hide resolved
db/bbolt.go Show resolved Hide resolved
db/bbolt.go Show resolved Hide resolved
Copy link
Contributor

@bap2pecs bap2pecs left a comment

Choose a reason for hiding this comment

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

product management ship it - 9JyQbpKdPa1DeDAFyo

Copy link
Contributor

Choose a reason for hiding this comment

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

why is this removed? should we add back?

@bap2pecs
Copy link
Contributor

bap2pecs commented Nov 4, 2024

lgtm but will let @SebastianElvis take another look

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Great work! Some non-blocking comments

@@ -174,6 +166,39 @@ func (bb *BBoltHandler) GetBlockByHash(hash string) (*types.Block, error) {
return bb.GetBlockByHeight(blockHeight)
}

func (bb *BBoltHandler) QueryIsBlockRangeFinalizedByHeight(startHeight, endHeight uint64) ([]bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible that block h is finalised but block h+1 is not? I think it's not possiible as BTC staking enforces consecutive finalisation. So wdyt of maintaining the last finalised height in DB and accessing that value, instead of iterating over all these blocks?

Copy link
Contributor

@bap2pecs bap2pecs Nov 4, 2024

Choose a reason for hiding this comment

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

Would it be possible that block h is finalised but block h+1 is not?

no it's not possible (we can enforce it in bbolt.go as well but can go into another pr)

So wdyt of maintaining the last finalised height in DB and accessing that value, instead of iterating over all these blocks?

good point. we already maintain fist and last finalized heights. we can use them

const (
	blocksBucket          = "blocks"
	blockHeightsBucket    = "block_heights"
	indexerBucket         = "indexer"
	earliestBlockKey      = "earliest"
	latestBlockKey        = "latest"
	activatedTimestampKey = "activated_timestamp"
)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if using last finalised height then the logic can be greatly simplified. Can be done in a separate PR though

@parketh
Copy link
Contributor Author

parketh commented Nov 5, 2024

we tested it with a new devnet and it works! will merge to main

@parketh parketh merged commit 1e22461 into main Nov 5, 2024
11 checks passed
@parketh parketh deleted the fix/circular-deps-finalized-head branch November 5, 2024 13:54
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.

4 participants