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

Separate election from ticket generation (Part of #2225) #3287

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

ZenGround0
Copy link
Contributor

@ZenGround0 ZenGround0 commented Aug 23, 2019

Why do we need this PR

Right now ticket generation and election proof generation are one and the same. To get closer to the planned consensus construction we need to separate those things out. This PR does so while at the same time tying the ticket and election proof operations to objects to organize functionality together.

  • TicketMachine and ElectionMachine refactor
  • Refactor consensus.Expected and mining.Worker to use these dependencies instead of consensus free functions
  • Fix mining.Worker to use separate ticket generation and election proving operations

Non-goals

@ZenGround0 ZenGround0 force-pushed the feat/2222 branch 3 times, most recently from 1f57cf7 to b1a6c4b Compare August 29, 2019 19:11
@ZenGround0 ZenGround0 changed the base branch from feat/2222 to master August 29, 2019 22:40
@ZenGround0 ZenGround0 force-pushed the refactor/ticket branch 4 times, most recently from 1a9317b to 4c3257b Compare September 11, 2019 02:00
@ZenGround0
Copy link
Contributor Author

@anorth @frrist I've put in effort to make this PR's commit history helpful for review. It is best reviewed one commit at a time.

  • The first commit introduces the fundamental refactor that puts election proof and ticket generation and validation on separate objects
  • The second commit is a prefactor that turned out to be necessary in order to maintain existing structure. It imperfectly adds a call to determine the miner worker address to the power table. While this is not very clean it only messes with an abstraction that is arguably not that clean already (the power table).
  • The third commit integrates the new election and ticket validation logic into the consensus validation rules checking
  • The fourth commit integrates this logic into the mining worker. It removes the dependency-injection doSomeWorkFunc because now these dependencies can be injected using the ticket generation interface. It also adds a clock as a worker dependency so that sleep calls here can be made using a system aware clock.
  • The fifth commit integrates all of the above into the node. A lot of work went into getting node tests to pass. Mocking out fake tickets and election proofs was challenging enough that I opted to actually mine winning blocks in process to use for the tests. We'll need something better soon.

consensus/election.go Outdated Show resolved Hide resolved
consensus/election.go Outdated Show resolved Hide resolved
consensus/election.go Outdated Show resolved Hide resolved
consensus/election.go Outdated Show resolved Hide resolved
consensus/election.go Outdated Show resolved Hide resolved
log.Debugf("Mining on tipset: %s, with %d null blocks.", base.String(), nullBlkCount)
if ctx.Err() != nil {
log.Warningf("Worker.Mine returning with ctx error %s", ctx.Err().Error())
return false
}

challenge, err := consensus.CreateChallengeSeed(base, uint64(nullBlkCount))
// Read uncached worker address
workerAddr, err := w.api.MinerGetWorkerAddress(ctx, w.minerAddr)
Copy link
Member

Choose a reason for hiding this comment

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

This is one of those calls that should be clearly parameterised by the tipset keying the state in which the address is looked up, #3022. Can you make that change in a prefactor?

Copy link
Contributor Author

@ZenGround0 ZenGround0 Sep 11, 2019

Choose a reason for hiding this comment

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

#3401 addresses this. That may merge first anyway, but I think this PR can go in before that if this one is ready to go first as the race #3401 prevents is rare and is unlikely to cause test flakiness / bad user behavior in a short amount of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging #3401 is proving tricky. Deferring to an immediate follow on in favor of getting this in for 0.5 staging.

mining/worker.go Outdated Show resolved Hide resolved
mining/worker_test.go Outdated Show resolved Hide resolved
node/block_propagate_test.go Show resolved Hide resolved
wallet/wallet_test.go Outdated Show resolved Hide resolved
@anorth
Copy link
Member

anorth commented Sep 11, 2019

My feedback is mostly minor: great work. The opaque hex strings are perhaps my biggest problem.

The factoring here definitely needs some work so that we can write much more thorough tests, but well done on improving the situation.

@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

Merging #3287 into master will increase coverage by <1%.
The diff coverage is 65%.

@@           Coverage Diff           @@
##           master   #3287    +/-   ##
=======================================
+ Coverage      47%     47%   +<1%     
=======================================
  Files         229     230     +1     
  Lines       14254   14305    +51     
=======================================
+ Hits         6751    6850    +99     
+ Misses       6513    6444    -69     
- Partials      990    1011    +21

@ZenGround0 ZenGround0 mentioned this pull request Sep 11, 2019
@ZenGround0 ZenGround0 force-pushed the refactor/ticket branch 2 times, most recently from b5a692c to 1f02fd7 Compare September 11, 2019 22:33
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

This looks great @ZenGround0!

consensus/election.go Show resolved Hide resolved
consensus/election.go Show resolved Hide resolved
consensus/election.go Show resolved Hide resolved
consensus/election.go Show resolved Hide resolved
consensus/election.go Show resolved Hide resolved
mining/block_generate.go Outdated Show resolved Hide resolved
mining/worker.go Outdated Show resolved Hide resolved
mining/worker.go Outdated Show resolved Hide resolved
mining/worker.go Outdated Show resolved Hide resolved
node/block_propagate_test.go Outdated Show resolved Hide resolved
- Include worker address query in power table view
- Integrate elections into block validation
- Integrate elections into block mining
- Integrate updated components into node
@ZenGround0 ZenGround0 merged commit 4e737f6 into master Sep 12, 2019
@ZenGround0 ZenGround0 deleted the refactor/ticket branch October 29, 2019 14:44
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