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

test: op e2e verifier daemon itest #533

Open
wants to merge 7 commits into
base: base/consumer-chain-support
Choose a base branch
from

Conversation

parketh
Copy link
Collaborator

@parketh parketh commented Jul 26, 2024

Summary

This PR adds integration tests for the new verifier daemon in babylon-finality-gadget, a stateful program used to track consecutive quorum and query the latest BTC-finalized block.

Test Plan

make lint
make test-e2e-op

Comment on lines 115 to +120
test-e2e-op: clean-e2e install-babylond
@go test -race -mod=readonly -timeout=25m -v $(PACKAGES_E2E_OP) -count=1 --tags=e2e_op

TEST_NAME ?= .
test-e2e-op-single: clean-e2e install-babylond
@go test -mod=readonly -timeout=25m -v $(PACKAGES_E2E_OP) -count=1 --tags=e2e_op --run ^$(TEST_NAME)$
Copy link
Collaborator

Choose a reason for hiding this comment

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

ncie change! but can we merge w the above command instead of adding a new one?

FILTER ?= .
test-e2e-op: clean-e2e install-babylond
	@go test -mod=readonly -timeout=25m -v $(PACKAGES_E2E_OP) -count=1 --tags=e2e_op --run ^$(FILTER)$

I am also curious why make didn't complain since you didn't add test-e2e-op-single to .PHONY in Line 84

@@ -195,7 +196,7 @@ require (
github.com/aead/siphash v1.0.1 // indirect
github.com/andybalholm/brotli v1.1.0 // indirect
github.com/aws/aws-sdk-go v1.44.312 // indirect
github.com/babylonchain/babylon-finality-gadget v0.1.3-alpha.0.20240716025522-a7f8cd19f44f
github.com/babylonchain/babylon-finality-gadget v0.1.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does it work since v0.1.3 doesn't exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a replace later in the same file. There was some dependency bug when specifying the version directly here

github.com/cockroachdb/pebble => github.com/cockroachdb/pebble v0.0.0-20231018212520-f6cde3fc2fa4
github.com/ethereum-optimism/optimism => github.com/babylonchain/optimism v1.7.5-0.20240717115935-8fad1ec9aa03
github.com/ethereum-optimism/optimism => github.com/babylonchain/optimism v1.7.5-0.20240717131100-fa941f083b02
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you update it to latest commit on feat/babylon-rfc branch? I just merged the latest PR there

@@ -177,3 +179,43 @@ func TestFinalityStuckAndRecover(t *testing.T) {
"OP chain fianlity is recovered, the latest finalized block height %d",
), nextFinalizedHeight)
}

func TestOpVerifierDaemon(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

since a new test is added. let's also modify .circleci/config.yml to increase parallelism to 4 on Line 146 for our job so each our test will run on a separate container

Comment on lines +121 to +123
postgres := epg.NewDatabase(epg.DefaultConfig().Username("postgres").Password("postgres").Database("babylon").Port(5433))
err = postgres.Start()
require.NoError(t, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make it optional so tests that don't need to run verifier won't need to start a db?

actually I guess we won't need to to worry about it once we changed babylonchain/babylon-finality-gadget#62 to use https://github.com/etcd-io/bbolt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes i think this will be superseded by the db refactor. what we can do is start a db only if db configs are provided (ie make it optional)

itest/opstackl2/op_test_manager.go Show resolved Hide resolved

// get latest finalized block via API and check response
fmt.Printf("targetBlockHeight: %d\n", targetBlockHeight)
checkLatestConsecutivelyFinalizedBlock(t, 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

so 2 mean latest finalized block with consecutive quorom is block at height 2? then where do we need targetBlockHeight? I am a bit confused

Copy link
Collaborator Author

@parketh parketh Jul 27, 2024

Choose a reason for hiding this comment

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

when run normally, ctm.verifier.ProcessBlocks() will process blocks indefinitely, polling for and handling new blocks at a fixed interval. However, for testing purposes we call a modified function ctm.verifier.ProcessNBlocks() which terminates after processing N blocks, meaning it has a finite execution time.

Particularly, in L215 we specify ProcessNBlocks to terminate after processing N = 2 blocks, so we expect the DB to return the latest consecutively finalized block as N = 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just thought of it again. I think a better way is just to use the require.Eventually loop to wait for checkLatestConsecutivelyFinalizedBlock(targetBlockHeight+1) to return true. you can see many examples in this file or the test manager

so in this way, we don't need to create a special function only for testing purpose

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.

2 participants