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

Fixing dynamic re-work and trend from elections #1968

Merged
merged 24 commits into from
May 13, 2019

Conversation

guilhermelawless
Copy link
Contributor

@guilhermelawless guilhermelawless commented May 8, 2019

Closes #1963 . It's been cherry picked in this PR.

Discussion

  • If we have more use cases in the future it might be best to abstract the difficulty arithmetic into its own class.
  • 100% of the uses of nano::difficulty::to/from_multiplier use the network threshold as the second argument. Where could we provide these methods using a default for that argument?

Explanation of PR

  • Adds difficulty manipulation (to/from multiplier) methods under nano::difficulty namespace in lib/numbers
  • Adds tests for above
  • Removes some overflow checks since we're now in multiplier space (read below)
  • Fixes arithmetic in rework / active difficulty trend
  • Fixes a bug where the work watcher would not update the block upon doing rework
  • Sets a new work value for test_genesis_data since the previous one had a very high difficulty, too much for the test net.

Difficulty arithmetic is a pain, it must be done under the "multiplier space", that is, must first transform the difficulty to a multiplier, and only then we're able to perform sums/divisions/etc.

To get a difficulty into multiplier space: multiplier = ((1<<64) - base_difficulty) / ((1<<64) - difficulty). In C++, (1<<64) - value is the same as (-value) if value is unsigned int 64 by abusing underflow.

Note that the difficulty being in the denominator is what makes it not trivial to perform arithmetic in the value space. The expression (a/x + a/y + a/z) cannot be simplified further.

Once in multiplier space, we can perform averages / trend analysis (like active difficulty trend), and at the end go back to a difficulty by inverting the previous operation. Precision loss in going back and forth is minimal and irrelevant in most (all?) use cases.

@zhyatt zhyatt requested a review from argakiig May 8, 2019 09:03
@zhyatt zhyatt added this to the V19.0 milestone May 8, 2019
Copy link
Contributor

@clemahieu clemahieu left a comment

Choose a reason for hiding this comment

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

Nice I like the concept, that'll be a lot easier to manage and the precision loss is immaterial.

@guilhermelawless guilhermelawless changed the title Fixing dynamic re-work and trend from elections WIP: Fixing dynamic re-work and trend from elections May 8, 2019
@guilhermelawless guilhermelawless changed the title WIP: Fixing dynamic re-work and trend from elections Fixing dynamic re-work and trend from elections May 9, 2019
@guilhermelawless
Copy link
Contributor Author

So fad8c68 fixed the issues observed on beta where the active difficulty was extremely high. Since the max multiplier can easily be up in the tens of thousands even while computing work at base threshold, the average was biased towards these values. Median is correct here and fixed the issue.

@zhyatt zhyatt added the functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality label May 13, 2019
@zhyatt zhyatt merged commit 41506cb into nanocurrency:master May 13, 2019
@guilhermelawless guilhermelawless deleted the rework-fix branch May 13, 2019 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants