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

testnode doesn't support custom chain IDs #3662

Closed
rootulp opened this issue Jul 4, 2024 · 0 comments · Fixed by #3673
Closed

testnode doesn't support custom chain IDs #3662

rootulp opened this issue Jul 4, 2024 · 0 comments · Fixed by #3673
Assignees
Labels
bug Something isn't working testing items that are strictly related to adding or extending test coverage

Comments

@rootulp
Copy link
Collaborator

rootulp commented Jul 4, 2024

Context

Integration tests are failing in celestiaorg/celestia-node#3453

Problem

I discovered that there is a bug in testnode that prevents it from starting a network with a custom chain ID. Confirmed with this unit test:

package app_test

import (
	"testing"

	"github.com/celestiaorg/celestia-app/v2/test/util/testnode"
)

func Test_testnode(t *testing.T) {
	t.Run("testnode can start a network with default config", func(t *testing.T) {
		testnode.NewNetwork(t, testnode.DefaultConfig())
	})
	t.Run("testnode can start a network with a custom chain ID", func(t *testing.T) {
		config := testnode.DefaultConfig()
		config.Genesis.ChainID = "foo"
		testnode.NewNetwork(t, config)
	})
}

where the second test case fails. I think this is a regression because in v1.x the integration tests in celestia-node passed and they used testnode from celestia-app.

Proposal

  1. Fix testnode so that it can support custom chain IDs. celestia-node spins up swamp tests with the chain ID "private".
  2. Backport the fix to v2.x
  3. Cut another release candidate
  4. Bump to that release candidate in celestia-node PR
@rootulp rootulp added bug Something isn't working testing items that are strictly related to adding or extending test coverage labels Jul 4, 2024
@evan-forbes evan-forbes added the needs:discussion item needs to be discussed as a group in the next sync. if marking an item, pls be prepped to talk label Jul 8, 2024
@rootulp rootulp removed the needs:discussion item needs to be discussed as a group in the next sync. if marking an item, pls be prepped to talk label Jul 13, 2024
rootulp added a commit that referenced this issue Jul 15, 2024
Closes #3662

Before, genTx transactions were created prematurely via
[`WithValidators`](https://github.com/rootulp/celestia-app/blob/34cd4b34d99e27c5e8611abab6aaecc550499045/test/util/testnode/config.go#L123)
which was problematic if a user tried to later override the chainID via
`WithChainID` because the genTx transactions would use the default chain
ID but the state machine would be created with a custom chain ID.

This PR refactors the genTx creation to happen only when
`genesis.Export` is invoked which is _after_ a user configures a custom
chain ID.
mergify bot pushed a commit that referenced this issue Jul 16, 2024
Closes #3662

Before, genTx transactions were created prematurely via
[`WithValidators`](https://github.com/rootulp/celestia-app/blob/34cd4b34d99e27c5e8611abab6aaecc550499045/test/util/testnode/config.go#L123)
which was problematic if a user tried to later override the chainID via
`WithChainID` because the genTx transactions would use the default chain
ID but the state machine would be created with a custom chain ID.

This PR refactors the genTx creation to happen only when
`genesis.Export` is invoked which is _after_ a user configures a custom
chain ID.

(cherry picked from commit 1703fe2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing items that are strictly related to adding or extending test coverage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants