Skip to content

Comments

make public import to support SOV L1s#2746

Merged
felipemadero merged 8 commits intomainfrom
improve-public-import
May 14, 2025
Merged

make public import to support SOV L1s#2746
felipemadero merged 8 commits intomainfrom
improve-public-import

Conversation

@felipemadero
Copy link
Collaborator

@felipemadero felipemadero commented Apr 16, 2025

Why this should be merged

Closes #2704

Currently, avalanche blockchain import public only supports importing non SOV blockchains.
As the trend is to only support L1, a change is needed here.

This also fixes avalanche blockchain validators so as to correctly tell is the validator is SOV or NOSOV

This also removes local network version of helper to get subnet validators, changing also avalanche blockchain validators part related to this.

How this works

How this was tested

How is this documented

@felipemadero felipemadero requested a review from Tonix517 April 25, 2025 12:50
"the blockchain ID",
)
cmd.Flags().StringVar(&rpcURL, "rpc", "", "rpc endpoint for the blockchain")
cmd.Flags().BoolVar(&noRPCAvailable, "no-rpc-available", false, "use this when an RPC if offline and can't be accesed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that noRPCAvailable flag needs to be set to true so that importBlockchain can:

if rpcIsAvailable {
			sc.ValidatorManagement, err = validatorManagerSDK.GetValidatorManagerType(rpcURL, common.HexToAddress(validatorManagerAddress))
			if err != nil {
				return models.Sidecar{}, nil, fmt.Errorf("could not obtain validator manager type: %w", err)
			}
			printFunc("  Validation Kind: %s", sc.ValidatorManagement)
			if sc.ValidatorManagement == validatormanagertypes.ProofOfAuthority {
				owner, err := contract.GetContractOwner(rpcURL, common.HexToAddress(validatorManagerAddress))
				if err != nil {
					return models.Sidecar{}, nil, err
				}
				sc.ValidatorManagerOwner = owner.String()
				printFunc("  Validator Manager Owner: %s", sc.ValidatorManagerOwner)
			}
		}

and

if rpcIsAvailable {
			blockchainID, _ = precompiles.WarpPrecompileGetBlockchainID(rpcURL)
		}
		if blockchainID == ids.Empty {
			blockchainID, err = app.Prompt.CaptureID("What is the Blockchain ID?")
			if err != nil {
				return models.Sidecar{}, nil, err
			}
		}

Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we just remove this flag and:
return error if rpc is unavailable when:

sc.ValidatorManagement, err = validatorManagerSDK.GetValidatorManagerType(rpcURL, common.HexToAddress(validatorManagerAddress)

since we need to know if poa/ pos anyways

and if

blockchainID, _ = precompiles.WarpPrecompileGetBlockchainID(rpcURL)
		}

returns error due to url being unavailable we can just prompt for the blockchainID

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR: I think we should remove this flag

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are cases where a rpc in unavailable because all rpc nodes are down but you anyway want to be
able to partially import the L1, most notably to increase validator balances, or to try out some local tests, or for any
misc debugging reason like be able to easily have access to certain L1 info.
I indeed used this functionality twice in the past month

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eg if all your validator drawn the balance, you need to use increase balance, and a reasonable flow would be to
call first import on the rpc, and then increase the balances

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using flag why not we just make a call to this RPC, and if this RPC is not returning any response then we know that it is not available (don't return the error) and respond accordingly as if we have the flag set as true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it can be the case that there is a temporal network failure and the import from a failed rpc will be assumed to be successful, being not the case, also, the idea of this flag is to even avoid asking for the url instead of requiring the user to provide
a fake url.
not opposing to this but the flag, being kind of a bit more ugly, seems a better option to me.
as you wish

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or we can just accept any URL string at prompt, and tell the user which to use for empty rpc, eg just enter

@sukantoraymond
Copy link
Contributor

TLDR: I think we want to avoid adding more flags into our commands. If possible we should replace / reuse our existing flags.

@felipemadero felipemadero merged commit 041e402 into main May 14, 2025
68 of 69 checks passed
@github-project-automation github-project-automation bot moved this from Backlog 🧊 to Done 🎉 in avalanchego May 14, 2025
@felipemadero felipemadero deleted the improve-public-import branch May 14, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Add SOV support to blockchain import

3 participants