这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@gusin13
Copy link
Contributor

@gusin13 gusin13 commented Jul 22, 2025

Closes
#521

Partially closes
#528
#535

whats included in this pr

  • adds e2e tests similar to babylon e2e
  • rm individual unit style tests
  • updates contract binary to latest and related config in test code
  • sets up local anvil node using public image, block time set to 1s, finality sig interval set to 5 rollup blocks

whats not included in this pr/still pending

  • ci related tests, there are 4-5 dedicated rollup-fpd commands which need to be tested, will add it in separate pr - issue
  • some refactoring is needed to improve test managers - issue

@gusin13 gusin13 linked an issue Jul 22, 2025 that may be closed by this pull request
2 tasks
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no bugs!


Bugbot free trial expires on July 31, 2025
Learn more in the Cursor dashboard.

@gusin13 gusin13 marked this pull request as ready for review July 27, 2025 21:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive E2E tests for the rollup BSN (Blockchain Security Network) and updates related infrastructure. The tests mirror the existing Babylon E2E test patterns but are specifically adapted for rollup environments.

  • Adds new E2E test suite covering FP lifecycle, double signing protection, and catch-up scenarios
  • Removes individual unit-style tests in favor of comprehensive E2E tests
  • Updates contract binaries and test configurations for rollup BSN testing

Reviewed Changes

Copilot reviewed 25 out of 29 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
bsn/rollup/e2e/rollup_e2e_test.go Adds comprehensive E2E tests for rollup BSN finality provider lifecycle
bsn/rollup/e2e/rollup_test_manager.go Updates test manager with Anvil support and contract allowlist management
itest/container/container.go Adds Anvil container support for local Ethereum node testing
finality-provider/service/finality_submitter.go Refactors batch signature submission to handle per-block proofs individually
Multiple files Updates import paths from rollup-finality-provider to rollup directory structure
Comments suppressed due to low confidence (1)

itest/container/config.go:40

  • The Anvil version 'v1.2.3' may not exist or be valid. Foundry/Anvil typically uses versions like 'nightly' or specific commit hashes. Please verify this version exists in the ghcr.io/foundry-rs/foundry repository.
		AnvilVersion:      "v1.2.3",

Comment on lines +267 to 285
for _, block := range blocks {
// Get public randomness for this specific height
pr, err := ds.GetPubRandList(block.GetHeight(), 1)
if err != nil {
return nil, fmt.Errorf("failed to get public randomness for height %d: %w", block.GetHeight(), err)
}
prList = append(prList, pr[0])

// Get proof for this specific height
proofs, err := ds.proofListGetterFunc(block.GetHeight(), 1)
if err != nil {
return nil, fmt.Errorf("failed to get public randomness inclusion proof for height %d: %w\nplease recover the randomness proof from db", block.GetHeight(), err)
}
if len(proofs) != 1 {
return nil, fmt.Errorf("expected exactly one proof for height %d, got %d", block.GetHeight(), len(proofs))
}
proofBytesList = append(proofBytesList, proofs[0])
}

Copy link

Copilot AI Jul 27, 2025

Choose a reason for hiding this comment

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

The refactored code now makes individual calls to get proofs for each block instead of batch retrieval. This could significantly impact performance when processing many blocks, as it increases the number of database/storage operations from 1 to N.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

@gitferry can you take a look at this too?

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 👍 That's quite a lot of things.

Would be great if @Lazar955 can also take a look

Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

Lgtm, fix comments I left.

@SebastianElvis @gusin13 Do these additioanl test feel redundant compared to the main FP ones?

@Lazar955
Copy link
Member

#554 this made everything run in parallel, not sure why the regression in this PR? @gusin13

@Lazar955
Copy link
Member

#554 this made everything run in parallel, not sure why the regression in this PR? @gusin13

I can fix this in next commit not to block merging of this PR, let's move forward but in future let's not kick the rock down the road :D

@SebastianElvis
Copy link
Member

Do these additioanl test feel redundant compared to the main FP ones?

I don't think they are redundant. At the end of the day these tests need to exist in every FP.

We might consider making the test suite more versatile such that writing e2e tests for BSN FPs becomes easier. We have a Github issue for tracking this #527

@Lazar955
Copy link
Member

Do these additioanl test feel redundant compared to the main FP ones?

I don't think they are redundant. At the end of the day these tests need to exist in every FP.

We might consider making the test suite more versatile such that writing e2e tests for BSN FPs becomes easier. We have a Github issue for tracking this #527

ah actually yeah you are correct regarding tests

@gusin13
Copy link
Contributor Author

gusin13 commented Jul 28, 2025

#554 this made everything run in parallel, not sure why the regression in this PR? @gusin13

I can fix this in next commit not to block merging of this PR, let's move forward but in future let's not kick the rock down the road :D

@Lazar955 I am not sure how to fix this, whenever I add t.Parallel() I keep getting these errors - this needs to be fixed separately - I see such issue is already tracked here
#519

Screenshot 2025-07-28 at 1 29 51 PM

@gusin13 gusin13 merged commit 80eb48b into main Jul 28, 2025
18 checks passed
@gusin13 gusin13 deleted the 528-e2e-more-e2e-tests-for-rollup-bsn-fps branch July 28, 2025 18:57
@Lazar955
Copy link
Member

#554 this made everything run in parallel, not sure why the regression in this PR? @gusin13

I can fix this in next commit not to block merging of this PR, let's move forward but in future let's not kick the rock down the road :D

@Lazar955 I am not sure how to fix this, whenever I add t.Parallel() I keep getting these errors - this needs to be fixed separately - I see such issue is already tracked here #519

Screenshot 2025-07-28 at 1 29 51 PM

but this is not due to port conflict error, but due to FP restart, and db file not open

@gusin13
Copy link
Contributor Author

gusin13 commented Jul 29, 2025

#554 this made everything run in parallel, not sure why the regression in this PR? @gusin13

I can fix this in next commit not to block merging of this PR, let's move forward but in future let's not kick the rock down the road :D

@Lazar955 I am not sure how to fix this, whenever I add t.Parallel() I keep getting these errors - this needs to be fixed separately - I see such issue is already tracked here #519
Screenshot 2025-07-28 at 1 29 51 PM

but this is not due to port conflict error, but due to FP restart, and db file not open

hm maybe my bad, i thought its due to some port conflicts, i had increased the sleep timer in tests when fp instance restarted which fixed the db open issue

thanks for enabling the t.Parallel() again 👍

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.

e2e: more e2e tests for rollup BSN FPs

4 participants