ipc: add bitcoin-mine test program#30437
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30437. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste 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. |
|
Thanks. I'll try to get the Mining interface in good shape as well, which this can then be rebased on:
|
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
I'll work on dropping the spawn functionality from this PR next, and just having the
This would simplify the PR by allowing 3 commits to be dropped:
Other things I'd like to do:
|
You won't need that if you include #30440, though maybe it's better to wait until that's merged to avoid too much code churn. |
Yeah definitely some of that should not be needed. Some parts will probably be needed though. For example, the initial version of this PR did not support deserializing |
|
I'll certainly need |
|
I opened draft PRs for all interface changes that I think I will need, see updated comment above: #30437 (comment) If you can cherry-pick them all, I'll expand the demo app to simulate all the steps a Template Provider would do. That should reveal any remaining gaps in the interface. |
|
I wrote:
Once that works the next step for me would be to open an alternative to #29432 which runs the Template Provider in I could then modify the guix build to only produce That way the user can simply install and run a recent Bitcoin Core release in the manner they're familiar with. They would install If later on we can also turn This would be in parallel to the alternative approach of writing With both approaches it will take a while to reach feature parity with #29432 so I'll have to keep maintaining that too (using the same interface, but not as a separate process). |
|
I simplified PR as suggested so I also added a new commit to make the IPC code compatible with all the interfaces added in #30409, #30356, #30440, and #30443, so this PR is easier to test with those interfaces. I still need to break up the main commit ("Add bitcoin-mine test program") and add unit tests for the new connecting, listening, and serialization code that's introduced. Rebased 3c0a13c -> 4e1a434 ( For reference, the code that was dropped from this PR allowing bitcoin-mine to spawn a bitcoin-node process is in commits:
These changes would probably be useful in another context, but aren't needed for this test program. re: #30437 (comment) I think your ideas for packaging bitcoin-tp make sense. And maybe it would also be good to try to integrate IPC in bitcoind and bitcoin-qt executables instead of separate bitcoin-node and bitcoin-gui executables. Would have to think about how to implement this and what the tradeoffs would be. |
This comment was marked as off-topic.
This comment was marked as off-topic.
| } | ||
| block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, block_template->getCoinbaseTx()); | ||
|
|
||
| if (tip->hash != mining->getTip()->hash) { |
There was a problem hiding this comment.
In 1a844b8 bitcoin-mine: Extend example to mine a block: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
There was a problem hiding this comment.
re: #30437 (comment)
In 1a844b8 bitcoin-mine: Extend example to mine a block: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
I guess technically the message didn't say who mined the block. But reworded it to avoid unnecessary excitement
|
re-ACK a2365f1 Since my last review of e1f139b, commits 8bf2269 and b094d38 were absorbed into #33201 which added direct coverage of IPC via a functional test. This also allowed some simplification of I initially wanted to suggest folding this test into |
There was a problem hiding this comment.
Updated 2cb5e64 -> 057ea0a (pr/mine.34 -> pr/mine.35, compare) just tweaking bitcoin-mine status message
Rebased 057ea0a -> 69d23a0 (pr/mine.35 -> pr/mine.36, compare) due to conflicts with #34452
Rebased 69d23a0 -> bffde2f (pr/mine.36 -> pr/mine.37, compare) due to conflict with #34860
|
|
||
| while (tries_remaining> 0 && block.nNonce < std::numeric_limits<uint32_t>::max() && !CheckProofOfWork(block.GetHash(), block.nBits, consensus_params)) { | ||
| ++block.nNonce; | ||
| --tries_remaining; |
There was a problem hiding this comment.
re: #30437 (comment)
+1. I think just having simple code to demonstrate the API with comments pointing out how usage could be more robust is a good middle ground.
|
|
||
| // Note: This loop ignores tip changes so the submitSolution call below | ||
| // could fail even if the CheckProofOfWork succeeds! A more realistic miner | ||
| // could avoid this problem by calling call block_template->waitNext() |
| } | ||
| block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, block_template->getCoinbaseTx()); | ||
|
|
||
| if (tip->hash != mining->getTip()->hash) { |
There was a problem hiding this comment.
re: #30437 (comment)
In 1a844b8 bitcoin-mine: Extend example to mine a block: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
I guess technically the message didn't say who mined the block. But reworded it to avoid unnecessary excitement
|
ACK 057ea0a |
|
Would be good to figure out how to proceed, given that this has been an open PR with ACKs for a while now.
I think this motivation is a bit stale at this point. We both have a rust and c++ client for the existing mining interface under the umbrella of the sv2 project and it seems less pressing to have our own example client. Is there a better motivation that could be provided now? |
|
I do think having our own example client is useful, ideally one that covers a variety of methods and data types. It might help catch regressions and add test coverage for fixes. It's more time consuming to wait for downstream projects to report things and test our fixes. However so far we've put most effort into the functional test
If the Rust client takes off, it might replace most of the c++ client usage due to its easier integration with the rest of sv2. So it could see less maintainance in the future. |
Would a more dedicated test binary implementing a libmultiprocess client for these ipc interfaces that we don't consume ourselves be more suited then? Maybe what is introduced here could be morphed into its own standalone binary using boost test, capable of spawning bitcoin-node, waiting for it to warm up and then executing a few units tests? |
| message(" bitcoin-util ........................ ${BUILD_UTIL}") | ||
| message(" bitcoin-wallet ...................... ${BUILD_WALLET_TOOL}") | ||
| message(" bitcoin-chainstate (experimental) ... ${BUILD_UTIL_CHAINSTATE}") | ||
| message(" bitcoin-mine (experimental) ......... ${bitcoin_daemon_status}") |
There was a problem hiding this comment.
I think this should get its own variable. If I read this correctly, we would ship this as part of a release with the current configuration? Having a CI job that exercises it seems enough to me for its current purposes.
There was a problem hiding this comment.
I don't think this is something that needs to be shipped in releases. Looks like some of the code here is for supporting this bin on Windows, but that doesn't seem needed, given IPC isn't even supported on Windows? Maybe it can just be disabled for Windows entirely?
There was a problem hiding this comment.
It seems similar to bitcoin-chainstate: a demo and test app that we should at least build in dev preset, but probably don't need to ship in a release.
Perhaps it's better to move both of them to an "examples" folder?
Regarding Windows: support is still on the agenda for libmultiprocess: bitcoin-core/libmultiprocess#231
And specifically for mining, my understanding is that people actually use that operating system. Although at the same time so far I haven't had anyone ask for it for sv2-tp (stratum-mining/sv2-tp#7).
There was a problem hiding this comment.
re: #30437 (comment)
Yes good catch. This should get it's own variable and not be part of binary releases. At the time this was written ENABLE_IPC was off by default so it didn't seem like another variable was needed, but definitely makes sense now.
re: #30437 (comment)
I do like the idea of treating this similarly to bitcoin-chainstate since they are both examples and tests. Having an examples folder coud be an alternative to renaming the binary to test_bitcoin-mining as suggested #30437 (comment).
Yes, I think something like this makes sense. Right now there is no IPC client executable in bitcoin core to test coverage of client IPC code or to provide an example of what C++ client would look like. So this executable fills that gap, but maybe it would be better to write it more like a test and less like an example. It would be pretty simple to rename the executable to The idea of using the boost test framework could also make sense, but it would probably require the test program to spawn the node binary itself instead of relying on python to do it. And earlier version of this PR supported that in 3 commits that were dropped to reduce the size of the PR (#30437 (comment)). These changes could be brought back or be a followup. (They could be useful in other cases like a GUI or other wrapper connecting to the node over IPC and starting it if not started.) Also want to note that some changes here may be more generally useful. The first commit 1987f89 was also used in #32297 and #32898 and the second commit f28ac51 was used in #32898. Also 3 commits that were originally in this PR became part of #33201. I guess I generally see the changes here as being useful and I wonder if just renaming the new executable & command to be clear it is for testing would address concerns about it. Exposing this as |
Yes, I think that would be enough (next to making its compilation conditional). I also like the idea of an examples folder, but I don't think the changes here should be conditional on that. |
Allows InitError and InitWarning to be used by other executables beside bitcoind and bitcoin-node.
See src/bitcoin-mine.cpp for usage information. Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
This includes createNewBlock fix from bitcoin#34860 to allow mining blocks at heights <16. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Suggested by Sjors Provoost <sjors@sprovoost.nl> in bitcoin#30437 (comment)
|
|
||
| uint64_t max_tries{std::max<uint64_t>(DEFAULT_MAX_TRIES, args.GetIntArg("-maxtries", DEFAULT_MAX_TRIES))}; | ||
| auto tries_remaining{max_tries}; | ||
| auto block_template{mining->createNewBlock({}, /*cooldown=*/false)}; |
There was a problem hiding this comment.
Mining::createNewBlock() can legally return nullptr when bitcoin-node is shutting down or mining is interrupted.
diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index f02ed3dfba..d33c9e2326 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -133,6 +133,10 @@ MAIN_FUNCTION
uint64_t max_tries{std::max<uint64_t>(DEFAULT_MAX_TRIES, args.GetIntArg("-maxtries", DEFAULT_MAX_TRIES))};
auto tries_remaining{max_tries};
auto block_template{mining->createNewBlock({}, /*cooldown=*/false)};
+ if (!block_template) {
+ tfm::format(std::cerr, "Error: Failed to create block template. bitcoin-node may be shutting down.\n");
+ return EXIT_FAILURE;
+ }
auto block{block_template->getBlock()};
block.hashMerkleRoot = BlockMerkleRoot(block);|
|
||
| auto consensus_params{Params().GetConsensus()}; | ||
|
|
||
| uint64_t max_tries{std::max<uint64_t>(DEFAULT_MAX_TRIES, args.GetIntArg("-maxtries", DEFAULT_MAX_TRIES))}; |
There was a problem hiding this comment.
With std::max(DEFAULT_MAX_TRIES, ...), max_tries can never be lower than DEFAULT_MAX_TRIES, so values like -maxtries=0 or -maxtries=100 are ignored and become 10000. Is this intentional ?
Also, GetIntArg() returns a signed integer, but std::max<uint64_t>() converts it to unsigned. A negative value like -maxtries=-1 can become a huge uint64_t.
diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index f02ed3dfba..d92a45501e 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -130,9 +130,18 @@ MAIN_FUNCTION
auto consensus_params{Params().GetConsensus()};
- uint64_t max_tries{std::max<uint64_t>(DEFAULT_MAX_TRIES, args.GetIntArg("-maxtries", DEFAULT_MAX_TRIES))};
+ int64_t max_tries_arg{args.GetIntArg("-maxtries", DEFAULT_MAX_TRIES)};
+ if (max_tries_arg < 0) {
+ tfm::format(std::cerr, "Error: -maxtries must be non-negative.\n");
+ return EXIT_FAILURE;
+ }
+ uint64_t max_tries{static_cast<uint64_t>(max_tries_arg)};
auto tries_remaining{max_tries};
auto block_template{mining->createNewBlock({}, /*cooldown=*/false)};|
It might be good to emphasize in the PR description that this adds coverage for how a libmultiprocess based client interacts with the node. Our functional tests do not use libmutltiprocess on the client side. We know from experience with sv2-tp vs. the Rust based SRI implementation that these differences can be important.
Yes, I think this should be seen as a combination of a usage example and test coverage. |
Add
bitcoin-mineexecutable to test connecting to thebitcoin-nodeprocess over a unix socket and callinginterface::Miningmethods.This is useful as discussed in #29432 (comment) to work on getting mining code working in an external process.
This PR initially supported spawning a new
bitcoin-nodeprocess if a connection could not be established with an existing process, but it was dropped to simplify the code. A branch building on this PR and adding that support ispr/mine-detach.
This PR is part of the process separation project.