multiprocess: add bitcoin-mine test program

ryanofsky commented at 10:37 am on July 12, 2024: contributor

Add bitcoin-mine executable to test connecting to the bitcoin-node process over a unix socket and calling interface::Mining methods.

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-node process 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 is pr/mine-detach.

This PR is part of the process separation project.

DrahtBot commented at 10:37 am on July 12, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot renamed this:
~~multiprocess: add bitcoin-mine test program~~
multiprocess: add bitcoin-mine test program
on Jul 12, 2024

Sjors commented at 11:47 am on July 12, 2024: member

Thanks. I’ll try to get the Mining interface in good shape as well, which this can then be rebased on:

#30409 (first commit)
#30356
#30440
#30443

ryanofsky force-pushed on Jul 12, 2024

DrahtBot commented at 12:32 pm on July 12, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27368335663

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Jul 12, 2024

ryanofsky commented at 12:35 pm on July 12, 2024: contributor

Updated 8e9367371b47d7a88136a80d0cb091e1f32d2a4e -> 3c0a13c9a56a11d795b243cedb3b00a71a81042c (pr/mine.1 -> pr/mine.2, compare) filling in missing serialization for CBlockTemplate and BlockValidationState types, also fixing link errors for test programs that were causing CI tasks to fail.

ryanofsky commented at 12:44 pm on July 12, 2024: contributor

I’ll work on dropping the spawn functionality from this PR next, and just having the bitcoin-mine program print an error when it can’t connect to the node socket, instead of starting the node itself. Sjors suggested this in #29432 (comment):

If it helps getting IPC stuff merged quicker, it’s fine by me if the miner can’t spin up a node process (see e.g. Sjors@b6dfa5f)

This would simplify the PR by allowing 3 commits to be dropped:

Other things I’d like to do:

Write unit tests to cover the new code
Split up the last commit
Fix remaining CI errors
Add -ipconnect option to bitcoin-mine to allow specifying custom path for unix socket.

Sjors commented at 1:40 pm on July 12, 2024: member

filling in missing serialization for CBlockTemplate

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.

ryanofsky commented at 2:19 pm on July 12, 2024: contributor

filling in missing serialization for CBlockTemplate

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 CTransaction or CBlock objects (because CTransaction does not have an Unserialize method, and also requires a version parameter to be set).

Sjors commented at 2:25 pm on July 12, 2024: member

I’ll certainly need CBlock. It’s probably good to support CTransaction as well, as it might allow for more efficiently fetching only missing transactions in a template update, but that would be a future optimisation.

Sjors commented at 3:41 pm on July 12, 2024: member

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.

Sjors commented at 10:18 am on July 13, 2024: member

I wrote:

I’ll expand the demo app to simulate all the steps a Template Provider would do

Once that works the next step for me would be to open an alternative to #29432 which runs the Template Provider in bitcoin-tp. I might wait with that until we ship a release with this interface enabled on the node side, ideally in bitcoind and bitcoin-qt rather than in seperate bitcoin-node and bitcoin-gui binaries.

I could then modify the guix build to only produce bitcoin-tp. This effectively turns it into a separate project that just happens to have access to the entire Bitcoin Core codebase.

That way the user can simply install and run a recent Bitcoin Core release in the manner they’re familiar with. They would install bitcoin-tp separately and it should just work(tm), at most having to add a line to bitcoin.conf to turn IPC on.

If later on we can also turn libbitcoin_net into a library (see #30350), I could look into completely separating the codebase (I’d probably copy-paste the Bitcoin Core code and strip everything out I don’t need, rather starting a new c++ project and build system from scratch). E.g. it could pull in libbitcoin_net, libbitcoin_util and whatever else it needs, add its own Transport subclass. This lets me reuse #29346, #30315 and #30332, but without having to merge them into Bitcoin Core itself.

This would be in parallel to the alternative approach of writing bitcoin-tp from scratch in rust reusing SRI code.

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).

ryanofsky force-pushed on Jul 16, 2024

ryanofsky commented at 7:24 pm on July 16, 2024: contributor

I simplified PR as suggested so bitcoin-mine no longer spawns bitcoin-node if it can’t connect to it. Instead it just prints an error message and suggests a command line to run bitcoin-node manually.

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 3c0a13c9a56a11d795b243cedb3b00a71a81042c -> 4e1a4342f3b2a857e3211587cd14e36704197483 (pr/mine.2 -> pr/mine.3, compare) with those changes.

For reference, the code that was dropped from this PR allowing bitcoin-mine to spawn a bitcoin-node process is in commits:

09621da38291fbfc22d86ea98e4499b3aa2c9976 multiprocess: Add IPC spawnProcess detach option
3d23332240f103f6c639662d7c732a52adf5d13f multiprocess: Add IPC attach and detach methods
3c112629d99d64b1c7719cd432d13fb2e8ec8f4b multiprocess: Allow bitcoin-mine to spawn bitcoin-node

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.

DrahtBot removed the label CI failed on Jul 16, 2024

in src/bitcoin-mine.cpp:15 in 8a20be71f0 outdated

10+#include <common/args.h>
11+#include <common/system.h>
12+#include <compat/compat.h>
13+#include <init/common.h>
14+#include <interfaces/init.h>
15+#include <interfaces/ipc.h>

Sjors commented at 12:19 pm on July 17, 2024:

8a20be71f07cfe0129107cb8effb2490b79639bf: #include <logging.h>

Sjors commented at 1:16 pm on July 17, 2024: member

If you split the capnp additions out of 4e1a4342f3b2a857e3211587cd14e36704197483 it’s a bit easier for me to combine this PR with #29432 (by omitting the interface changes commit here or there).

in src/interfaces/mining.h:76 in 4e1a4342f3 outdated

71@@ -46,6 +72,9 @@ class Mining
72      */
73     virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
74 
75+    // Implemented in https://github.com/bitcoin/bitcoin/pull/30356
76+    virtual std::unique_ptr<BlockTemplate> createNewBlock2(const CScript& script_pub_key, const node::BlockCreateOptions& options={}) { return {}; }

Sjors commented at 1:29 pm on July 17, 2024:

Is it possible to rename this to createNewBlock and drop the one above? Or is there an inconsistency between my PRs?

ryanofsky commented at 2:47 pm on July 17, 2024:

re: #30437 (review)

Is it possible to rename this to createNewBlock and drop the one above? Or is there an inconsistency between my PRs?

It’s not really possible without making the PR bigger because mining code relies on createNewBlock returning CBlockTemplate not BlockTemplate, and I want mining code to work and CI to pass.

The purpose of createNewBlock2 is just to make sure serialization works when createNewBlock is changed to return BlockTemplate.

Sjors commented at 3:34 pm on July 17, 2024:

Responded in #30437 (comment)

Sjors commented at 2:30 pm on July 17, 2024: member

Can you add support for -loglevel? Typically when testing Stratum v2 stuff I’ll run with -debug=sv2 -loglevel=sv2:trace.

Sjors commented at 2:32 pm on July 17, 2024: member

While trying to start the Template Provider from bitoin-mine it crashes as soon as it tried any libsecp operation, at this line: ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));

Oh that’s a matter of doing ECC_Context ecc_context{};.

ryanofsky commented at 2:57 pm on July 17, 2024: contributor

re: #30437 (comment)

If you split the capnp additions out of 4e1a434 it’s a bit easier for me to combine this PR with #29432 (by omitting the interface changes commit here or there).

Hmm, what specifically would you like me to change here? This PR will be pretty difficult for me maintain if it based on other PRs, so it is just based on master.

Sjors commented at 3:29 pm on July 17, 2024: member

Hmm, what specifically would you like me to change here?

Just to have the changes in src/ipc/capnp be their own commit. I need those in https://github.com/Sjors/bitcoin/pull/48, while at the same I need to avoid including the rest of 4e1a4342f3b2a857e3211587cd14e36704197483.

The purpose of createNewBlock2 is just to make sure serialization works when createNewBlock is changed to return BlockTemplate.

In that case I’ll just add a commit to https://github.com/Sjors/bitcoin/pull/48 that renames createNewBlock2 to createNewBlock. That shouldn’t be a problem to keep rebased.

ryanofsky force-pushed on Jul 17, 2024

ryanofsky commented at 3:41 pm on July 17, 2024: contributor

Updated 4e1a4342f3b2a857e3211587cd14e36704197483 -> e1ae38f4bbcdb1b3fa5e1eab1abfa9cf2232798c (pr/mine.3 -> pr/mine.4, compare) adding several unit tests, and also starting to split up the “Add bitcoin-mine test program” commit by moving CTransaction serialization support to a new commit.

ryanofsky force-pushed on Jul 17, 2024

DrahtBot commented at 3:53 pm on July 17, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27571383752

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Jul 17, 2024

ryanofsky commented at 3:53 pm on July 17, 2024: contributor

Updated e1ae38f4bbcdb1b3fa5e1eab1abfa9cf2232798c -> e1fa475a5becca0083f59d7cafac9bc67c2f4f1c (pr/mine.4 -> pr/mine.5, compare) adding support for -loglevel and other logging parameters as suggested #30437 (comment)

ryanofsky force-pushed on Jul 17, 2024

ryanofsky commented at 4:27 pm on July 17, 2024: contributor

Updated e1fa475a5becca0083f59d7cafac9bc67c2f4f1c -> bdd68d5bca483071ca0729e6f0d2d71706ad21e7 (pr/mine.5 -> pr/mine.6, compare) fixing lint error and splitting up “Expand mining interface” commit into two parts.

re: #30437 (comment)

Just to have the changes in src/ipc/capnp be their own commit. I need those in Sjors#48, while at the same I need to avoid including the rest of 4e1a434.

Makes sense. The latest push splits up the “Expand mining interface” commit into two parts so it should be easier to cherry-pick the part you need now.

Sjors referenced this in commit 8c593f3ec4 on Jul 17, 2024

DrahtBot removed the label CI failed on Jul 17, 2024

Sjors commented at 8:39 am on July 18, 2024: member

I ran into:

0make -j17 src/bitcoin-mine
1make: *** No rule to make target `src/bitcoin-mine'.  Stop.

Fixed with https://github.com/Sjors/bitcoin/commit/68979b8f4c4e069bc9b6d19679ef6832839ceae4

Sjors referenced this in commit 97f3230c30 on Jul 18, 2024

Sjors referenced this in commit b5044653b2 on Jul 18, 2024

DrahtBot added the label Needs rebase on Jul 18, 2024

ryanofsky force-pushed on Jul 18, 2024

ryanofsky commented at 8:41 pm on July 18, 2024: contributor

Rebased bdd68d5bca483071ca0729e6f0d2d71706ad21e7 -> fba04d7756c77ed9371c5dc90d3bebc9aa767f4b (pr/mine.6 -> pr/mine.7, compare) due to conflict with #30356

DrahtBot removed the label Needs rebase on Jul 18, 2024

Sjors referenced this in commit f951af069e on Jul 19, 2024

Sjors commented at 10:40 am on July 22, 2024: member

If you want to simplify this PR a bit further, you can drop waitFeesChanged, as well as getCoinbaseMerklePath() and submitSolution from the BlockTemplate interface.

I moved those out of the PRs on the repo and into my own fork (https://github.com/Sjors/bitcoin/pull/52 and https://github.com/Sjors/bitcoin/pull/53). I can apply the required changes to ipc/capnp/mining.capnp myself in https://github.com/Sjors/bitcoin/pull/48; that’s much easier than I expected.

That should remove any friction from getting the mining interface changes from #30440 and #30409 merged.

We should then aim to get everything merged in the PR, maybe with the exception of multiprocess: Add bitcoin-mine test program.

By the time multiprocess is merged into Bitcoin Core (unlikely to make it into v28) I’ll make PRs for the last missing bits. Those should be more fleshed out by then.

ryanofsky commented at 2:56 pm on July 23, 2024: contributor

re: #30437 (comment)

We should then aim to get everything merged in the PR, maybe with the exception of multiprocess: Add bitcoin-mine test program.

To separate essential and nonessential parts of this PR, I created #30509 and #30510 which I think contain all the needed functionality from the PR, and I also broke the changes down and wrote tests for them. I will rebase this PR on top of those two.

DrahtBot added the label Needs rebase on Jul 24, 2024

ryanofsky force-pushed on Jul 24, 2024

ryanofsky commented at 10:17 pm on July 24, 2024: contributor

Rebased fba04d7756c77ed9371c5dc90d3bebc9aa767f4b -> ec5f45b9186184cac9c64ed08b8b8cc596e301a3 (pr/mine.7 -> pr/mine.8, compare), moving most code that was here into base PRs #30509 and #30510. I also followed up various review suggestions above like fixing the top-level make target and dropping unneeded parts of the mining interface.

DrahtBot removed the label Needs rebase on Jul 24, 2024

Sjors referenced this in commit 6a9b6c51e3 on Aug 13, 2024

Sjors referenced this in commit 81b82a4146 on Aug 13, 2024

hebasto added the label Needs CMake port on Aug 16, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

maflcko removed the label Needs CMake port on Sep 3, 2024

Sjors commented at 8:43 am on September 6, 2024: member

Can you rebase this for CMake?

ryanofsky force-pushed on Sep 6, 2024

ryanofsky commented at 3:20 pm on September 6, 2024: contributor

Rebased ec5f45b9186184cac9c64ed08b8b8cc596e301a3 -> 697ba11c9496e3a2aaf6fc3ba64fd39a028109c2 (pr/mine.8 -> pr/mine.9, compare) just updating for cmake

DrahtBot removed the label Needs rebase on Sep 6, 2024

achow101 referenced this in commit df3f63ccfa on Sep 9, 2024

DrahtBot added the label Needs rebase on Sep 16, 2024

ryanofsky force-pushed on Sep 19, 2024

DrahtBot removed the label Needs rebase on Sep 19, 2024

DrahtBot added the label Needs rebase on Sep 23, 2024

achow101 referenced this in commit 65f6e7078b on Sep 25, 2024

interface_ui: move from node to common library

Allows InitError and InitWarning to be used by other executables beside
bitcoind and bitcoin-node.

aec7e7d897

multiprocess: Add bitcoin-mine test program

See src/bitcoin-mine.cpp for usage information.

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

2f9c2f4c6d

ryanofsky force-pushed on Sep 26, 2024

ryanofsky commented at 12:49 pm on September 26, 2024: contributor

Rebased 7b9aa0b6eb1126cb32da91f3c9d2df12325aa90c -> 2227afac0372358287fb879f3b0bd07fd653f3f8 (pr/mine.10 -> pr/mine.11, compare) after base PR’s #30509 and #30510 were merged.

Marking ready for review

ryanofsky marked this as ready for review on Sep 26, 2024

DrahtBot removed the label Needs rebase on Sep 26, 2024

in src/bitcoin-mine.cpp:120 in 2227afac03 outdated

107+
108+    auto tip{mining->getTip()};
109+    if (tip) {
110+        tfm::format(std::cout, "Tip hash is %s.\n", tip->hash.ToString());
111+    } else {
112+        tfm::format(std::cout, "Tip hash is null.\n");

Sjors commented at 8:49 am on October 1, 2024:

I can’t reach this condition. Even when I spin up a mainnet node and immediately start bitcoin-mine it just seems to wait a bit and then return a hash.

Maybe we should simplify the getTip() interface to not return an optional. Though I’m not sure if it’s completely impossible to hit this.

Sjors commented at 8:50 am on October 1, 2024: member

tACK 2227afac0372358287fb879f3b0bd07fd653f3f8

Except I noticed that bitcoin-mine -debug=ipc doesn’t print anything, which is surprising.

Tested the standalone application. I plan to do another rebase of https://github.com/Sjors/bitcoin/pull/48 on top of this later (which turns bitcoin-mine into a Template Provider).

ryanofsky force-pushed on Oct 1, 2024

ryanofsky commented at 4:22 pm on October 1, 2024: contributor

re: #30437 (comment)

Thanks for the review! Fixed the problem with -debug=ipc now. This was missing a call to parse -debug arguments.

Updated 2227afac0372358287fb879f3b0bd07fd653f3f8 -> 2f9c2f4c6dcf8c45514e99d1018301aba61940fc (pr/mine.11 -> pr/mine.12, compare)

Sjors commented at 4:33 pm on October 1, 2024: member

ACK 2f9c2f4c6dcf8c45514e99d1018301aba61940fc

Logging works now.

Sjors commented at 5:23 pm on October 4, 2024: member

In order to get bitcoin-mine in the guix build (once it supports multiprocess) I think you need to move set(installable_targets) up in the file, and also add bitcoin-mine to the list in foreach(target IN ITEMS bitcoind in Maintenance.cmake. Though I’m not sure why that doesn’t use installable_targets, cc @hebasto.

Maybe add a placeholder man page for gen-manpages.py.

in src/bitcoin-mine.cpp:80 in 2f9c2f4c6d

75+    }
76+    if (!CheckDataDirOption(args)) {
77+        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", args.GetArg("-datadir", ""));
78+        return EXIT_FAILURE;
79+    }
80+    SelectParams(args.GetChainType());

Sjors commented at 6:02 pm on October 4, 2024:

Can you move this above AddArgs? In https://github.com/Sjors/bitcoin/pull/48 I’m adding some argument documentation that need accsess to BaseParams.

hebasto commented at 7:41 am on October 7, 2024: member

In order to get bitcoin-mine in the guix build (once it supports multiprocess) I think you need to move set(installable_targets) up in the file, and also add bitcoin-mine to the list in foreach(target IN ITEMS bitcoind in Maintenance.cmake.

I don’t think set(installable_targets) needs to be moved up because it already precedes all subsequent use cases. Or did you mean readability improving?

Though I’m not sure why that doesn’t use installable_targets

There are two local installable_targets variables: one scoped to the src directory and another to src/qt. This separation is necessary to be able to install GUI targets as part of the “GUI” component. On the other hand, the list of targets in the add_maintenance_targets() function is global and intended to hold targets built in the Guix script to run symbol-check.py and security-check.py on them.

Sjors commented at 8:00 am on October 7, 2024: member

@hebasto I forgot that I moved target_link_libraries(bitcoin-mine up myself in https://github.com/Sjors/bitcoin/pull/48/commits/254cf216b5e312297ec34a4ee58256d21ab8571e. So no need to change set(installable_targets) here.

DrahtBot added the label CI failed on Oct 13, 2024

DrahtBot commented at 7:38 pm on October 13, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30925928696

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

multiprocess: add bitcoin-mine test program #30437

Code Coverage

Reviews

Conflicts