Skip to content

test: adding compatibility test to perform syncing#3023

Merged
julienrbrt merged 16 commits intomainfrom
cian/compat-test
Feb 3, 2026
Merged

test: adding compatibility test to perform syncing#3023
julienrbrt merged 16 commits intomainfrom
cian/compat-test

Conversation

@chatton
Copy link
Copy Markdown
Contributor

@chatton chatton commented Jan 28, 2026

Overview

This PR adds a compatibility test that will deploy a specified image, execute some transactions, then deploy the PR image and verify that syncing still works correctly.

closes #3019

Add EVMCompatTestSuite to test syncing between different ev-node
versions. The test:
- Starts sequencer with version A (env: SEQUENCER_EVM_IMAGE_*)
- Submits transactions
- Starts full node with version B (env: FULLNODE_EVM_IMAGE_*)
- Verifies full node syncs and can query transactions
- Submits post-sync transactions to verify ongoing sync

Add make test-docker-compat target for running the test.
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 28, 2026

Claude finished @chatton's task in 1m 57s —— View job


Code Review Complete ✅

I've reviewed this PR, which adds a valuable compatibility test suite to verify cross-version syncing between different ev-node-evm versions. Overall, this is a well-structured addition that enhances the testing infrastructure. Below are my findings organized by category.


Summary

Strengths:

  • Well-designed compatibility test that validates real-world upgrade scenarios
  • Clean refactoring of shared test utilities into reusable components
  • Good separation of concerns between sequencer and full node setup
  • Comprehensive test coverage including transaction verification

Issues Identified:

  • 1 blocking issue (flaky sleep-based synchronization)
  • 1 architectural improvement opportunity
  • Several minor improvements for robustness and clarity

🔴 Blocking Issues

1. Flaky Sleep-Based Synchronization (test/docker-e2e/compat_test.go:68)

Issue: Using time.Sleep(5 * time.Second) to wait for DA propagation is flaky and can cause intermittent test failures.

// wait for blocks to be posted to DA before starting full node
time.Sleep(5 * time.Second)

Impact: Tests may fail in slower CI environments or pass when they shouldn't in faster ones.

Recommendation: Replace with a conditional wait that checks for block production progress:

s.T().Log("Waiting for DA propagation...")
lastTxHeight, err := s.sequencerClient.BlockNumber(ctx)
s.Require().NoError(err)
const daPropagationBlocks = 3
targetHeight := lastTxHeight + daPropagationBlocks
s.Require().Eventually(func() bool {
    h, err := s.sequencerClient.BlockNumber(ctx)
    return err == nil && h >= targetHeight
}, 15*time.Second, time.Second, "sequencer did not produce blocks for DA propagation")

This ties the wait to actual block production rather than arbitrary time, making it both more reliable and potentially faster.

Reference: test/docker-e2e/compat_test.go:68

Fix this issue →


🟡 Architectural Improvements

2. Missing Cleanup/Teardown Logic

Observation: The test suite doesn't have explicit teardown logic for Docker containers. While the test framework may handle this, explicit cleanup ensures resources are released even on test failures.

Recommendation: Add a TearDownSuite method:

func (s *EVMCompatTestSuite) TearDownSuite() {
    ctx := context.Background()
    if s.fullNode != nil {
        _ = s.fullNode.Remove(ctx)
    }
    if s.sequencerNode != nil {
        _ = s.sequencerNode.Remove(ctx)
    }
}

Reference: test/docker-e2e/compat_test.go:25-36


🟢 Code Quality & Best Practices

3. Good Test Organization and Readability

Strengths:

  • Excellent use of subtests (s.Run()) to organize test phases
  • Clear, descriptive test phase names that document the test flow
  • Good logging throughout that aids debugging
  • Proper use of s.Require() for test assertions

4. Well-Structured Helper Functions

The refactoring in docker_test.go is excellent:

  • SetupRethNode encapsulates complex setup logic
  • SetupCelestiaAndDABridge is properly reusable
  • SetupEVMSingle follows the builder pattern consistently
  • Good naming conventions throughout

5. Environment Variable Configuration

The use of environment variables for image configuration is appropriate:

  • getSequencerImage() and getFullNodeImage() provide sensible defaults
  • Clear documentation via the GitHub workflow file
  • Proper trimming of whitespace from env vars

🔍 Error Handling & Resilience

6. Comprehensive Error Checking

Strengths:

  • All error returns are properly checked with s.Require().NoError(err)
  • Error messages include contextual information (e.g., transaction indices)
  • Good use of formatted error messages for debugging

7. Timeout Configuration

Observation: The waitForSync function uses a 120-second timeout, which seems generous but appropriate for cross-version sync testing.

func (s *EVMCompatTestSuite) waitForSync(ctx context.Context, targetHeight uint64) {
    s.Require().Eventually(func() bool {
        height, err := s.fullNodeClient.BlockNumber(ctx)
        return err == nil && height >= targetHeight
    }, 120*time.Second, 2*time.Second, "full node did not sync to height %d", targetHeight)
}

Good practice: The timeout is appropriate given the cross-network nature of the test.

Reference: test/docker-e2e/compat_test.go:204-210


🔒 Security Considerations

8. Hardcoded Test Credentials

Observation: The test uses hardcoded private keys and passphrases:

  • evmTestPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"
  • WithEVMSignerPassphrase("secret")

Status:Acceptable - These are test-only credentials clearly marked as constants in test files. They're not used in production code.

Reference: test/docker-e2e/docker_test.go:44 and compat_test.go:103


⚡ Performance & Resource Efficiency

9. Transaction Nonce Management

Good practice: The suite properly manages nonce state with s.txNonce, preventing nonce conflicts:

tx := evm.GetRandomTransaction(s.T(), evmTestPrivateKey, evmTestToAddress, evmTestChainID, evmTestGasLimit, &s.txNonce)

Reference: test/docker-e2e/compat_test.go:194

10. Sequential Transaction Submission

Observation: Transactions are submitted sequentially with immediate inclusion verification:

s.Require().NoError(s.sequencerClient.SendTransaction(ctx, tx))
// ...
s.WaitForTxIncluded(ctx, s.sequencerClient, tx.Hash())

Status:Appropriate - This ensures deterministic test behavior and proper ordering.


📋 Testing & Reliability

11. Comprehensive Transaction Verification

Excellent: The verifyTransactionsOnFullNode function validates both:

  • Transaction status (success/failure)
  • Block number consistency between sequencer and full node
s.Require().Equal(seqReceipt.BlockNumber.Uint64(), fnReceipt.BlockNumber.Uint64(),
    "tx %d block number mismatch: sequencer=%d, fullnode=%d",
    i, seqReceipt.BlockNumber.Uint64(), fnReceipt.BlockNumber.Uint64())

Reference: test/docker-e2e/compat_test.go:212-226

12. P2P Address Resolution

Good implementation: The getSequencerP2PAddress function properly:

  • Reads the node key file
  • Unmarshals and parses the Ed25519 public key
  • Constructs the multiaddr with DNS hostname
  • Uses libp2p peer ID derivation

Reference: test/docker-e2e/compat_test.go:172-189


📚 Documentation & Developer Experience

13. Missing TODO Follow-up

Observation: The workflow file contains a TODO:

SEQUENCER_EVM_IMAGE_TAG: main # TODO: set this to the last released tag

Recommendation: Create a follow-up issue to:

  • Determine the appropriate stable release tag to use
  • Update the workflow once a stable release exists
  • Consider parameterizing this for testing against multiple versions

Reference: .github/workflows/docker-tests.yml:68

14. Test Documentation

Strength: The test struct and functions have clear docstrings:

// EVMCompatTestSuite tests cross-version compatibility between different ev-node-evm versions.

Minor improvement: Consider adding a package-level comment explaining the compatibility test strategy and requirements.


🔧 Dependency Updates

15. Go Version and Module Updates

Changes reviewed:

  • Go version: 1.25.01.25.6 (patch update, safe)
  • Added github.com/libp2p/go-libp2p/core dependency (required for peer ID derivation)
  • Updated various transitive dependencies via Dependabot

Status:Approved - All dependency updates appear routine and safe.


📦 Build & CI Integration

16. New Make Target

Good addition: The test-docker-compat target is well-structured:

test-docker-compat:
	@echo "--> Running Docker Sync Compatibility E2E tests"
	@cd test/docker-e2e && go test -mod=readonly -failfast -v -tags='docker_e2e evm' -timeout=30m -run '^TestEVMCompatSuite$$/^TestCrossVersionSync$$' ./...

Reference: scripts/test.mk:70-72

17. GitHub Workflow Integration

Well-designed: The new workflow job properly:

  • Uses separate environment variables for sequencer and full node images
  • Runs in isolation from other tests
  • Has appropriate permissions (contents: read)

Reference: .github/workflows/docker-tests.yml:53-71


📊 Test Coverage

Note: Codecov shows that all modified lines are covered, and overall coverage improved by +0.05%. The new test code itself is integration test code, so it doesn't require unit test coverage.


Final Verdict

This PR is nearly ready to merge pending the fix for the sleep-based synchronization issue. The code quality is high, the design is sound, and the testing additions are valuable for ensuring cross-version compatibility.

Required Changes:

  1. ✅ Fix the flaky time.Sleep at line 68 (blocking)

Recommended Changes:
2. Consider adding explicit teardown logic
3. Follow up on the TODO in the workflow file

Approval Status: ⏸️ Pending fix for Issue #1


Additional Context

This compatibility test will be crucial for:

  • Validating rolling upgrades in production
  • Catching breaking changes in serialization formats
  • Ensuring backward compatibility of the sync protocol

Great work on this addition! 🎉


@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 28, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedFeb 3, 2026, 7:50 PM

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @chatton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the testing infrastructure by introducing a robust Docker E2E compatibility test. This new test ensures that different versions of the ev-node-evm can seamlessly synchronize, specifically verifying that a newer full node can correctly process and validate data from an older sequencer. The changes also include refactoring of test utilities to promote cleaner, more maintainable code and updating core dependencies to their latest versions, contributing to overall system stability and future compatibility.

Highlights

  • New Compatibility Test Suite: A new Docker E2E test suite, EVMCompatTestSuite, has been introduced to verify cross-version syncing capabilities of the ev-node-evm. This test deploys an older sequencer image, executes transactions, and then deploys the current PR image as a full node to ensure it can correctly sync and validate the historical transactions.
  • Docker E2E Test Utilities Refactoring: Existing Docker E2E test utilities have been refactored for improved reusability and clarity. This includes defining shared EVM test constants, renaming RethSetup to RethSetupConfig, and introducing EVMSingleSetupConfig and SetupEVMSingle helper functions to streamline node setup.
  • Dependency and Go Version Updates: The Go version has been updated from 1.25.0 to 1.25.6, and several Go module dependencies, including github.com/fatih/color, github.com/google/flatbuffers, github.com/hashicorp/go-hclog, and github.com/celestiaorg/tastora, have been updated to newer versions. New dependencies like github.com/libp2p/go-libp2p/core were also added.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/docker-tests.yml
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a valuable compatibility test suite to ensure syncing works across different node versions. The new EVMCompatTestSuite effectively simulates a real-world scenario where a new full node syncs from a sequencer running a different version. The changes also include beneficial refactoring of the existing test suite, such as centralizing test constants and creating reusable helper functions for setting up test nodes. My review includes one suggestion to improve the robustness of the new test by replacing a fixed sleep with a conditional wait.

Comment on lines +68 to +71
time.Sleep(5 * time.Second)

sequencerHeight, err := s.sequencerClient.BlockNumber(ctx)
s.Require().NoError(err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a fixed time.Sleep can introduce flakiness into tests, as the time required for an operation can vary between test runs. A more robust approach is to wait for a specific condition. Here, you're waiting for blocks to be posted to the DA. While directly querying the DA might be complex, you can achieve a more reliable wait by ensuring a few more blocks are produced by the sequencer. This provides a time window for DA propagation that is tied to block production rather than a fixed duration.

s.T().Log("Waiting for a few blocks to allow for DA propagation...")
	lastTxHeight, err := s.sequencerClient.BlockNumber(ctx)
	s.Require().NoError(err)
	const daPropagationBlocks = 3
	targetHeight := lastTxHeight + daPropagationBlocks
	s.Require().Eventually(func() bool {
		h, err := s.sequencerClient.BlockNumber(ctx)
		return err == nil && h >= targetHeight
	}, 15*time.Second, time.Second, "sequencer did not produce new blocks for DA propagation")

	sequencerHeight, err := s.sequencerClient.BlockNumber(ctx)
	s.Require().NoError(err)

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.19%. Comparing base (120195f) to head (b366a45).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3023      +/-   ##
==========================================
- Coverage   56.24%   56.19%   -0.05%     
==========================================
  Files         118      118              
  Lines       12075    12075              
==========================================
- Hits         6791     6786       -5     
- Misses       4537     4542       +5     
  Partials      747      747              
Flag Coverage Δ
combined 56.19% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chatton chatton marked this pull request as ready for review January 29, 2026 14:54
@julienrbrt julienrbrt added this pull request to the merge queue Feb 1, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 1, 2026
@julienrbrt julienrbrt enabled auto-merge February 2, 2026 16:37
@julienrbrt julienrbrt added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@julienrbrt julienrbrt added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@julienrbrt julienrbrt enabled auto-merge February 3, 2026 19:50
@julienrbrt julienrbrt added this pull request to the merge queue Feb 3, 2026
Merged via the queue into main with commit 432d5a7 Feb 3, 2026
28 checks passed
@julienrbrt julienrbrt deleted the cian/compat-test branch February 3, 2026 20:26
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] Sync Compatibility Test

2 participants