Remove buggy and confusing IncrementExtraNonce#24732
Merged
maflcko merged 2 commits intobitcoin:masterfrom Apr 6, 2022
Hidden character warning
The head ref may contain hidden characters: "2204-remove-code-\ud83c\udf17"
Merged
Conversation
maflcko
force-pushed
the
2204-remove-code-
branch
from
fa7624f to
faf8e55
Compare
maflcko
force-pushed
the
2204-remove-code-
branch
from
faf8e55 to
fa38b1c
Compare
Member
|
Concept ACK, nice cleanup. |
Contributor
|
Concept ACK |
1 similar comment
Contributor
|
Concept ACK |
Contributor
|
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. |
This was referenced Apr 1, 2022
Merged
Member
|
Code review ACK fa38b1c |
ajtowns
reviewed
Apr 5, 2022
Member
Author
|
Removed more code |
Contributor
|
ACK cccc4e8 |
Member
|
The RPC should probably throw an error if |
Member
Author
|
Yeah, and also when |
bitcoinhodler
added a commit
to bitcoinhodler/GlacierProtocol
that referenced
this pull request
Oct 27, 2022
bitcoin/bitcoin#24732 changes how new blocks are mined in regtest, which changes all the txids for our regtest transactions. I ran: t/online_regtest_wallet.py recreate-all-tests All of these tests are failing now because the golden no longer matches, which I will update next, but I first wanted to commit the automated step here separately. t/create-withdrawal-data.address-needs-correction.run required hand-editing because the input transaction appears twice.
bitcoinhodler
added a commit
to bitcoinhodler/GlacierProtocol
that referenced
this pull request
Oct 27, 2022
Created by running `make remake`. All tests passing again on bitcoin built from 79bf1a0fa2c72911bb964f7303dd3acee3db584f (just after bitcoin/bitcoin#24732 was merged).
bitcoinhodler
added a commit
to bitcoinhodler/GlacierProtocol
that referenced
this pull request
Oct 28, 2022
bitcoin/bitcoin#24732 changes the coinbase output of new blocks in regtest, which changes all the txids for our regtest transactions. I ran: t/online_regtest_wallet.py recreate-all-tests make remake t/create-withdrawal-data.address-needs-correction.run required hand-editing because the input transaction appears twice.
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jul 27, 2023
Summary: PR description: > IncrementExtraNonce has many issues: > > - It is test-only code, but part of bitcoind > - It is using the block height of the tip, as opposed to the block's previous block as reference for the new height. > - It has no use case in regtest testing. With a low difficulty the extra nonce won't be incremented. With a high difficulty the test-only functions are clumsy to handle anyway. For example, the generate* RPCs will return an empty array once they reached maxtries, as opposed to an error. Also the calls can't be aborted early unless the node shuts down completely. So I think it is fine to just remove the extra nonce functionality and leave it to the outside to implement, if needed. For example, a wrapper script can call the generate* RPCs once every second, to use the timestamp as extra nonce. Note that core did not need to update `m_assumeutxo_data` because their `TestChain100Setup` no longer used `IncrementExtraNonce` since bitcoin/bitcoin@fad84b7 (*test: Activate segwit in TestChain100Setup*) The changes in feature_utxo_set_hash are not relevant for now as we are still checking the hashes generated from the chain loaded from cache (we are missing the core#21390 ackport). This is a backport of [[bitcoin/bitcoin#24732 | core#24732]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D14312
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IncrementExtraNonce has many issues:
maxtries, as opposed to an error. Also the calls can't be aborted early unless the node shuts down completely. So I think it is fine to just remove the extra nonce functionality and leave it to the outside to implement, if needed. For example, a wrapper script can call thegenerate*RPCs once every second, to use the timestamp as extra nonce.