mining: Remove unused extranonce logic #21533
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
It's always called with extra_nonce=0
It's always called with 0.
It's always set to 1 and never changed. Just use a integer literal.
jnewbery
force-pushed
the
2021-03-extra-nonce
branch
from
4740100 to
28f052f
Compare
Takes the height of the desired block instead of the *CBlockIndex of the previous block.
It's redundant with the block in/out param.
|
Thanks for the review @Empact! I've addressed your comment. |
jnewbery
force-pushed
the
2021-03-extra-nonce
branch
from
28f052f to
fcc0b52
Compare
jnewbery
changed the title
There was a problem hiding this comment.
Concept ACK
Reviewed commits 05d053c ... b97a35b, changes LGTM
However, I think the last commit (fcc0b52) 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
truewith validblock_hashout-param: generation of block succeeded - returns
truewith invalid (nullified)block_hashout-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.
| block_hash = block.GetHash(); | ||
| if (block.GetHash().IsNull()) { | ||
| throw JSONRPCError(RPC_MISC_ERROR, "Failed to make block."); | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Thanks for the reviews @theStack and @Empact. I realize now that I've misread the new I think it's very confusing that state is being passed around and shared between |
4 participants
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 inCreateNewBlock(), and the extra nonce could be incremented. See dcc8332#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.