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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Oct 14 2017, 09:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 14 2017, 09:39

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

Mengerian accepted this revision as: Mengerian.Oct 14 2017, 22:01

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.
deadalnix updated this revision to Diff 1563.Oct 16 2017, 13:39

Fix grammar

deadalnix updated this revision to Diff 1564.Oct 16 2017, 13:41

Fix more grammar

deadalnix updated this revision to Diff 1565.Oct 16 2017, 14:31

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

kyuupichan added inline comments.Oct 17 2017, 02:46
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.

schancel added inline comments.Oct 29 2017, 22:27
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

deadalnix added inline comments.Oct 30 2017, 16:33
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.

deadalnix updated this revision to Diff 1614.Oct 30 2017, 16:35

Update comments

schancel accepted this revision.Oct 30 2017, 16:37

Looks good.

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