Addressing smartnode-issue-572 #621
Conversation
This is commit 1/3 to address smartnode-issue-572. This commit includes changes to: 1) add an enable-partial-rebuild flag to the rebuild flow, 2) return error data per key from the API and 3) add logging to inform the user of the outcomes. Update recover-keys.go Update go.mod
SolezOfScience
force-pushed
the
refactor/smartnode-issue-572-rebuild-rework
branch
from
ec51995 to
9bd5510
Compare
| func (r *WalletRequester) Rebuild() (*types.ApiResponse[api.WalletRebuildData], error) { | ||
| return client.SendGetRequest[api.WalletRebuildData](r, "rebuild", "Rebuild", nil) | ||
| func (r *WalletRequester) Rebuild(enablePartialRebuild bool) (*types.ApiResponse[api.WalletRebuildData], error) { | ||
| return client.SendGetRequest[api.WalletRebuildData](r, "rebuild", "Rebuild", map[string]string{"enable-partial-rebuild": strconv.FormatBool(enablePartialRebuild)}) |
There was a problem hiding this comment.
interesting. bool types are native json types- do we need to use a string?
There was a problem hiding this comment.
I believe I was just following convention on this one. For example, the skip-validator-key-recovery parameter uses a similar code flow to translate the string to boolean.
I could see the perspective that this is a bit silly since the parameter is a string, gets converted to a bool to call the method then is immediately converted back to a string for the payload. I also cringe at the idea of passing a string around which is serving as a boolean, so neither way is elegant.
There was a problem hiding this comment.
The SendGetRequest method takes a map[string]string. Refactoring this down could make this more generic, but for now requires parameters as strings.
| // Log | ||
| fmt.Println("Rebuilding node validator keystores...") | ||
| fmt.Printf("Partial rebuild enabled: %s.\n", enablePartialRebuild.Name) |
There was a problem hiding this comment.
enablePartialRebuild.Name here is equal to enable-partial-rebuild per
Did you mean to printf the value of enablePartialRebuildValue instead?
There was a problem hiding this comment.
Thanks for pointing this out. Frankly I found the use of these oddly confusing. For example, recover.go, it checks if the name is non-empty before using it but given the definition of the flags, I don't see how it could be empty.
Regardless, I'll definitely update this log.
There was a problem hiding this comment.
Well that looks like a bug :(
There was a problem hiding this comment.
Gotcha - once you left that comment I figured that may be the case. I'll take a note.
rocketpool-daemon/common/validator/key-recovery-manager/dry-run-key-recovery-manager.go
Outdated
Show resolved
Hide resolved
rocketpool-daemon/common/validator/key-recovery-manager/key-recovery-manager.go
Outdated
Show resolved
Hide resolved
… support test and partial recovery modes. This change addresses all comments from the first draft of the PR to introduce partial recovery support. This removes multiple implementations of a KeyRecoveryManager interface and instead uses class fields to determine how to appropriately enact the recover code flow.
| } | ||
| fmt.Println("Validator Client restarted successfully.") | ||
| } else { | ||
| fmt.Println("No validator keys were found.") |
There was a problem hiding this comment.
I have a slight preference to see this moved up to L84 and the conditional inverted. An early return can prevent the need for an else block.
Hey @SolezOfScience, thanks for the PR! Are you still considering unit tests? |
|
Hey @0xfornax, sorry for the delay. A change in my professional role has left me with less bandwidth to finish this than I originally expected. The next few weeks are very busy for me between going to Pragma and ETHGlobal's hackathon and some personal life commitments. It is probably best to have the testing picked up by someone from the team. |
Overview
This is change 1/2 to address smartnode-issue-572.
This change includes a
key-recovery-managerclass which is a refactored version of the existingrecover-keys. For the time being, the old version is being kept and will be removed in a follow-up PR which migrates the remaining daemon API implementations to use the new version and support the new CLI parameters.Failure Reason Collection
The approach collects errors as it iteratively tries to recover keys. If keys have not been attempted to be processed, you won't get a publicKey -> error association. However, you will get a general error response. Once publicKeys are able to be procured, the response objects are used.
Limiting Unexpected State
Overall, this change maintains the status quo for how the API tries to manage/limit risk of unexpected state and how it's documented. At their core, these APIs will always have risk of unexpected state given the serial nature of how keys are written by the keystore manager. Without a true atomic batch-write procedure, there will always be risk of the process leaving unfinished work. This change does not intend to address this, but is acknowledged as a potential improvement in the future should it become worthwhile.
Testing
No testing has been run on this change yet. I would prefer to setup unit testing rather than try enacting manual testing. I'll be following up on this topic as the PR receives feedback.