Page MenuHomePhabricator

Add function to validate difficulty changes
ClosedPublic

Authored by PiRK on Jan 10 2024, 08:37.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCf662f94f685d: Add function to validate difficulty changes
Summary

The rule against difficulty adjustments changing by more than a factor of 4 can
be helpful for anti-DoS measures in contexts where we lack a full headers
chain, so expose this functionality separately and in the narrow case where we
only know the height, new value, and old value.

The function was adjusted to account for the changes in difficulty algorithms after the 2017 BCH fork. The EDA algorithm no longer limits the difficulty change interval to 2016 blocks but still keeps the rule that the difficulty cannot change by more than a factor 4. The DAA (CW-144) and ASERTI3D algorithms no longer have hard rules for difficulty transitions, so the function always returns true after the november 15 2017 hard fork block height.

This is a backport of core#25717
https://github.com/bitcoin/bitcoin/pull/25717/commits/1d4cfa4272cf2c8b980cc8762c1ff2220d3e8d51

Test Plan

ninja && ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Jan 10 2024, 08:37
Fabien requested changes to this revision.Jan 10 2024, 16:43
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/pow/eda.cpp
127 ↗(On Diff #44011)

Why don't you move that in the place you put the DAA check ? This is inconsistent

129 ↗(On Diff #44011)

This is different from what the source material does if height % params.DifficultyAdjustmentInterval() != 0 && old_nbits == new_nbits (which is the normal case). This will still return true but you have to do all the computation which defeats the purpose of the modulus on height

172–175 ↗(On Diff #44011)
This revision now requires changes to proceed.Jan 10 2024, 16:43

review: avoid unnecessary computation when the difficulty stays constants, move the UAHF specific check from eda.cpp to pow.cpp, simplify boolean return statement.

Fabien requested changes to this revision.Jan 11 2024, 08:35
Fabien added inline comments.
src/pow/pow.cpp
61 ↗(On Diff #44053)

This is wrong when you are recomputing the difficulty. If you got a billion blocks in the last interval, clearly using the same difficulty is wrong

70 ↗(On Diff #44053)

I still don't get it. UAHF height < DAA height, so why do you need to check both ? If UAHF is enabled, then DAA is enabled. Can't you just return true if UAHF is enabled and then just use the PermittedEDADifficultyTransition algo ?

This revision now requires changes to proceed.Jan 11 2024, 08:35
src/pow/pow.cpp
61 ↗(On Diff #44053)

Intervals are defined as a number of blocks, so you cannot have a billion blocks in them. This function really only checks that the difficulty only changes when allowed to do so, and does not change by more than a factor 4. I don't see how PermittedEDADifficultyTransition can ever return false if old_nbits == new_nbits.

70 ↗(On Diff #44053)

I don't understand what you mean. My understanding is that we need to treat the legacy algorithm (height < uahfHeight) differently from the EDA algorithm (height < daaHeight). In the first case the difficulty is not allowed to change unless height % params.DifficultyAdjustmentInterval() == 0. In both cases we can check for the factor 4 rule.

PiRK requested review of this revision.Jan 11 2024, 09:16

As per offline discussion it's worth checking for min pow on every algo

src/pow/pow.cpp
61 ↗(On Diff #44053)

yeah I meant short times in the interval. You're correct this can never return false if old_nbits == new_nbits

70 ↗(On Diff #44053)

OK the intent was to avoid that computation entirely for the interval [uahf..daa] but this solution is obviously more accurate and the computation cost might not be that terrible on second thought

move powLimit check out of PermittedEDADifficultyTransition and before IsDAAEnabled(params, height - 1), to always check for it.

Tested by rebasing D15129 and running a header sync on mainnet and testnet

Fabien requested changes to this revision.Jan 11 2024, 12:57
Fabien added inline comments.
src/pow/eda.cpp
135 ↗(On Diff #44053)

This is still relevant, you're clamping the difficulty after it's been multiplied by the largest time span and not comparing new_target directly here

153 ↗(On Diff #44053)

dito

This revision now requires changes to proceed.Jan 11 2024, 12:57

restore powLimit clamping in PermittedEDADifficultyTransition, and remove unused height argument

fix lack of verb in comment

This revision is now accepted and ready to land.Jan 11 2024, 15:16