Both waitTipChanged() and createNewBlock() can take a long time to
return. Add a way for clients to interrupt them.
The new m_interrupt_mining is safely accessed with a lock on
m_tip_block_mutex, but it has no guard annotation. A more thorough
solution is discussed here:
https://github.com/bitcoin/bitcoin/pull/34184#discussion_r2743566474
At startup, if the needs to catch up, connected mining clients will
receive a flood of new templates as new blocks are connected.
Fix this by adding a cooldown argument to createNewBlock(). When set
to true, block template creation is briefly paused while the best
header chain is ahead of the tip.
This wait only happens when the best header extends the current tip,
to ignore competing branches.
Additionally, cooldown waits for isInitialBlockDownload() to latch to
false, which happens when there is less than a day of blocks left to sync.
When cooldown is false createNewBlock() returns immediately. The argument
is optional, because many tests are negatively impacted by this
mechanism, and single miner signets could end up stuck if no block
was mined for a day.
The getblocktemplate RPC also opts out, because it would add a delay
to each call.
Fixes#33994
This increments the field number of the `Init.makeMining` method and makes the
old `makeMining` method return an error, so existing IPC mining clients not
using the latest schema file will get an error and not be able to access the
Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the
mining interface has evolved a lot since it was first introduced, with old
clients using the original methods less stable and performant than newer
clients. So now is a good time to introduce a cutoff, drop deprecated methods,
and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that
would be awkward to implement compatibly, so a few of these were implemented in
commits immediately preceding this one.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Adding a context parameter ensures that these methods are run in
their own thread and don't block other calls. They were missing
for:
- createNewBlock()
- checkBlock()
The missing parameters were first pointed out by plebhash in
https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3383290115 and
adding them should prevent possible performance problems and lockups,
especially with #34184 which can make the createNewBlock method block for a
long time before returning. It would be straightforward to make this change in
a backward compatible way
(https://github.com/bitcoin/bitcoin/pull/34184#discussion_r2770232149) but nice
to not need to go through the trouble.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This change removes deprecated methods from the ipc mining interface.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
This change copies default option values from the C++ mining interface to the
Cap'n Proto interface. Currently, no capnp default values are set, so they are
implicitly all false or 0, which is inconvenient for the rust and python
clients and inconsistent with the C++ client.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
This commit only declares constants without using them. They will be applied in
seperate commit since changing struct default field values in cap'n proto is
not backwards compatible.
faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
ryanofsky:
Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
48f57bb35bbdbce509b8ef195de69e2a61a2511e mining: add new getCoinbaseTx() returning a struct (Sjors Provoost)
d59b4cdb5772917ee13e48552d51662160104b62 mining: rename getCoinbaseTx() to ..RawTx() (Sjors Provoost)
Pull request description:
The first commit renames `getCoinbaseTx()` to `getCoinbaseRawTx()` to reflect that it returns a serialised transaction. This does not impact IPC clients, because they do not use the function name.
The second commit then introduces a replacement `getCoinbase()` that provides a struct with everything clients need to construct a coinbase. This avoids clients having to parse and manipulate our dummy transaction.
Deprecate but don't remove `getCoinbaseRawTx()`, `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.
After this change we can drop these deprecated methods, which in turn would allow us to clear the dummy transaction from the `getBlock()` result. But that is left for a followup to keep this PR focussed. See https://github.com/Sjors/bitcoin/pull/106 for an approach.
Expand the `interface_ipc.py` functional test to document its usage.
Can be tested using:
- https://github.com/stratum-mining/sv2-tp/pull/59
ACKs for top commit:
ryanofsky:
Code review ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change
ismaelsadeeq:
code review ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e
vasild:
ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e
Tree-SHA512: c4f1d752777fb3086a1a0b7b8b06e4205dbe2f3adb41f218855ad1dee952adccc263cf82acd3bf9300cc83c2c64cebd2b27f66a69beee32d325b9a85e3643b0d
Introduce a new method intended to replace getCoinbaseRawTx(), which
provides a struct with everything clients need to construct a coinbase.
This is safer than providing a raw dummy coinbase that clients then have
to manipulate.
The CoinbaseTx data is populated during the dummy transaction generation
and stored in struct CBlockTemplate.
Expand the interface_ipc.py functional test to document its usage
and ensure equivalence.
This frees up the name getCoinbaseTx() for the next commit.
Changing a function name does not impact IPC clients, as they only
consider the function signature and sequence number.
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "ipc:".
This requires some small refactors to silence false-positive warnings.
Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.
866bbb98fd365962840ee99df0d9f7ad557cc025 cmake, test: Improve locality of `bitcoin_ipc_test` library description (Hennadii Stepanov)
ae2e438b257f2b5322080087ed7dc8843d4f9cca cmake: Move IPC tests to `ipc/test` (Hennadii Stepanov)
Pull request description:
This PR follows up on https://github.com/bitcoin/bitcoin/pull/33445 and:
1. Organizes the IPC tests in the same way as the wallet tests.
2. Removes no longer needed `src/test/.clang-tidy.in`.
See the previous discussion:
- https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340
- https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329
Additionally, the locality of the `bitcoin_ipc_test` build target description has been improved.
ACKs for top commit:
Sjors:
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025
janb84:
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025
ryanofsky:
Code review ACK 866bbb98fd365962840ee99df0d9f7ad557cc025, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time.
Tree-SHA512: ed7cc817ccb88595d8516978bff0ea2560048d35b3f548e7913aec7d58b8d6ac550e230e992c527fb747bef175580be92dc4df6342e4485f3a9870dba0a25cba
dcb56fd4cb59e6857c110dd87019459989dc1ec3 interfaces: add interruptWait method (ismaelsadeeq)
Pull request description:
This is an attempt to fix#33575 see the issue for background and the usefulness of this feature.
This PR uses one of the suggested approaches: adding a new `interruptWaitNext()` method to the mining interface.
It introduces a new boolean variable, `m_interrupt_wait`, which is set to `false` when the thread starts waiting. The `interruptWaitNext()` method wakes the thread and sets `m_interrupt_wait` to `true`.
Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set ` m_interrupt_wait ` to false and return`nullptr`.
This PR also adds a functional test for the new method. The test uses `asyncio` to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing.
If that buffer elapses without `waitNext` executing, the test will fail because a transaction is created after the buffer.
ACKs for top commit:
furszy:
Code ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
ryanofsky:
Code review ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.
Sjors:
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
TheCharlatan:
ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
Tree-SHA512: a03f049e1f303b174a9e5d125733b6583dfd8effa12e7b6c37bd9b2cff9541100f5f4514e80f89005c44a57d7e47804afe87aa5fdb6831f3b0cd9b01d83e42be
Without this change, logging (even if unused) may account for a
substantial portion of bitcoin-node's and/or client's runtime cpu usage, due
to libmultiprocess's expensive message serialization.
This (along with some recent upstream changes) avoids the overhead by opting
out of log handling for messages that we're not interested in.
Info, Warning, and Error are logged unconditionally to match our behavior
elsewhere. See BCLog::Logger::GetCategoryLogLevel .
589b65f06c3376df4cde3fac5c1d39a2d3254920 clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc` (Hennadii Stepanov)
Pull request description:
The warnings are false positive and have been fixed upstream. See: https://github.com/capnproto/capnproto/pull/2334.
This PR:
1. Disables the `UndefinedBinaryOperatorResult` clang-tidy check for source files generated by the `mpgen` tool.
2. Is an alternative to the draft https://github.com/bitcoin/bitcoin/pull/33281.
3. Fixes https://github.com/bitcoin/bitcoin/issues/33256.
ACKs for top commit:
Sjors:
ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920
fjahr:
ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920
achow101:
ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920
ryanofsky:
Code review ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920. Thanks for the fix!
Tree-SHA512: 6d376a82641a5b85d4dd1fa164fdcbd8e15f1262e7d4f582f4d9959031d35852e28ff1b8268336e39ba6779fdd10ecdb986af42407d0545f4217f41d64556272
The warnings are false positive and have been fixed upstream.
See: https://github.com/capnproto/capnproto/pull/2334.
This change disables the `UndefinedBinaryOperatorResult` clang-tidy
check for source files generated by the `mpgen` tool.
This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if Ctrl-C is pressed when `bitcoin-node` is started, it is
handled by both `bitcoin-node` and `bitcoin-wallet` processes, causing the
wallet to shutdown abruptly instead of waiting for the node and shutting down
cleanly.
This change fixes the problem by having the wallet process print to stdout when
it receives a Ctrl-C signal but not otherwise react, letting the node shut
everything down cleanly.
Use EventLoopRef to avoid reference counting bugs and be more exception safe
and deal with removal of addClient/removeClient methods in
https://github.com/bitcoin-core/libmultiprocess/pull/160
A test update is also required due to
https://github.com/bitcoin-core/libmultiprocess/pull/160 to deal with changed
reference count semantics. In IpcPipeTest(), it is now necessary to destroy
the client Proxy object instead of just the client Connection object to
decrease the event loop reference count and allow the loop to exit so the test
does not hang on shutdown.
386eecff5f14d508688e6e7374b67cb54aaa7249 doc: add release notes (ismaelsadeeq)
3eaa0a3b663782bb1bd874ea881b21649f1db767 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd14841e35ce39d7a6f51131e6a41de2 doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d5a7617764857b51777c076fd7ef13d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc9155ed58ad97fc04b47b3d317a3ec2 test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d67a159332d109aab278632d64078f0b 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 386eecff5f14d508688e6e7374b67cb54aaa7249
fjahr:
Code review ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
glozow:
utACK 386eecff5f14d508688e6e7374b67cb54aaa7249, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
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.
processNewBlock was added in 7b4d3249ced93ec5986500e43b324005ed89502f, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ced93ec5986500e43b324005ed89502f.
getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface).
It's very low level and not used by the proposed Template Provider.
This method was introduced in d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3
and a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed.
