Skip to content
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

Add commands to change the worker key #4513

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Conversation

Stebalien
Copy link
Member

No description provided.

var actorConfirmChangeWorker = &cli.Command{
Name: "confirm-change-worker",
Usage: "Confirm a worker address change",
ArgsUsage: "[address]",
Copy link
Member Author

Choose a reason for hiding this comment

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

I could drop this flag, but it's nice to check twice.

cmd/lotus-storage-miner/actor.go Show resolved Hide resolved
Comment on lines 821 to 834
if !cctx.Bool("really-do-it") {
fmt.Println("Pass --really-do-it to actually execute this action")
return nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not necessary for this command?

Comment on lines -194 to -197
worker, err := api.StateAccountKey(ctx, mi.Worker, types.EmptyTSK)
if err != nil {
return nil, err
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I assume we just needed to do this to perform the sanity checks inside the miner. I'm now only resolving the worker key there: https://github.com/filecoin-project/lotus/pull/4513/files#diff-e5e6c18c093f7eb4f4d0bb2e19388a831f34ded5ceeff4737091f6dc686aafe6R181-R184

storage/wdpost_run.go Show resolved Hide resolved
@Stebalien
Copy link
Member Author

Looks like real test failures, still need to figure out what I broke...

@Stebalien Stebalien force-pushed the steb/feat-change-worker-key branch 2 times, most recently from b1d4653 to fed47cd Compare October 21, 2020 05:13
@Stebalien
Copy link
Member Author

Ok... fixed.

@Stebalien Stebalien force-pushed the steb/feat-change-worker-key branch 2 times, most recently from f6b0de1 to 43eb06f Compare October 21, 2020 23:15
@Stebalien Stebalien marked this pull request as draft October 21, 2020 23:15
@Stebalien Stebalien force-pushed the steb/feat-change-worker-key branch 4 times, most recently from b86d633 to 9ea309a Compare October 22, 2020 00:24
@Stebalien Stebalien marked this pull request as ready for review October 22, 2020 01:23
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Looks like this may even work, would be really good to test on some devnet / in pond first

Just 1.5 nits

cmd/lotus-storage-miner/actor.go Outdated Show resolved Hide resolved
cmd/lotus-storage-miner/actor.go Outdated Show resolved Hide resolved
api/test/deals.go Outdated Show resolved Hide resolved
@Stebalien Stebalien force-pushed the steb/feat-change-worker-key branch 2 times, most recently from 8f560e0 to d244d96 Compare October 23, 2020 18:15
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM

@Stebalien
Copy link
Member Author

Fixed and rebased.

if s.worker != worker {
s.worker = worker
msg.From = s.worker
return xerrors.Errorf("error getting miner info: %w", err)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

My original patch didn't bother caching (see above comment) but this was added in by a subsequent commit on master. I can add this back if desired, but it seems unnecessary and storing the key seems error prone.

@Stebalien Stebalien merged commit 89f6da1 into master Oct 27, 2020
@Stebalien Stebalien deleted the steb/feat-change-worker-key branch October 27, 2020 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants