Daily cherry pick#23
Merged
Merged
Conversation
…p include_dummy_extranonce 8544537 mining: drop unused include_dummy_extranonce option (Sjors Provoost) 58eeab7 mining: only pad with OP_0 at heights <= 16 (Sjors Provoost) 00d2232 mining: pad coinbase to fix createNewBlock at heights <=16 (Sjors Provoost) 605ff37 test: bad-cb-length for createNewBlock() at low heights (Sjors Provoost) 1966621 test: refactor IPC mining test to use script_BIP34_coinbase_height (Sjors Provoost) Pull request description: Blocks 0-16 on any new chain require mining code to be careful not to violate the `bad-cb-length` rule, which states the coinbase transaction scriptSig must be at least 2 bytes. Our mining code deals with that by padding the `scriptSig` with a 0 `extraNonce`. It does this for every height. As a result IPC clients would get an unnecessary `0` in the `scriptSigPrefix` field of `CoinbaseTx`. bitcoin#32420 fixed that by introducing a `include_dummy_extranonce` option in `BlockCreateOptions` and turning that off for IPC clients. A minor issue was missed though: `createNewBlock()` now fails with `bad-cb-length`. An easy workaround is to use the `generate` RPC for the first 16 blocks, as demonstrated in the 2nd commit. The real fix is to have the miner code always pad the `scriptSig` at lower heights, but to _not_ include that in the `scriptSigPrefix` field of `CoinbaseTx` (introduced in bitcoin#33819). This is what the 3rd commit implements. Now that we set `scriptSigPrefix` independent of what our internal miner code does - to get past `CheckBlock()` - the original motivation for `include_dummy_extranonce` goes away and we can just drop it entirely. The last commit drops it, while the 4th commit adjusts the tests and hardcoded block and assume utxo hashes. This last change does not break IPC clients, because `include_dummy_extranonce` was never exposed in `mining.capnp`. Instead of adjusting the hardcoded hashes, an alternative approach would be to just always pad the `scriptSig` internally, since we exclude the padding from `scriptSigPrefix` anyway. However, IPC clients can also call `getBlock()` to get the raw block and might be confused about the difference. The miner code is also easier to understand if we limit the exception (`coinbase_tx.script_sig_prefix != coinbaseTx.vin[0].scriptSig`) to `nHeight <= 16`, where the explanation is based purely on consensus rules rather than historical test suite reasons. The first two commits are preperation test changes: - extract `assert_capnp_failed` helper for macOS (also part of bitcoin#34727) - use `script_BIP34_coinbase_height` in IPC mining test (existing code in `interface_ipc_mining.py` was incorrect for low height Fixes bitcoin#35126 ACKs for top commit: ryanofsky: Code review ACK 8544537. Just rebased to fix silent conflict and applied some minor suggestions since last review. As part of rereviewing I left some more minor suggestions that are fine to ignore. sedited: Re-ACK 8544537 Tree-SHA512: a01d48842bf4bcc1a9c51a89ef9d750766db7d04edb4dcd6b3a8bf195c6b4fa07445256a49367ff0db00ab489a52a3d7ff6a5c3ab9290ecb1fcb82f532552e9b (cherry picked from commit 7802e57)
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
No description provided.