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

Refuse to start if binary network doesn't match chain node network #340

Open
magik6k opened this issue Dec 6, 2024 · 5 comments · May be fixed by #345
Open

Refuse to start if binary network doesn't match chain node network #340

magik6k opened this issue Dec 6, 2024 · 5 comments · May be fixed by #345
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@magik6k
Copy link
Collaborator

magik6k commented Dec 6, 2024

Basically on startup detect if the curio binary is e.g. calib and the chain node is mainnet - will prevent from deploying the wrong binary to a wrong environment (like I may have just done 😅)

@magik6k magik6k added good first issue Good for newcomers help wanted Extra attention is needed labels Dec 6, 2024
@Winter-Soren
Copy link

I would love to contribute and resolve this issue, please assign it to me.

@LexLuthr
Copy link
Contributor

LexLuthr commented Dec 7, 2024

Please feel free to ask any questions you have.

@Winter-Soren
Copy link

Hi @LexLuthr ,
Add network validation checks that prevent Curio from starting if the binary network doesn't match the chain node network.

I've implemented this by:

  1. I have added network constants and validation function in build/params.go:
+ const (
+     NetworkMainnet    = "mainnet"
+     NetworkCalibnet   = "calibnet"
+     NetworkTestnet    = "testnet"
+ )
+ 
+ func CurrentNetwork() string {
+     switch BuildType {
+     case BuildMainnet:
+         return NetworkMainnet
+     case BuildCalibnet:
+         return NetworkCalibnet
+     case Build2k:
+         return NetworkTestnet
+     default:
+         return "unknown"
+     }
+ }
  1. And also added network validation in cmd/curio/run.go:
+ func validateNetworkMatch(ctx context.Context, api v0api.FullNode) error {
+     // Get network name from the chain node
+     networkName, err := api.StateNetworkName(ctx)
+     if err != nil {
+         return xerrors.Errorf("failed to get network name from chain node: %w", err)
+     }
+ 
+     // Get binary's network
+     binaryNetwork := build.CurrentNetwork()
+ 
+     // Compare networks
+     if string(networkName) != binaryNetwork {
+         return xerrors.Errorf("network mismatch: binary built for %s but chain node is on %s", 
+             binaryNetwork, networkName)
+     }
+ 
+     return nil
+ }

  // I have added some validation check in run command
+ api, closer, err := lcli.GetFullNodeAPI(cctx)
+ if err != nil {
+     return err
+ }
+ defer closer()
+ 
+ if err := validateNetworkMatch(ctx, api); err != nil {
+     return xerrors.Errorf("network validation failed: %w", err)
+ }

The validation happens early in the startup process and will prevent Curio from starting if there's a network mismatch. The error message indicates which network the binary was built for and which network the chain node is on.

The validation happens early in the startup process before any services are started. The error message will indicate the mismatch (e.g., "network mismatch: binary built for mainnet but chain node is on calibnet").

Could you please review this approach and let me know if:

  1. This is the right direction for implementing the network validation.
  2. The placement of the check in the startup sequence is appropriate.
  3. Is there anything left unchecked?

Thank you!

@LexLuthr
Copy link
Contributor

  1. You don't need a separate function. One already exists build.BuildTypeString()
  2. Network validation should not happen in the run command itself. It should happen in func GetFullNodeAPIV1Curio(). Each available chain node should be checked for version and any mismatch should be rejected. If we are left with none after removing incorrect ones, we should not proceed. Otherwise an alert should be raised about network mismatch.

@Winter-Soren
Copy link

I've implemented the feedback :

  1. Removed the separate CurrentNetwork() function and now using the existing build.BuildTypeString() as suggested
  2. Moved the network validation from run.go to GetFullNodeAPIV1Curio() where it:
    • Validates each chain node's network
    • Filters out incompatible nodes
    • Only proceeds if at least one compatible node is found
    • Provides clear logging about network mismatches

The validation now happens early in the connection process, providing better error handling and reporting when network mismatches occur.

Could you lemme know if there's any adjustments required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants