mining: Remove unused extranonce logic

jnewbery commented at 10:53 am on March 27, 2021: member

Since at least #17693, the extra nonce logic has been unused.

Prior to that PR, if the nNonce field rolled, then the while loop inside generateBlocks() would iterate, a new block would be generated in CreateNewBlock(), and the extra nonce could be incremented. See https://github.com/bitcoin/bitcoin/pull/17693/commits/dcc8332543f8fb6d1bb47cb270fcbb6a814a7d6e#diff-ccc24453c13307f815879738d3bf00eec351417537fbf10dde1468180cacd2f1L132-L133.

After that PR, the extra_nonce can only be set to 0 in the call to IncrementExtraNonce().

I think this is fine and we should just remove the unused code. The generate RPCs are only used for regtest and signet. For regtest, the difficulty is always minimum, so the nNonce field won’t roll. If the nNonce field rolls in signet and the rpc fails, then the rpc can just be called again and a new candidate block will be generated.

DrahtBot added the label Mining on Mar 27, 2021

DrahtBot added the label RPC/REST/ZMQ on Mar 27, 2021

DrahtBot commented at 4:10 pm on March 27, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21391 ([Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl)
#20017 (rpc: Add RPCContext by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/miner.cpp:437 in 4740100090 outdated

433@@ -434,19 +434,13 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
434     }
435 }
436 
437-void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce)
438+void SetCoinbaseScriptSig(CBlock* pblock, int32_t height)

Empact commented at 5:43 am on March 30, 2021:

nit: assert against invalid height?

jnewbery commented at 8:30 am on March 30, 2021:

done!

Empact commented at 5:48 am on March 30, 2021: member

Concept ACK

[rpc] Remove extra_nonce argument from GenerateBlock()

It's always called with extra_nonce=0

05d053cee3

[mining] Remove nExtraNonce argument from IncrementExtraNonce()

It's always called with 0.

5a0379f0aa

[mining] Remove nExtraNonce variable from IncrementExtraNonce

It's always set to 1 and never changed. Just use a integer literal.

dd5c07e97e

[mining] Remove hashPrevBlock static variable from IncrementExtraNonce

It's unused.

b971c6c538

[mining] Rename IncrementExtraNonce to SetCoinbaseScriptSig 2214b41fd3

jnewbery force-pushed on Mar 30, 2021

jnewbery commented at 8:07 am on March 30, 2021: member

Rebased to pick up android CI fix: https://github.com/bitcoin/bitcoin/compare/4740100090bde0668b7baf3044d56d628974aa45..28f052f0b85907bdf49df994014a6708c6dfc218

[miner] Update SetCoinbaseScriptSig to take height

Takes the height of the desired block instead of the *CBlockIndex of the
previous block.

b97a35b3b7

[rpc] Remove block_hash out param from GenerateBlock

It's redundant with the block in/out param.

fcc0b525e2

jnewbery commented at 8:30 am on March 30, 2021: member

Thanks for the review @Empact! I’ve addressed your comment.

jnewbery force-pushed on Mar 30, 2021

jnewbery renamed this:
~~[mining] Remove unused extranonce logic~~
mining: Remove unused extranonce logic
on Mar 30, 2021

theStack commented at 1:18 am on April 13, 2021: member

Concept ACK

Reviewed commits 05d053cee3852f68213008efda575e90cf4d9787 … b97a35b3b7fc1bc09f5f38516dadf0afaed6a6e7, changes LGTM However, I think the last commit (fcc0b525e212c3c2b5ff58de9c19cc4c152720f6) changes the logic unintentionally, as reaching the maximum nNonce then leads to a silent fail, i.e. the generateblock RPC call would succeed without having generated a block. Before this change, GenerateBlock used kind of a tri-state logic to show if it’s been successful:

returns false: generation of block failed
returns true with valid block_hash out-param: generation of block succeeded
returns true with invalid (nullified) block_hash out-param: generation of block didn’t succeed, maximum nNonce reached

The third case is not needed anymore. The introduced check in GenerateBlock for block.GetHash().IsNull() can be removed (I don’t think the condition can ever be true) and if nNonce is about to wrap, simply false rather than true should be returned, IMHO.

in src/rpc/mining.cpp:133 in fcc0b525e2

127@@ -132,7 +128,10 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
128         throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
129     }
130 
131-    block_hash = block.GetHash();
132+    if (block.GetHash().IsNull()) {
133+        throw JSONRPCError(RPC_MISC_ERROR, "Failed to make block.");
134+    }

Empact commented at 3:28 am on April 13, 2021:

I tend to agree with @theStack that throwing here is a change of failure modes for generateBlocks, impacting generatetodescriptor and generatetoaddress.

However, I’m not clear on when this condition occurs. CBlock::GetHash() resolves to SerializeHash(*this), and I didn’t see a failure possible in the serialization logic (through a cursory glance).

Options seem to be:

retain existing behavior by returning false in this case
fail more noisily, given that this is not an expected error, as here

jnewbery commented at 12:35 pm on April 14, 2021:

It’s a very strange and unintuitive interface. The passed out hash will be null if block.nNonce == std::numeric_limits<uint32_t>::max(). The nullness of that out-param is what is used to determine whether the outer while loop should increment the height or try again at the same height.

jnewbery commented at 12:38 pm on April 14, 2021: member

Thanks for the reviews @theStack and @Empact. I realize now that I’ve misread the new GenerateBlock() function and that the effective tri-state return means that the extranonce can be incremented if we exit from GenerateBlock() with block_hash unset.

I think it’s very confusing that state is being passed around and shared between generateBlocks(), GenerateBlock(), and IncrementExtraNonce() (which is storing a static variable of the hash of the previous block). I think that could be clarified/cleaned up, but for now I’m just going to close this PR.

jnewbery closed this on Apr 14, 2021

DrahtBot locked this on Aug 18, 2022

mining: Remove unused extranonce logic #21533

Conflicts