Use 512-bit arithmetic for difficulty retarget calculation. #6162

jrick commented at 4:14 PM on May 19, 2015: none

This prevents an integer overflow during the difficulty retarget calculation, and fixes a consensus-changing bug introduced by df9eb5e14fa8072bc8a82b59e712c2ba36f13f4c. While it is not possible to hit this consensus bug on mainnet or testnet3 due to the higher current difficulties, it is easily hit on regtest after generating 2016 blocks, forcing the retarget.

Note: to convert between the arith_uint256 and arith_uint512 classes, I created a new static function in the base_uint base class that copies the data from one to the other. However, to do this, I had to make the protected members WIDTH and pn public, so they could be accessed. Not too happy with this, and I think that I need to use friend to allow access, but I'm not sure exactly how to do that. Suggestions welcome.

jrick commented at 4:27 PM on May 19, 2015: none

More details about the bug are posted here: https://bitcointalk.org/index.php?topic=1065504.0

sdaftuar commented at 4:42 PM on May 19, 2015: member

Also reported in #5712. FYI I don't think this is possible to hit on testnet even if it were to be reset, because the starting difficulty is high enough to prevent overflow. Would be nice to fix this for regtest though...

davecgh commented at 4:44 PM on May 19, 2015: contributor

Correct. It can only be hit on regtest.

jrick commented at 4:46 PM on May 19, 2015: none

You're right, testnet3 shares the same pow limit as mainnet. I'll edit my commit message to match.

jrick force-pushed on May 19, 2015

jrick commented at 5:06 PM on May 19, 2015: none

Fixed the protected members by declaring all other base_uint classes to be friends of any base_uint<BITS>.

jonasschnelli commented at 9:43 AM on May 20, 2015: contributor

@jrick: wouldn't it make sense to provide a RPC test (maybe qa/rpc-tests/reorg-overflow.py or so) where you could prove your implementation? The test should fail on current master and succeed with master+this PR on top.

laanwj added the label Consensus on May 20, 2015

jrick commented at 2:20 PM on May 20, 2015: none

@jonasschnelli agree that exercising this with a test would be good to have. I can try my hand at the RPC test at it if no one else picks it up, but I am not familiar with python.

Don't want to sidetrack the discussion, but writing tests in a different language that contributors might not know seems like a strange decision. Testing everything over RPC instead of calling the functions directly also complicates the RPC API by requiring exposing many internals that otherwise an RPC client should not need.

jonasschnelli commented at 2:30 PM on May 20, 2015: contributor

@jrick: no need to expose internals over RPC. If you PR solves a real world problems (which this PR does IMO) you should be able to automate the steps-to-produce-the-error-you-solve by creating a RPC test. You don't need to know python. Just copy'n'paste from other tests and adapt your scenario. :)

Use 512-bit arithmetic for difficulty retarget calculation.

This prevents an integer overflow during the difficulty retarget
calculation, and fixes a consensus-changing bug introduced by
df9eb5e14fa8072bc8a82b59e712c2ba36f13f4c.  While it is not possible to
hit this consensus bug on mainnet or testnet3 due to the higher
proof-of-work limits, it is easily hit on regtest after generating
2016 blocks, forcing the retarget.

db5ad5e83d

jrick force-pushed on May 20, 2015

jrick commented at 9:46 PM on May 20, 2015: none

Added RPC tests.

in qa/pull-tester/rpc-tests.sh:None in 8aced2167b outdated

  33 | @@ -34,6 +34,7 @@ testScripts=(
  34 |      'maxblocksinflight.py'
  35 |      'invalidblockrequest.py'
  36 |      'rawtransactions.py'
  37 | +    'retarget-overflow.py'

jonasschnelli commented at 8:39 AM on May 21, 2015:

This test should be disabled in rpc-tests.sh because it mines serval hundred blocks and it's therefore not ideal for travis/CI (timeouts,etc.).

jrick commented at 2:39 PM on May 21, 2015:

What is the command to run all RPC tests, including those disabled for the pull tester?

jonasschnelli commented at 3:08 PM on May 21, 2015:

You can't. But there is a PR in this direction #6097

jrick commented at 3:19 PM on May 21, 2015:

Should retarget-overflow.py not appear in rpc-tests.sh at all, or be commented out like forknotify.py?

jonasschnelli commented at 3:55 PM on May 21, 2015:

IMO best practice (for the current state) is to add your test commented out in rpc-tests.sh.

jrick commented at 4:00 PM on May 21, 2015:

done

sipa commented at 4:09 PM on May 21, 2015:

The rule for regtest is just to start with a much lower difficulty than mainnet, but otherwise is identical in retargetting.

It's unfortunate that we would need larger integer support for just the regtest.

As regtest mining beyond the retargetting interval hasn't worked for a while, could we not just change its logic instead, and disable retargetting entirely? That seems closer to what people expect and needs fewer code changes to non-test code.

And yes, we do need a test that exercises retargetting... @sdaftuar?

sipa commented at 4:19 PM on May 21, 2015:

Ignore my suggestion to disable retarget on regtest, as testing retarget on mainnet itself is not feasible.

Can we easily redefine it to not require larger arithmetic?

If not, switching everything to 512 and adding tests for it like this PR does is probably the best option, as it's the only way to make sure the code under test is the same as the one affecting mainnet.

jonasschnelli commented at 8:47 AM on May 21, 2015: contributor

Tested (regtest only!). ACK. Changes look sane to me but i'm not sure if the changes in src/pow.cpp should go over chainparams (or something similar) so that this PR only affects regtest.

laanwj commented at 10:46 AM on May 21, 2015: member

Changes look sane to me but i'm not sure if the changes in src/pow.cpp should go over chainparams (or something similar) so that this PR only affects regtest.

I'm also wary of this, if this is a problem that affects only testing code (regtest), it's unnecessary risk to change mainnet consensus code for it.

Remind me: Why does it retarget difficulty on regtest at all? From what I've always understood, the difficulty was supposed to be fixed at the bare minimum to allow instant block generation.

jrick commented at 2:37 PM on May 21, 2015: none

I can't speak for regtest, but @davecgh and I hit this issue while I was patching in support for btcd's simnet so I could do some testing against one of our private test networks. The difficulty retarget is needed in this case, or block generation timings on our network would not be as realistic as we need.

In the past, whenever we've talked about simnet in #bitcoin-dev, we've been told "that sounds just like regtest, why not use that instead?", so there are some people who do expect regtest to also work for simulation testing and long-lived private networks. A network that always has the lowest possible difficulty would indeed be useful for some situations, but useless for others.

Assumming the regtest retarget is kept, how would you incorporate the logic to use the 512-bit math only on regtest? I'm all for avoiding touching working mainnet code, but tests become less and less useful the more special cases are made for the test environment. In this case, I would think that sharing the difficulty retarget code on all networks is safest.

davecgh commented at 3:53 PM on May 21, 2015: contributor

I would also suggest that having a test for the retarget code which does not rely on special casing or branches specific to the network makes the most sense and is the least risky approach.

This code path is currently uncovered by the test code. The block tester tool does not generate enough blocks to hit the retarget interval, and I haven't seen any test cases anywhere which otherwise cover it. Empirically, it works properly on mainnet and testnet, and naturally mainnet can be externally tested via an ASIC by disabling checkpoints, which are all good things. However, I personally don't think it's a good idea to solely rely on human testing for consensus-related code.

Add RPC tests to ensure no retarget overflow. bfa0f38f1d

jrick force-pushed on May 21, 2015

jrick commented at 5:03 PM on May 21, 2015: none

It's probably better for the rpc tests to calculate the exact expected retarget difficulty, instead of simply checking that the change was within the expected bounds.

sipa commented at 2:04 PM on June 14, 2015: member

Regtest does retarget just like mainnet, the only difference is that it starts off at a much lower difficulty.

Given that part of the purpose of that regtest is covering code to be used by mainnet, I think it's important that they do use the same code. I'm not very keen on changing the code for mainnet, though - even if it shouldn't make any difference, so I don't really know what the best solution is...

jtimon commented at 12:38 AM on August 29, 2015: contributor

NACK, I would prefer regtest not to ever retarget as @laanwj suggests.

jgarzik commented at 5:54 PM on September 15, 2015: contributor

Leaning towards closing. Seems unlikely to be merged anytime soon given current comments.

dcousens commented at 7:03 AM on September 16, 2015: contributor

[Weak] NACK.

If it isn't an issue, IMHO, don't bother adding the code to calculate it, however, it would be nice to document and assert that assumption.

sipa commented at 11:59 AM on September 16, 2015: member

Well regtest currently cannot deal with retargets. I believe that's a significant limitation and reduces the usefulness of regtest (but bringing it back ideally means using the same code for both).

davecgh commented at 2:13 PM on September 16, 2015: contributor

As previously mentioned, it is in fact an issue on regtest. Retargets are not possible on regtest currently (and they used to be before commit df9eb5e14fa8072bc8a82b59e712c2ba36f13f4c). Unfortunately, there is currently no test coverage which exercise the retarget code and is why this was able to go through.

Repeating what I said above, I would also suggest that having a test for the retarget code which does not rely on special casing or branches specific to the network makes the most sense and is the least risky approach. That is the approach this pull request takes. It would appear @sipa agrees as he commented that it "ideally means using the same code for both."

This isn't a theoretical issue. We hit it because we were doing some testing with btcd and Bitcoin Core on regtest which involved retargets and noticed that btcd handles it correctly while Bitcoin Core does not even though they used to both handle it correctly in the past. This creates a situation where 3rd-party software can't properly be tested across retarget intervals on the regression test network with Bitcoin Core.

laanwj commented at 10:43 AM on October 19, 2015: member

As retargetting never worked on regtest I'd prefer it to be disabled completely, as from what I've gathered, people don't really expect the difficulty on regtest to ramp up.

laanwj closed this on Oct 19, 2015

sipa commented at 11:15 AM on October 19, 2015: member

In that case I think we should add a boolean to Consensus::Params called fNoRetargetting, that disables the logic entirely. Currently we just can't create regtest chains longer than 2016 blocks...

btcdrak commented at 11:21 AM on October 19, 2015: contributor

@sipa I agree, that would be a much clearer solution. I know the versionbits integration tests are going to need the ability to generate more than 2016 blocks on regtestnet (afaik). refs #6816

CodeShark commented at 11:24 AM on October 19, 2015: contributor

@sipa @btcdrak Regarding your fNoRetargetting, indeed! I had to force CheckProofOfWork to return true to actually run tests on #6816

btcdrak commented at 11:30 AM on October 19, 2015: contributor

@CodeShark maybe you should just fix it in your PR, or should there be a separate PR?

davecgh commented at 1:26 PM on October 19, 2015: contributor

@laanwj: It is not true that retargetting never worked on regtest. It worked prior to commit df9eb5e.

morcos commented at 2:09 PM on October 19, 2015: member

For the record, I was in favor of this pull, but not strongly enough to continue advocating for it.

jonasschnelli commented at 2:12 PM on October 19, 2015: contributor

I also ACKed this PR. If only the mainnet effects would be reduced by moving/encapsulating the changes from the PR in CalculateNextWorkRequired() somehow towards chainparams.cpp ...

jtimon commented at 5:37 PM on October 19, 2015: contributor

This code path is currently uncovered by the test code.

Actually there's currently some unittest for CalculateNextWorkRequired(), see https://github.com/bitcoin/bitcoin/blob/master/src/test/pow_tests.cpp. In fact, testing was the reason why CalculateNextWorkRequired() was separated from GetNextWorkRequired() in the first place (see https://github.com/bitcoin/bitcoin/commit/34e5015cd21e27c1bf635d92531afac93f553096 ).

DrahtBot locked this on Sep 8, 2021