Closed msmania closed 1 year ago
@msmania
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
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 x/nodes/keeper/valStateChanges.go
, adding test functions and helper functions for handling stakes, introducing a new GetMsgStakeOutputSigner
function to the Keeper struct, and modifying the ValidateTransaction
function in x/auth/ante.go
. Additionally, changes to the nodes module store and improved connections between account keeping and node modules have been made.
New functions such as fundAccount
, IsAfterOutputAddressEditorUpgrade
, and methods related to the PosKeeper
interface have also been introduced. Error handling has been updated with the addition of CodeDisallowedOutputAddressEdit
and its corresponding error function. Finally, various code quality improvements have been made, such as rearranging import statements, providing extended descriptions for certain fields, and adding new utility functions in the app/cmd/cli/txUtil.go
file. Overall, these changes mainly affect output address handling, transaction signing, and improving code readability and robustness.
@msmania
Here's a mermaid diagram ChatGPT gave me using your github description.
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.
flowchart 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)
flowchart 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 StakeNode
function. Additionally, a new interface PosKeeper
, a corresponding implementation in the Keeper struct, and an app/app.go
update to set the POSKeeper property have been introduced. Lastly, import statements have been reorganized throughout various files, and new test functions and utilities have been added for more comprehensive testing.
@msmania Wanted to encourage you to submit another PR updating the documentation so these awesome diagrams don't go to waste!
@msmania Wanted to encourage you to submit another PR updating the documentation so these awesome diagrams don't go to waste!
Created #1544
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
MsgStake
transaction to change the output address pass through all Validate* functions and lets the final functionEditStakeValidator
accept 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,MsgStake
must be signed byMsgStake.PublicKey
orMsgStake.Output
. Since we intend to allow only the current output address owner to change the output address, however, a newMsgStake
transaction 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
GetSigners
that returns a slice ofAddress
. When a transaction arrives atValidateTransaction
, it callsGetSigners
and checks if a transaction is signed by any of the addresses returned fromGetSigners
. If not, the transaction is rejected withCodeUnauthorized
. The challenge here is thatValidateTransaction
is in the auth package that doesn’t know aboutMsgStake
. One possible solution is to makeMsgStake.GetSigners
return three addresses,MsgStake.PublicKey
,MsgStake.Output
, and the current output address. However, implementing it is not straightforward becauseMsgStake
is 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
GetMsgStakeOutputSigner
in the keeper package andValidateTransaction
calls it to append the current output address to the slice returned fromGetSigners
.The changes in valStateChanges.go are straightforward.
ValidateValidatorMsgSigner
simply checks if a signer is either the node’s operator or output address. We need to skip the call toValidateValidatorStaking
with the new validator because the signer does not appear in the new validator. The final validate functionValidateEditStake
ensures thatMsgStake
to change the output address must be signed by the current output address. Otherwise it returns a new errorErrDisallowedOutputAddressChange
.After all validate functions,
EditStakeValidator
accepts a new output address.Testing: