-
Notifications
You must be signed in to change notification settings - Fork 86
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
Support PoA ACP99 + Externals EVM signatures for validator setup txs #2650
base: main
Are you sure you want to change the base?
Conversation
Excited about this! Going to be testing it today |
Was able to successfully get a changeWeight end to end adjustment completed today using this feature. Much appreciated! I'll try adding a validator (and removing one too probably) on Monday. The main tweak that would be helpful for us is if the complete step could be automatically called using a stored-key, rather than outputting the call data and access list. since those methods are not onlyOwner, anyone (including stored-keys) should be able to call them, saving us a manual step :) |
@@ -403,6 +410,7 @@ func InitializeValidatorManager( | |||
aggregatorAllowPrivatePeers, | |||
aggregatorLogger, | |||
validatorManagerAddrStr, | |||
useACP99, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With ACP 99, we no longer have to differentiate initializing validator manager for poa and pos. We can just initialize validator manager and if user wants to do pos, we can do additional steps after.
Example implementation:
avalanche-cli/cmd/blockchaincmd/convert.go
Line 374 in f0a6d96
// both POA and POS have to do this step first now |
@@ -186,6 +216,7 @@ func SetupPoA( | |||
aggregatorAllowPrivatePeers bool, | |||
aggregatorLogger logging.Logger, | |||
validatorManagerAddressStr string, | |||
useACP99 bool, | |||
) error { | |||
return subnet.InitializeProofOfAuthority( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't have to be InitializeProofOfAuthority anymore. Can just be initialize validator manager
Don't think we need to support both ACP 99 and validator manager 1.0.0 contracts. Supporting both adds additional complexity esp to our sdk. Plus once ACP 99 is released, don't think there's a need for users to use validator manager 1.0.0 contracts. Deferring to @learyce |
cmd.Flags().Uint64Var(&weight, validatorWeightFlag, uint64(constants.DefaultStakeWeight), "set the weight of the validator") | ||
cmd.Flags().StringVar(&validatorManagerOwner, "validator-manager-owner", "", "force using this address to issue transactions to the validator manager") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we technically dont even need to get validator manager using flag anymore. Currently we only get validator manager from on chain if user doesn't provide argument to add validator. But I think its better if we just do this for all cases -> meaning even if user provides argument to add validator. This means that we don't even need validator-manager-owner
flag anymore in add_validator, same goes for remove_validator
and change_weight
@@ -66,6 +66,8 @@ these prompts by providing the values with flags.`, | |||
cmd.Flags().BoolVar(&aggregatorLogToStdout, "aggregator-log-to-stdout", false, "use stdout for signature aggregator logs") | |||
cmd.Flags().Uint64Var(&uptimeSec, "uptime", 0, "validator's uptime in seconds. If not provided, it will be automatically calculated") | |||
cmd.Flags().BoolVar(&force, "force", false, "force validator removal even if it's not getting rewarded") | |||
cmd.Flags().BoolVar(&externalValidatorManagerOwner, "external-validator-manager-owner", false, "validator manager owner is external, make hex dump of ech evm transactions, so they can be signed in a separate flow") | |||
cmd.Flags().StringVar(&validatorManagerOwner, "validator-manager-owner", "", "force using this address to issue transactions to the validator manager") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here, dont think we need validator manager flag anymore, like in add_validator
@@ -47,6 +59,14 @@ The L1 has to be a Proof of Authority L1.`, | |||
cmd.Flags().StringVar(&nodeEndpoint, "node-endpoint", "", "gather node id/bls from publicly available avalanchego apis on the given endpoint") | |||
cmd.Flags().BoolVarP(&useLedger, "ledger", "g", false, "use ledger instead of key (always true on mainnet, defaults to false on fuji/devnet)") | |||
cmd.Flags().StringSliceVar(&ledgerAddresses, "ledger-addrs", []string{}, "use the given ledger addresses") | |||
cmd.Flags().BoolVar(&externalValidatorManagerOwner, "external-validator-manager-owner", false, "validator manager owner is external, make hex dump of ech evm transactions, so they can be signed in a separate flow") | |||
cmd.Flags().StringVar(&validatorManagerOwner, "validator-manager-owner", "", "force using this address to issue transactions to the validator manager") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment on no need for validator-manager-owner
flag like in add_validator
var txOpts *bind.TransactOpts | ||
if generateRawTxOnly { | ||
txOpts = &bind.TransactOpts{ | ||
From: from, | ||
Signer: idempotentSigner, | ||
NoSend: true, | ||
} | ||
} else { | ||
txOpts, err = evm.GetTxOptsWithSigner(client, privateKey) | ||
if err != nil { | ||
return nil, nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice one!
@@ -23,25 +23,46 @@ func PoAValidatorManagerInitialize( | |||
privateKey string, | |||
subnetID ids.ID, | |||
ownerAddress common.Address, | |||
useACP99 bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to differentiate PoAValidatorManagerInitialize and PoSValidatorManagerInitialize anymore. We can just have one function InitializeValidatorManager and have it in root.go / some common file
validatormanager.AddValidatorMessagesContractToAllocations(params.initialTokenAllocation) | ||
validatormanager.AddPoAValidatorManagerContractToAllocations(params.initialTokenAllocation) | ||
if useACP99 { | ||
validatormanager.AddValidatorMessagesACP99ContractToAllocations(params.initialTokenAllocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deployedValidatorMessagesACP99Bytecode has the same value as validator manager 1.0.0 btw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so can just reuse AddValidatorMessagesContractToAllocations
if from == (common.Address{}) { | ||
from = crypto.PubkeyToAddress(privateKey.PublicKey) | ||
} | ||
gasFeeCap, gasTipCap, nonce, err := CalculateTxParams(client, from.Hex()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't foresee from
being an empty address, especially if we are getting "validator manager owner" address on chain anyways. This means that we don't need from
in argument and we can just use crypto.PubkeyToAddress(privateKey.PublicKey)
func idempotentSigner( | ||
_ common.Address, | ||
tx *types.Transaction, | ||
) (*types.Transaction, error) { | ||
return tx, nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dont we name this emptySigner
if privateKeyStr != "" { | ||
privateKey, err = crypto.HexToECDSA(privateKeyStr) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
address := crypto.PubkeyToAddress(privateKey.PublicKey) | ||
gasFeeCap, gasTipCap, nonce, err := CalculateTxParams(client, address.Hex()) | ||
if from == (common.Address{}) { | ||
from = crypto.PubkeyToAddress(privateKey.PublicKey) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that either privateKeyStr == "" or from == "", maybe we can just do if else here to prevent unintended overriding of from
values
client ethclient.Client, | ||
generateRawTxOnly bool, | ||
from common.Address, | ||
privateKeyStr string, | ||
warpMessage *avalancheWarp.Message, | ||
contract common.Address, | ||
callData []byte, | ||
value *big.Int, | ||
) (*types.Transaction, error) { | ||
const defaultGasLimit = 2_000_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a check here where either privateKeyStr != "" or from != "" and both can't be empty and both can't be not empty also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for txtomethod
cmd.Flags().Uint64Var(&weight, validatorWeightFlag, uint64(constants.DefaultStakeWeight), "set the weight of the validator") | ||
cmd.Flags().StringVar(&validatorManagerOwner, "validator-manager-owner", "", "force using this address to issue transactions to the validator manager") | ||
cmd.Flags().BoolVar(&externalValidatorManagerOwner, "external-validator-manager-owner", false, "validator manager owner is external, make hex dump of ech evm transactions, so they can be signed in a separate flow") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unclear what external-validator-manager-owner
means, I think we can just rename this to generate-raw-tx-only
, with desc as "set this value to true when signing validator manager tx outside of cli" or sth similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or i think sign-externally
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and say in description for multisig or ledger
Why this should be merged
ACP99 is needed soon. This adds support for ACP99 PoA while keeping support for v1.0.0
There are users which have external signature processes for evm txs. Eg mutlisig.
The PR enable dumping the txs so can be separately signed, for adding , removing, and changing
weight.
How this works
How this was tested
How is this documented