Back when we implemented coin age priority as a miner policy, miners
mempools might admit transactions paying very low fees, but then want to
set a higher fee for block inclusion. However, since coin age priority
was removed in v0.15, the block assembly policy is solely based on fees,
so we do not need to apply minimum feerate rules in multiple places. In
fact, the block assembly policy ignoring transactions that are added to
the mempool is likely undesirable as we waste resources accepting and
storing this transaction.
Instead, rely on mempool policy to enforce a minimum entry feerate to
the mempool (minrelaytxfee). Set the minimum block feerate to the
minimum non-zero amount (1sat/kvB) so it collects everything it finds in
mempool into the block.
Github-Pull: #33106
Rebased-From: 5f2df0ef78be7b24798d0983c9b962740608f1f4
386eecff5f doc: add release notes (ismaelsadeeq)
3eaa0a3b66 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d6 miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)
Pull request description:
* This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
* Fixes#21950
We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`
7590e93bc7/src/policy/policy.h (L23)
And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.
7590e93bc7/src/node/types.h (L36-L40)
**Motivation**
- This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
- It was later reported in issue #21950.
- I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.
---
This PR fixes this by consolidating the reservation to be in a single location in the codebase.
This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.
ACKs for top commit:
Sjors:
ACK 386eecff5f
fjahr:
Code review ACK 386eecff5f
glozow:
utACK 386eecff5f, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f
Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c
- This commit renamed coinbase_max_additional_weight to block_reserved_weight.
- Also clarify that the reservation is for block header, transaction count
and coinbase transaction.
Previously in getblocktemplate only curtime took the timewarp rule into account.
Mining pool software could use either, though in general it should use curtime.
73db95c65c kernel: Make bitcoin-chainstate's block validation mirror submitblock's (TheCharlatan)
bb53ce9bda tests: Add functional test for submitting a previously pruned block (Greg Sanders)
1f7fc73825 rpc: Remove submitblock duplicate pre-check (TheCharlatan)
e62a8abd7d rpc: Remove submitblock invalid-duplicate precheck (TheCharlatan)
36dbebafb9 rpc: Remove submitblock coinbase pre-check (TheCharlatan)
Pull request description:
With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden.
The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.
The coinbase check is repeated again early during ProcessNewBlock. Pre-checking it may also shadow more fundamental problems with a block. In most cases the block header is checked first, before validating the transactions. Checking the coinbase first therefore masks potential issues with the header. Fix this by removing the pre-check.
Similary the duplicate checks are repeated early in the contextual checks of ProcessNewBlock. If duplicate blocks are detected much of their validation is skipped. Depending on the constitution of the block, validating the merkle root of the block is part of the more intensive workload when validating a block. This could be an argument for moving the pre-checks into block processing. In net_processing this would have a smaller effect however, since the block mutation check, which also validates the merkle root, is done before.
Testing spamming a node with valid, but duplicate unrequested blocks seems to exhaust a CPU thread, but does not seem to significantly impact keeping up with the tip. The benefits of adding these checks to net_processing are questionable, especially since there are other ways to trigger the more CPU-intensive checks without submitting a duplicate block. Since these DOS concerns apply even less to the RPC interface, which does not have banning mechanics built in, remove them too.
Finally, also remove the pre-checks from `bitcoin-chainstate.cpp`.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
Sjors:
re-utACK 73db95c65c
achow101:
ACK 73db95c65c
instagibbs:
ACK 73db95c65c
mzumsande:
ACK 73db95c65c
Tree-SHA512: 2d02e851cf402ecf6a1968c058df3576aac407e200cbf922a1a6391b7f97b4f42c6d9f6b0a78b9d1af0a6d40bdd529a7b11a1e6d88885bd7b8b090f6d1411861
This tests the new submitblock behaviour that is introduced in the
previous commit: Submitting a previously pruned block should persist the
block's data again.
The coinbase check is repeated again early during ProcessNewBlock.
Pre-checking it may also shadow more fundamental problems with a block.
In most cases the block header is checked first, before validating the
transactions. Checking the coinbase first therefore masks potential
issues with the header. Fix this by removing the pre-check.
The pre-check was likely introduced on top of
ada0caa165 to fix UB in
GetWitnessCommitmentIndex in case a block's transactions are empty. This
code path could only be reached because of the call to
UpdateUncommittedBlockStructures in submitblock, but cannot be reached
through net_processing.
Add some functional test cases to cover the previous conditions that
lead to a "Block does not start with a coinbase" json rpc error being
returned.
---
With the introduction of a mining ipc interface and the potential future
introduction of a kernel library API it becomes increasingly important
to offer common behaviour between them. An example of this is
ProcessNewBlock, which is used by ipc, rpc, net_processing and
(potentially) the kernel library. Having divergent behaviour on
suggested pre-checks and checks for these functions is confusing to both
developers and users and is a maintenance burden.
The rpc interface for ProcessNewBlock (submitblock) currently pre-checks
if the block has a coinbase transaction and whether it has been
processed before. While the current example binary for how to use the
kernel library, bitcoin-chainstate, imitates these checks, the other
interfaces do not.
The added regtest option -test=bip94 is only used in the functional
test for BIP94.
This is done because the default regtest consensus rules
should aim to follow to mainnet, not testnet.
Please review the last merge (not this commit) carefully. The bitcoin merge of bitcoin/bitcoin#24753
did not lead to any changes in the mining_basic functional test, so it's possible I made a mistake
which caused this test to be affected.
Previously, `wait_for_getheaders` would check whether a node had received **any**
getheaders message. This implied that, if a test needed to check for a specific block
hash within a headers message, it had to make sure that it was checking the desired message.
This normally involved having to manually clear `last_message`. This method, apart from being
too verbose, was error prone, given an undesired `getheaders` would make tests pass.
This adds the ability to check for a specific block_hash within the last `getheaders` message.
Included a test that checks if we call submitblock with
block.vtx.empty() then it throws an rpc deserialization error, currently
we only test if !block.vtx->IsCoinBase() throws an rpc deserialization
error
We comment out a newly-added assert from the Bitcoin
side, because it's unclear how to replicate it
correctly in Elements.
Co-authored-by: Glenn Willen <gwillen@nerdnet.org>
