Introduce `--force-winning-ticket` on `mining once` subcommand. #520

aboodman · 2018-05-30T23:48:08Z

Fixes #504

aboodman

This is an alternate implementation of #519. I greatly prefer this approach because it strengthens and reuses abstractions we already have (isWinningTicket, AsyncWorker, etc) rather than creating new ones. But it does have the downside that it pulls a tiny bit of the implementation up into the command, which we've said that we don't want to do. There isn't a good way to bridge this divide IMO.

aboodman · 2018-05-30T23:49:32Z

mining/worker.go

-	createPoST     DoSomeWorkFunc // TODO: rename createPoSTFunc?
-	mine           mineFunc
-	nullBlockTimer NullBlockTimerFunc
+	BlockGenerator  BlockGenerator


We were already exposing these configuration knobs via the NewWorker constructor - but doing it that way require you to specify each one of them, even when only a few are needed. Exposing the fields directly seems more convenient to users, and then we can remove NewWorkerWithDeps.

aboodman · 2018-05-30T23:58:25Z

mining/worker.go

-
-// NewWorkerWithDeps instantiates a new Worker with custom functions.
-func NewWorkerWithDeps(blockGenerator BlockGenerator, mine mineFunc, createPoST DoSomeWorkFunc, nullBlockTimer NullBlockTimerFunc) Worker {
+func NewWorker(blockGenerator BlockGenerator) *AsyncWorker {


It was already possible to construct and use AsyncWorker directly, because it was exported. But having NewWorker return it (pre-defaulted) makes it more convenient to use because you can then go and set just the customizations you want.

aboodman · 2018-05-30T23:59:48Z

mining/worker.go

 // Mine does the actual work. It's the implementation of worker.mine.
-func Mine(ctx context.Context, input Input, nullBlockTimer NullBlockTimerFunc, blockGenerator BlockGenerator, createPoST DoSomeWorkFunc, outCh chan<- Output) {


Nobody was using this function publicly, so I de-exported it.

aboodman · 2018-05-31T00:00:04Z

mining/worker.go

@@ -198,7 +201,6 @@ func Mine(ctx context.Context, input Input, nullBlockTimer NullBlockTimerFunc, b
 		proof := createProof(challenge, createPoST)
 		ticket := createTicket(proof)

-		// TODO: Test the interplay of isWinningTicket() and createPoST()


This was an old TODO I forgot to remove.

whyrusleeping · 2018-05-31T03:31:24Z

clarification, this causes the miner to think that the block that they mine has a winning ticket. Not for them to actually get a winning ticket. Other nodes they are connected to won't think their ticket is a winner.

dignifiedquire

Looks like a good way to go to me

acruikshank · 2018-05-31T13:26:44Z

commands/mining.go

+			forceWinningTicket = v.(bool)
+		}
+		worker := mining.NewWorker(blockGenerator)
+		if forceWinningTicket {


non-blocking: This would read a little easier and save a little code if you nested this if within the option loop above. i.e.

if forceTicket, ok := req.Options["force-winning-ticket"]; ok { if forceTicket(bool) { ... } }

or

if forceTicket, ok := req.Options["force-winning-ticket"]; ok && forceTicket(bool) { ... }

acruikshank · 2018-05-31T13:30:10Z

mining/worker_test.go

-		r = <-outCh
-	})()
+	go mine(ctx, input, nullBlockImmediately, mockBg, everyThirdWinningTicket(), func() { workCount++ }, outCh)
+	r = <-outCh


acruikshank

Looks great. I like this feature.

aboodman · 2018-05-31T14:59:50Z

Thanks, will land when tests pass. @phritz happy to circle back async.

whyrusleeping · 2018-05-31T15:47:23Z

I feel like people didn't read my comment.

whyrusleeping

blocking until we discuss the issue i brought up

aboodman · 2018-05-31T15:51:14Z

Sorry @whyrusleeping, I interpreted your comment as a statement, not a question. I agree with the statement. Do you have a specific change you want to see made?

whyrusleeping · 2018-05-31T15:56:30Z

i mean, it kinda makes this feature useless outside of the context of a single node. I think that kinda makes this less useful (and should be clearly documented at least). It also will break things pretty angrily if you restart the node, as it will (should) fail while loading the chain from disk.

aboodman · 2018-05-31T16:29:50Z

i mean, it kinda makes this feature useless outside of the context of a single node. I think that kinda makes this less useful (and should be clearly documented at least).

Agree. Are you thinking more documentation just in the flag description, or somewhere else?

It also will break things pretty angrily if you restart the node, as it will (should) fail while loading the chain from disk.

Yeah, hadn't thought about that.

===

To recap, this solution came out of the discussion here.

You suggested to make the mine always succeed. But since these tests are running the code via the CLI, there is not currently any pattern in the code to modify their behavior other than via flags.

Here are some potential ideas for how to do this better. Please let me know if you prefer one of them, or else if you want me to do something else:

We could write tests like these that require deeper modifications to core behavior as unit tests (e.g., in process, using the Go API), not integration tests. Then there would be no need for any CLI modifications.
We could come up with some other way to modify the behavior of the binary out of process, for example we could add an FILECOIN_TEST_MODE=1 env var that causes certain flags that only exist to facilitate tests to become available.
We could leave it pretty much as-is and improve the documentation/flag name.

I guess I prefer 1, personally, but I was trying to go with the flow of the existing code and what we had discussed when the issue came up.

whyrusleeping · 2018-05-31T18:25:41Z

this ones hard. Adding a flag that only works in test mode, and only works if you don't reboot your node, and only works locally, is a bit weird. I guess if we make the flag name a lot scarier, that might help? also better documentation around it? My biggest issue here (aside from the gimped functionality) is that this is a flag that we will ship the product with, unless we find some different way to accomplish the same thing in tests. shipping with a command flag that says "mine a block right now and just accept it" feels rather unprofessional, do you agree?

Choice 1 seems reasonable, but changes how we think about testing the daemon. which may be okay.

aboodman · 2018-05-31T18:39:33Z

shipping with a command flag that says "mine a block right now and just accept it" feels rather unprofessional, do you agree?

100%. I actually brought it up in original discussion -- #482 (comment) -- so I'm glad we're sorting it out.

I'll take a look and see if this can be accomplished reasonably in the unit testing style. If not we can talk about how to add a "test mode" to the binary.

dignifiedquire · 2018-06-11T11:52:26Z

@aboodman what is the state of this?

aboodman · 2018-06-11T15:23:08Z

I'll try to get to it this week, but unlikely. Probably will close it until I have time to come back.

…

On Mon, Jun 11, 2018 at 4:52 AM, Friedel Ziegelmayer < ***@***.***> wrote: @aboodman <https://github.com/aboodman> what is the state of this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE6BGCJB7mfg9fSOb8qV6geho_5MqEaks5t7ln7gaJpZM4UULdB> .

aboodman · 2018-06-22T08:11:47Z

Gonna close this for now, it doesn't seem to be causing problems.

aboodman force-pushed the fix/mine-once-b branch from 15d9e86 to 2ca2a1c Compare May 31, 2018 00:01

aboodman requested review from phritz and acruikshank May 31, 2018 00:05

aboodman commented May 31, 2018

View reviewed changes

aboodman force-pushed the fix/mine-once-b branch from 2ca2a1c to e9358be Compare May 31, 2018 00:08

aboodman mentioned this pull request May 31, 2018

Introduce --force-winning-ticket on mining once subcommand. #519

Closed

dignifiedquire approved these changes May 31, 2018

View reviewed changes

acruikshank reviewed May 31, 2018

View reviewed changes

acruikshank approved these changes May 31, 2018

View reviewed changes

aboodman force-pushed the fix/mine-once-b branch from 123aa63 to 36b923a Compare May 31, 2018 14:58

whyrusleeping suggested changes May 31, 2018

View reviewed changes

aboodman mentioned this pull request May 31, 2018

Transient test failures catchall #140

Closed

mishmosh added this to the Sprint 13 👻 milestone Jun 5, 2018

dignifiedquire modified the milestone: Sprint 13 👻 Jun 11, 2018

aboodman closed this Jun 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `--force-winning-ticket` on `mining once` subcommand. #520

Introduce `--force-winning-ticket` on `mining once` subcommand. #520

aboodman commented May 30, 2018

aboodman left a comment •

edited

Loading

aboodman May 30, 2018

aboodman May 30, 2018

aboodman May 30, 2018

aboodman May 31, 2018

whyrusleeping commented May 31, 2018

dignifiedquire left a comment

acruikshank May 31, 2018

acruikshank May 31, 2018

acruikshank left a comment

aboodman commented May 31, 2018

whyrusleeping commented May 31, 2018

whyrusleeping left a comment

aboodman commented May 31, 2018

whyrusleeping commented May 31, 2018

aboodman commented May 31, 2018

whyrusleeping commented May 31, 2018

aboodman commented May 31, 2018

dignifiedquire commented Jun 11, 2018

aboodman commented Jun 11, 2018 via email

aboodman commented Jun 22, 2018

		// Mine does the actual work. It's the implementation of worker.mine.
		func Mine(ctx context.Context, input Input, nullBlockTimer NullBlockTimerFunc, blockGenerator BlockGenerator, createPoST DoSomeWorkFunc, outCh chan<- Output) {

Introduce --force-winning-ticket on mining once subcommand. #520

Introduce --force-winning-ticket on mining once subcommand. #520

Conversation

aboodman commented May 30, 2018

aboodman left a comment • edited Loading

Choose a reason for hiding this comment

aboodman May 30, 2018

Choose a reason for hiding this comment

aboodman May 30, 2018

Choose a reason for hiding this comment

aboodman May 30, 2018

Choose a reason for hiding this comment

aboodman May 31, 2018

Choose a reason for hiding this comment

whyrusleeping commented May 31, 2018

dignifiedquire left a comment

Choose a reason for hiding this comment

acruikshank May 31, 2018

Choose a reason for hiding this comment

acruikshank May 31, 2018

Choose a reason for hiding this comment

acruikshank left a comment

Choose a reason for hiding this comment

aboodman commented May 31, 2018

whyrusleeping commented May 31, 2018

whyrusleeping left a comment

Choose a reason for hiding this comment

aboodman commented May 31, 2018

whyrusleeping commented May 31, 2018

aboodman commented May 31, 2018

whyrusleeping commented May 31, 2018

aboodman commented May 31, 2018

dignifiedquire commented Jun 11, 2018

aboodman commented Jun 11, 2018 via email

aboodman commented Jun 22, 2018

Introduce `--force-winning-ticket` on `mining once` subcommand. #520

Introduce `--force-winning-ticket` on `mining once` subcommand. #520

aboodman left a comment •

edited

Loading