e2e: refactor initialization with single node logic #1963
Conversation
|
|
||
| func InitSingleNode(chainId, dataDir string, existingGenesisDir string, nodeConfig *NodeConfig, votingPeriod time.Duration, trustHeight int64, trustHash string, stateSyncRPCServers []string) (*Node, error) { | ||
| if nodeConfig.IsValidator { | ||
| return nil, errors.New("creating individual validator nodes after starting up chain is not currently supported") |
There was a problem hiding this comment.
Should we make an issue for this?
There was a problem hiding this comment.
I think we can address this when there is a need for it in the future. Currently, for testing state sync we should be able to use a regular full node instead of a validator.
I don't think there's is a current use case for us to have the validator nodes
| require.NoError(t, err) | ||
|
|
||
| actualNode, err := initialization.InitSingleNode(existingChain.ChainMeta.Id, dataDir, filepath.Join(existingChain.Nodes[0].ConfigDir, "config", "genesis.json"), expectedConfig, time.Second*3, 3, "testHash", []string{"some server"}) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Do you think we should also check that the actual genesis.json file itself is the same between the single node and a randomly selected node from the chain, or do you think this is a good enough check already?
There was a problem hiding this comment.
I think this would be necessary to have if this was the actual system but since we are "testing a test", what we have now is sufficient IMO.
The goal for this test was to make sure the expected initialization files are created because I was finding myself doing that manually a lot while refactoring
| existingChain, err := initialization.InitChain(id, dataDir, existingChainNodeConfigs, time.Second*3, forkHeight) | ||
| require.NoError(t, err) | ||
|
|
||
| actualNode, err := initialization.InitSingleNode(existingChain.ChainMeta.Id, dataDir, filepath.Join(existingChain.Nodes[0].ConfigDir, "config", "genesis.json"), expectedConfig, time.Second*3, 3, "testHash", []string{"some server"}) |
There was a problem hiding this comment.
I assume the next step here is automating pulling a specified height hash from one of the validator's RPCs? Shouldn't we have enough here already to start filling in the RPC server address? IMO we should just fill this in with a single RPC twice ("1.23.45.67:26657,1.23.45.67:26657"), just makes it simpler to debug. Unless you feel we should be testing with two separate RPCs
There was a problem hiding this comment.
You are right, the next step would be to start actually utilizing this initialization logic for spinning up a state-synching full node. I don't have that completed yet. Before I continue with this, there is one more large refactoring PR to merge.
And thanks for the advice, I will make sure to use the same one twice
* e2e: refactor initialization with single node logic * lint and remove redundant getters from node * restore accidental change * rename init to InitChain * persistentPeers typo
ValarDragon
added
A:backport/v10.x
* e2e: refactor initialization with single node logic * lint and remove redundant getters from node * restore accidental change * rename init to InitChain * persistentPeers typo (cherry picked from commit 1da14b8) # Conflicts: # tests/e2e/e2e_setup_test.go # tests/e2e/e2e_test.go # tests/e2e/e2e_util_test.go # tests/e2e/initialization/init.go # tests/e2e/initialization/init_test.go # tests/e2e/initialization/node/main.go
Part of: #1879
What is the purpose of the change
Building up on #1898
This refactor separates the initialization logic into 2 parts:
initialization.InitChain()- for spinning up a chain with several validatorsinitialization.InitSingleNode()- for spinning up a single node (e.g. for testing state-sync)The documentation for this is already merged and available here: https://github.com/osmosis-labs/osmosis/tree/main/tests/e2e/initialization
Testing and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no