Allow the output address owner to change the output address via MsgStake #1534
Conversation
This patch allows the owner of the output address of a staked node to change the output address to another via MsgStake after the new feature key "OEDIT" is enabled. In addition to the server side change, this patch updates the behavior of the `pocket nodes stake non-custodial` command to get an MsgStake to be signed by the current output address if neither the operator nor the new output address is not available in the key base.
|
Here's a mermaid diagram ChatGPT gave me using your github description.
flowchart TD
A(BaseApp.runTx) --> B(auth.ValidateTransaction)
B --> C(BaseApp.runMsg)
C --> D(nodes.handleStake)
D --> E(keeper.ValidateValidatorStaking)
E --> F(keeper.ValidateValidatorMsgSigner)
F --> G(keeper.ValidateValidatorMsgSigner)
G --> H(keeper.ValidateEditStake)
H --> I(keeper.StakeValidator)
I --> J(keeper.EditStakeValidator)
subgraph Before
B -->|Validate using signed keys|L(MsgStake.GetSigners)
H -->|Ensure signed by current output address| M(Error)
end
subgraph After
B --> K(GetMsgStakeOutputSigner)
K -->|Append current output address|L(MsgStake.GetSigners)
H -->|Ensure signed by current output address| M(ValidateEditStake)
end
|
There was a problem hiding this comment.
A couple of minor NITS/comments, but otherwise this PR LGTM from a code perspective. Amazing test and amazingly clear code!
We need to submit a proposal to the DAO to approve this (cc @jessicadaugherty), but I don't expect any opposition from the community.
|
AI-Generated Summary: This pull request includes changes in various files related to the handling of outputs addresses, transaction signing, and validator staking. Key updates include refactoring and improvements of functions in New functions such as |
Interesting. Fortunately or unfortunately, AI made a mistake. That flow is not correct. I think it's hard to overlap before and after in the same chart. Here are two charts before and after. This matches the test scenario in the first description. Sending MsgStake without this patchflowchart TD
A(runTx) --> B(ValidateTransaction)
B -- If signed by the current output address --> K([sdk:4])
A --> C(runMsg)
C --> D(handleStake)
D --> E(ValidateValidatorStaking)
D -.-> F(StakeValidator)
E --> G(ValidateValidatorMsgSigner\nwith the current state)
G -- If signed by the new output address --> L([pos:125])
E --> H(ValidateValidatorMsgSigner\nwith the new state)
E --> I(ValidateEditStake)
I -- If signed by the operator --> M([pos:124])
F -.-> J(EditStakeValidator)
Sending MsgStake with this patchflowchart TD
A(runTx) --> B(ValidateTransaction)
B --> N(GetMsgStakeOutputSigner)
N -- If signed by the current output address --> K([ok])
A --> C(runMsg)
C --> D(handleStake)
D --> E(ValidateValidatorStaking)
D --> F(StakeValidator)
E --> O{Valid output address edit?}
O -. No .-> G(ValidateValidatorMsgSigner\nwith the current state)
G -.-> L([pos:125])
E --> H(ValidateValidatorMsgSigner\nwith the new state)
E --> I(ValidateEditStake)
I -- If signed by the operator --> M([pos:127])
F --> J(EditStakeValidator)
J --> P([Happy ending!])
|
|
AI-Generated Summary: This pull request introduces updates across several files, making significant changes to the staking and validation logic. Key changes include adding new functions and conditions to accommodate various upgrades, enhanced validation, and better handling of different block heights. Some refactoring has been done for improved code maintainability and robustness, particularly in the |
|
@msmania Wanted to encourage you to submit another PR updating the documentation so these awesome diagrams don't go to waste! |
This patch allows the owner of the output address of a staked node to change the output address to another via the existing stake transaction. The same idea was proposed as #1499.
Rationale:
This feature gives users more flexibility. One of the possible use cases is that a node runner can convert custodial stake nodes into non-custodial stake nodes without unstaking. Or a current non-custodial stake user may want to change the output address some time after they stake nodes.
Technical details:
Below are the key functions in the transaction processing flow. This patch makes a new
MsgStaketransaction to change the output address pass through all Validate* functions and lets the final functionEditStakeValidatoraccept the change in the world state if the new feature key “OEDIT” is enabled.The challenge was in
ValidateTransaction, which expects a transaction message to be signed by a key which appears in the transaction itself. For example,MsgStakemust be signed byMsgStake.PublicKeyorMsgStake.Output. Since we intend to allow only the current output address owner to change the output address, however, a newMsgStaketransaction must be signed by the current output address, and it doesn’t appear in the transaction because a new output address is there. This patch needs to change that signer restriction rule.Each message type implements
GetSignersthat returns a slice ofAddress. When a transaction arrives atValidateTransaction, it callsGetSignersand checks if a transaction is signed by any of the addresses returned fromGetSigners. If not, the transaction is rejected withCodeUnauthorized. The challenge here is thatValidateTransactionis in the auth package that doesn’t know aboutMsgStake. One possible solution is to makeMsgStake.GetSignersreturn three addresses,MsgStake.PublicKey,MsgStake.Output, and the current output address. However, implementing it is not straightforward becauseMsgStakeis in the types package which doesn’t have direct access to the keeper package that is required to get the current output address.The proposed solution in this patch is to introduce a new method
GetMsgStakeOutputSignerin the keeper package andValidateTransactioncalls it to append the current output address to the slice returned fromGetSigners.The changes in valStateChanges.go are straightforward.
ValidateValidatorMsgSignersimply checks if a signer is either the node’s operator or output address. We need to skip the call toValidateValidatorStakingwith the new validator because the signer does not appear in the new validator. The final validate functionValidateEditStakeensures thatMsgStaketo change the output address must be signed by the current output address. Otherwise it returns a new errorErrDisallowedOutputAddressChange.After all validate functions,
EditStakeValidatoraccepts a new output address.Testing: