Pulse: Slightly adjust difficulty 1 step for code review

- A complete fix would involve setting the DIFFICULTY_WINDOW to
1 unified constant for both pre/post HF16 code. My attempts to get this
to work have not succeeded and so I'm putting this aside to allocate
more time on getting Pulse ready for testnet.

For completeness sake, I'll be copying verbatim the changes requested below.
Of the changes requested only adjusting the DIFFICULTY_WINDOW to 59 was
done (with the appropriate offset changes to the other constants that
interact with it).

The changes requested by jagerman
(from https://github.com/loki-project/loki-core/pull/1235)

Two points about this analysis:

1. Most of the comments here belong in the commit message rather than
code comments.

2. Well before I joined LOKI I worked with zawy for a good bit of time
on LWMA implementations, and so I found this description very confusing
because adding 1 is the correct thing to do for an LWMA. You need 61
observations to do a LWMA 60 because you need solve times where are
timestamp differences of adjacent blocks.

However what you describe above is also correct. So then I investigated
the disparity, and figured it out:

We don't do a proper LWMA(60) because of this: N = (DIFFICULTY_WINDOW - 1)

That is just wrong: it means we are doing a LWMA(59). But even that
isn't quite right because the resize(N+1) done in next_difficulty_v2
also throws away the top value. So what we end up calculating is
a LWMA(59) with a lag of one (that is, we throw away the most recent
usable observation).

The fix in this commit is making the math line up, but in not quite the
right away because there is still a bunch of stuff that is off by one.
Instead what we need is:

DIFFICULTY_WINDOW_V2 should be changed to 59, since that's what it is
(and has always) actually done.

DIFFICULTY_BLOCKS_COUNT_V2 should remain as DIFFICULTY_WINDOW_V2 + 1 because that's the proper number of
required blocks needed for a LWMA(DIFFICULTY_WINDOW_V2).

- the -1 should be removed from next_difficulty_v2 (so that it remains at 59, with the
above change).

- if we're on HF15 or earlier then we still need
DIFFICULTY_BLOCKS_COUNT_V2 blocks, but because of this bug they need
to be the values from blocks (H-61) through (H-2) rather than (H-60)
through (H-1).

- The // TODO: put asserts here, so that the difficulty algorithm is never
called with an oversized window (removed in this commit) should be
actually done as it should work fine with these fixes.
This commit is contained in:
Doyle 2020-09-15 16:14:24 +10:00 committed by Jason Rhinelander
parent 9c4aa8fa2a
commit fa505d6037
2 changed files with 15 additions and 35 deletions

View File

@ -175,7 +175,7 @@ namespace cryptonote {
// HF12 switches to RandomX with a likely drastically reduced hashrate versus Turtle, so override
// difficulty for the first difficulty window blocks:
else if (hf_version >= cryptonote::network_version_12_checkpointing &&
height < hf12_height + DIFFICULTY_WINDOW)
height < hf12_height + (DIFFICULTY_WINDOW + 1))
{
result = difficulty_calc_mode::hf12_override;
}
@ -189,7 +189,7 @@ namespace cryptonote {
difficulty_calc_mode mode)
{
const int64_t T = static_cast<int64_t>(target_seconds);
size_t N = DIFFICULTY_WINDOW - 1;
size_t N = DIFFICULTY_WINDOW;
// Return a difficulty of 1 for first 4 blocks if it's the start of the chain.
if (timestamps.size() < 4) {

View File

@ -93,44 +93,24 @@ static_assert(STAKING_PORTIONS % 12 == 0, "Use a multiple of twelve, so that it
#define DYNAMIC_FEE_REFERENCE_TRANSACTION_WEIGHT_V12 ((uint64_t)240000) // Only v12 (v13 switches back)
constexpr auto TARGET_BLOCK_TIME = 2min;
constexpr uint64_t DIFFICULTY_WINDOW = 60;
constexpr uint64_t DIFFICULTY_WINDOW = 59;
constexpr uint64_t DIFFICULTY_BLOCKS_COUNT(bool before_hf16)
{
// NOTE(loki): next_difficulty_v2(...) calculates the next block difficulty
// by examining the past N window of blocks i.e.
// NOTE: We used to have a different setup here where,
// DIFFICULTY_WINDOW = 60
// DIFFICULTY_BLOCKS_COUNT = 61
// next_difficulty_v2's N = DIFFICULTY_WINDOW - 1
//
// N = (DIFFICULTY_WINDOW - 1)
// And we resized timestamps/difficulties to (N+1) (chopping off the latest timestamp).
//
// and resizes the timestamps/difficulty arrays to
// Now we re-adjust DIFFICULTY_WINDOW to 59. To preserve the old behaviour we
// add +2. After HF16 we avoid trimming the top block and just add +1.
//
// array.resize(N+1) or in otherwords array.resize(DIFFICULTY_WINDOW)
//
// However all the code responsible for filling the timestamps/difficulty
// arrays prior to HF16 used DIFFICULTY_BLOCKS_COUNT which was defined as,
//
// constexpr uint64_t DIFFICULTY_BLOCKS_COUNT = (DIFFICULTY_WINDOW + 1); // added +1 to make N=N
//
// The comment suggests that +1 makes N=N, assuming the N refers to
// next_difficulty_v2's N variable. Adding +1 here does not make
// DIFFICULTY_BLOCKS_COUNT be equal to N.
//
// N = N1 = (DIFFICULTY_WINDOW - 1)
// N2 = DIFFICULTY_BLOCKS_COUNT = (DIFFICULTY_WINDOW + 1)
//
// What this means in practice is that, we fill DIFFICULTY_BLOCKS_COUNT
// worth of values and resize to DIFFICULTY_WINDOW size (chopping off the
// latest 2 blocks).
//
// This for example means when calculating the difficulty for a block, we
// 1. Fill the timestamps/difficulties with the latest DIFFICULTY_BLOCKS_COUNT data
// 2. Resize the timestamps/difficulties to DIFFICULTY_WINDOW. This steps
// chops off the last 2 blocks from consideration.
//
// So prior to HF16, we revert to the incorrect value, and afterwards we
// calculate correctly. With Pulse in HF16, this is largely a non-issue, but
// for future correctness (when the network falls back to PoW stalled) and
// purpose of documentation this is fixed and addressed in this function.
uint64_t result = (before_hf16) ? DIFFICULTY_WINDOW + 1 : DIFFICULTY_WINDOW;
// Ideally, we just set DIFFICULTY_BLOCKS_COUNT to DIFFICULTY_WINDOW
// + 1 for before and after HF16 (having one unified constant) but this
// requires some more investigation to get it working with pre HF16 blocks and
// alt chain code without bugs.
uint64_t result = (before_hf16) ? DIFFICULTY_WINDOW + 2 : DIFFICULTY_WINDOW + 1;
return result;
}