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 could be useful as discussed in #29432 (comment) to work on getting mining code working in an external process.

This PR is a draft because it’s mostly useful for testing. But it might reasonable to merge, as an example of a small program that is able to connect over IPC.

This is based on #30510. The non-base commits are:

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30635 (rpc: add optional blockhash to waitfornewblock by Sjors)
#30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block by Sjors)
#29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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

depends: Update libmultiprocess library for cmake headers target

This update brings in the following changes:

https://github.com/chaincodelabs/libmultiprocess/pull/107 example: Remove manual client adding
https://github.com/chaincodelabs/libmultiprocess/pull/108 doc: Add comments for socket descriptor handling when forking
https://github.com/chaincodelabs/libmultiprocess/pull/109 example: Add missing thread.join() call so example can exit cleanly
https://github.com/chaincodelabs/libmultiprocess/pull/110 cmake: add target_capnp_sources headers target

72689b19db

build: Make bitcoin_ipc_test depend on bitcoin_ipc

This change is needed to allow generated capnp code in src/ipc/capnp/ to be
used in unit tests for better test coverage in upcoming commits.

89ae6b35de

multiprocess: update common-types.h to use C++20 concepts

Idea came from review comment by ion-
https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757372497

974e041b79

multiprocess: Add serialization code for CTransaction

Add support for passing CTransaction and CTransactionRef types to IPC
functions.

These types can't be passed currently because IPC serialization code currently
only supports deserializing types that have an Unserialize() method, which
CTransaction does not, because it is supposed to represent immutable
transactions. Work around this by adding a CustomReadField overload that will
call CTransaction's deserialize_type constructor.

These types also can't be passed currently because serializing transactions
requires TransactionSerParams to be set. Fix this by setting TX_WITH_WITNESS as
default serialization parameters for IPC code.

57f81aca2d

multiprocess: Add serialization code for vector<char> 246fd7faae

multiprocess: Add IPC wrapper for Mining interface 6c10f6c460

multiprocess: Add serialization code for BlockValidationState

Co-authored-by: TheCharlatan <seb.kung@gmail.com>

00a82f4500

doc: multiprocess documentation improvements

Most improvements suggested by stickies-v <stickies-v@protonmail.com>
https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1800375604

Omission of CustomReadMessage and CustomBuildMessage was noticed by
TheCharlatan in
https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708996040 and is
fixed here as well.

Co-authored-by: stickies-v <stickies-v@protonmail.com>

d59f5a31a1

Merge remote-tracking branch 'origin/pull/30510/head' 49e42288cf

interface_ui: move from node to common library

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

dad7468be0

multiprocess: Add bitcoin-mine test program

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

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

1ce0851001

multiprocess: Expand mining interface (part 1)

Add interface changes from

https://github.com/bitcoin/bitcoin/pull/30409
https://github.com/bitcoin/bitcoin/pull/30440

This commit changes the C++ Mining interface without changing the corresponding
Capn'Proto interface. The capnp interface is updated the next commit.

54612bd402

multiprocess: Expand mining interface (part 2)

Add interface changes from

https://github.com/bitcoin/bitcoin/pull/30409
https://github.com/bitcoin/bitcoin/pull/30440

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.

7b9aa0b6eb

ryanofsky force-pushed on Sep 19, 2024

DrahtBot removed the label Needs rebase on Sep 19, 2024

multiprocess: add bitcoin-mine test program #30437

Code Coverage

Reviews

Conflicts