Page MenuHomePhabricator

Implement simple moving average over work difficulty adjustement algorithm.
ClosedPublic

Authored by deadalnix on Oct 14 2017, 09:39.

Details

Summary

As per title. Hopefully this is much simpler than alternative, but more complex alternative do not perform suffisciently better to be worth it.

Test Plan
make check

Added unit tests for it.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
newpow
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 996
Build 996: arc lint + arc unit

Event Timeline

Unknown Object (User) added a subscriber: Unknown Object (User).Oct 14 2017, 18:10

Could someone give a quick, 3 sentence more detailed explanation of the algorithm? I read something about average of top 3 blocks?

Math and algorithm look good to me

src/pow.cpp
242 ↗(On Diff #1552)

It's not obvious to me why nHeight is compared the old DifficultyAdjustmentInterval

Maybe a short comment to explain?

244 ↗(On Diff #1552)

Redundant "suitable". Should be:

// Get the last suitable block of the difficulty interval.
254 ↗(On Diff #1552)

Rephrase as:

// Compute the target based on time and work done during the interval.

Add bounding as suggested by @Mengerian and update tests for it.

src/pow.cpp
173 ↗(On Diff #1565)

There are (patholigcal) cases where this assert could fail if MTP is not used. Since the condition is protected against anyway by the bounds you added immediately below, I suggest we remove this assert.

src/chain.h
360 ↗(On Diff #1565)

I believe the "in seconds" part is referring to the return value. It would make more sense as a second sentence. e.g. "Returned value is in seconds."

src/pow.cpp
161 ↗(On Diff #1565)

I assume there is no chance that these are from different chains due to other mechanisms?

173 ↗(On Diff #1565)

Blocks for this case should be rejected by the network. However, if they did get through, what would be the outcome of this assert triggering?

176 ↗(On Diff #1565)

144 * params.nPowerTargetSpacing would be the ideal value to find -- this caps it at twice that? If so, should we use 2 * (pindexLast->nHeight - pindexFirst->nHeight) * params.nPowerTargetSpacing ?

178 ↗(On Diff #1565)

Similar question

188 ↗(On Diff #1565)

Since this is uint, I assume -work on this class returns the two's complement? The implementation on base_uint class looks like it to me, but it's been a long time since dealing with this.

254 ↗(On Diff #1565)

This should always be the n-2 block assuming ordered timestamps?

src/pow.h
31 ↗(On Diff #1565)

adjustment

33 ↗(On Diff #1565)

Does this patch leave this function unused besides tests?

src/test/pow_tests.cpp
87 ↗(On Diff #1565)

Why is this change necessary? I don't see any changes which currently affect this.

269 ↗(On Diff #1565)

Why was >> 10 chosen?

275 ↗(On Diff #1565)

How was this value obtained?

290 ↗(On Diff #1565)

Ditto for >> 4

src/pow.cpp
173 ↗(On Diff #1565)

Node crash.

188 ↗(On Diff #1565)

Yes, -x return the 2's complement.

254 ↗(On Diff #1565)

This is n-1 most of the time.

src/pow.h
33 ↗(On Diff #1565)

It is required to verify history.

src/test/pow_tests.cpp
87 ↗(On Diff #1565)

It was invalid, but in practice doesn't change the result of the test.

269 ↗(On Diff #1565)

Just picked a very small value to check it move slowly.

275 ↗(On Diff #1565)

Run the code. It ensure that someone can test their implementation and see if they end up with the same number.

290 ↗(On Diff #1565)

Just checking a reasonable range for variation.

This revision is now accepted and ready to land.Oct 30 2017, 16:37
This revision was automatically updated to reflect the committed changes.