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

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

@zhyatt zhyatt added this to the V19.0 milestone May 8, 2019

@zhyatt zhyatt added this to During RC in V19 May 8, 2019

@clemahieu
Copy link
Collaborator

left a comment

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

Show resolved Hide resolved nano/core_test/difficulty.cpp Outdated
Show resolved Hide resolved nano/lib/numbers.cpp Outdated

guilhermelawless added some commits May 8, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

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 merged commit 41506cb into nanocurrency:master May 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@guilhermelawless guilhermelawless deleted the guilhermelawless:rework-fix branch May 13, 2019

@zhyatt zhyatt moved this from During RC to RC 3 (TBD) in V19 May 13, 2019

argakiig added a commit to argakiig/raiblocks that referenced this pull request May 22, 2019

Fixing dynamic re-work and trend from elections (nanocurrency#1968)
* Add to/from work multiplier utility methods

* Use new method in RPC work_validate

* Update block in work watcher to avoid recomputing work every work watcher iteration

* Interim commit (not compiling)

* Fix adjusted difficulty, all arithmetic must be done in 'multiplier space'

* Move multiplier utility methods to lib/numbers under nano::difficulty namespace

* Check before division by zero

* Fix rework related tests

* Add difficulty manipulation methods core tests

* Simplify RPC work_validate test, difficulty manipulation methods are now tested elsewhere

* Add multiplier to RPC active_difficulty response

* Use difficulty methods in RPC active_difficulty

* Only ASSERT_DEATH if asserts enabled

* Format

* Fix adjusted difficulty (currently hanging in tests)

* Fix two tests

* Attempt debugging

* Fix type in test

* from_multiplier calculation is more clear

* Use a low value work for test_genesis_data

* Cleanup

* The median should be used, not the average. Average is biased towards the extreme one-off difficulties

* Only consider roots with ongoing elections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.