multiprocess: add bitcoin-mine test program #30437

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/mine changing 11 files +231 −12
  1. 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.

  2. 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 & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30437.

    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:

    • #31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
    • #31161 (cmake: Set top-level target output locations by hebasto)
    • #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.

  3. DrahtBot renamed this:
    multiprocess: add bitcoin-mine test program 
    multiprocess: add bitcoin-mine test program
    on Jul 12, 2024
  4. 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:

  5. ryanofsky force-pushed on Jul 12, 2024
  6. 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.

  7. DrahtBot added the label CI failed on Jul 12, 2024
  8. 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.
  9. 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.
  10. 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.

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

  12. 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.
  13. 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.

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

  15. ryanofsky force-pushed on Jul 16, 2024
  16. 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.

  17. DrahtBot removed the label CI failed on Jul 16, 2024
  18. 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>
  19. 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).
  20. 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)
  21. 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.
  22. 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{};.

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

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

  25. ryanofsky force-pushed on Jul 17, 2024
  26. 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.
  27. ryanofsky force-pushed on Jul 17, 2024
  28. 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.

  29. DrahtBot added the label CI failed on Jul 17, 2024
  30. 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)
  31. ryanofsky force-pushed on Jul 17, 2024
  32. 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.

  33. Sjors referenced this in commit 8c593f3ec4 on Jul 17, 2024
  34. DrahtBot removed the label CI failed on Jul 17, 2024
  35. 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

  36. Sjors referenced this in commit 97f3230c30 on Jul 18, 2024
  37. Sjors referenced this in commit b5044653b2 on Jul 18, 2024
  38. DrahtBot added the label Needs rebase on Jul 18, 2024
  39. ryanofsky force-pushed on Jul 18, 2024
  40. 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
  41. DrahtBot removed the label Needs rebase on Jul 18, 2024
  42. Sjors referenced this in commit f951af069e on Jul 19, 2024
  43. 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.

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

  45. DrahtBot added the label Needs rebase on Jul 24, 2024
  46. ryanofsky force-pushed on Jul 24, 2024
  47. 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.
  48. DrahtBot removed the label Needs rebase on Jul 24, 2024
  49. Sjors referenced this in commit 6a9b6c51e3 on Aug 13, 2024
  50. Sjors referenced this in commit 81b82a4146 on Aug 13, 2024
  51. hebasto added the label Needs CMake port on Aug 16, 2024
  52. DrahtBot added the label Needs rebase on Sep 2, 2024
  53. maflcko removed the label Needs CMake port on Sep 3, 2024
  54. Sjors commented at 8:43 am on September 6, 2024: member
    Can you rebase this for CMake?
  55. ryanofsky force-pushed on Sep 6, 2024
  56. 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
  57. DrahtBot removed the label Needs rebase on Sep 6, 2024
  58. achow101 referenced this in commit df3f63ccfa on Sep 9, 2024
  59. DrahtBot added the label Needs rebase on Sep 16, 2024
  60. ryanofsky force-pushed on Sep 19, 2024
  61. DrahtBot removed the label Needs rebase on Sep 19, 2024
  62. DrahtBot added the label Needs rebase on Sep 23, 2024
  63. achow101 referenced this in commit 65f6e7078b on Sep 25, 2024
  64. ryanofsky force-pushed on Sep 26, 2024
  65. 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

  66. ryanofsky marked this as ready for review on Sep 26, 2024
  67. DrahtBot removed the label Needs rebase on Sep 26, 2024
  68. 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.

    ryanofsky commented at 4:21 pm on October 1, 2024:

    re: #30437 (review)

    It would be surprising if current code couldn’t hit this condition at all, though not surprising if this condition is just hard to hit because the tip is set to the genesis block very quickly on startup. Maybe could experiment with starting the node with reindex options or calling bitcoin-mine in a loop if it’s worth debugging.

    It would seem reasonable to not return optional though if getTip will not be available to call before the tip is connected or will wait for it.

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

  70. ryanofsky force-pushed on Oct 1, 2024
  71. 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)

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

    ACK 2f9c2f4c6dcf8c45514e99d1018301aba61940fc

    Logging works now.

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

  74. in src/bitcoin-mine.cpp:80 in 2f9c2f4c6d outdated 
    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.
    Sjors commented at 2:22 pm on November 25, 2024:
    Never mind, this causes more headaches that it solves. I need to use defaultBaseParams, testnet4BaseParams, etc instead.
  75. 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.

  76. 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.
  77. DrahtBot added the label CI failed on Oct 13, 2024
  78. 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.

  79. in src/bitcoin-mine.cpp:5 in 2f9c2f4c6d outdated 
    0@@ -0,0 +1,124 @@
1+// Copyright (c) 2024 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <config/bitcoin-config.h> // IWYU pragma: keep
    Sjors commented at 1:55 pm on November 25, 2024:
    2f9c2f4c6dcf8c45514e99d1018301aba61940fc: silent merge conflict with #30937.
  80. interface_ui: move from node to common library 
    Allows InitError and InitWarning to be used by other executables beside
bitcoind and bitcoin-node.
    1a4ca89fad
  81. multiprocess: Add bitcoin-mine test program 
    See src/bitcoin-mine.cpp for usage information.

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
    8fab41611a
  82. test: add interface_ipc_mining.py test calling bitcoin-mine 99f2f335b6
  83. in src/bitcoin-mine.cpp:65 in 2f9c2f4c6d outdated 
    60+    if (!args.ReadConfigFiles(error_message, true)) {
61+        tfm::format(std::cerr, "Error reading config files: %s\n", error_message);
62+        return EXIT_FAILURE;
63+    }
64+    if (HelpRequested(args) || args.IsArgSet("-version")) {
65+        std::string output{strprintf("%s bitcoin-mine version", PACKAGE_NAME) + " " + FormatFullVersion() + "\n"};
    Sjors commented at 1:58 pm on November 25, 2024:
    2f9c2f4c6dcf8c45514e99d1018301aba61940fc: PACKAGE_NAME changed in #31042
  84. ryanofsky force-pushed on Nov 27, 2024
  85. ryanofsky commented at 4:19 am on November 27, 2024: contributor

    Rebased 2f9c2f4c6dcf8c45514e99d1018301aba61940fc -> 1f18eefe1e5bc3a97f5f6c9637a3a193e2c2f22e (pr/mine.12 -> pr/mine.13, compare) fixing silent merge conflicts and adding a functional test and updating gen-manpages

    re: #30437 (comment) re: #30437 (comment) re: #30437 (comment)

    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.

    Not sure I understood the outcome of this cmake discussion, but it seems like installable_targets and Maintenance.cmake changes might not be needed? Happy to add any changes here that would help.

  86. ryanofsky force-pushed on Nov 27, 2024
  87. ryanofsky commented at 4:59 pm on November 27, 2024: contributor
    Updated 1f18eefe1e5bc3a97f5f6c9637a3a193e2c2f22e -> 27072547bb22cdd2080d1014eba9c30bc9d47650 (pr/mine.13 -> pr/mine.14, compare) to fix CI errors from skip_if_no_multiprocess check being broken and from default testdir path exceeding maximum socket length
  88. DrahtBot removed the label CI failed on Nov 27, 2024
  89. Sjors commented at 9:05 am on November 28, 2024: member

    I rebased https://github.com/Sjors/bitcoin/pull/48 to use the latest version of this PR, to figure out what I need for cmake. The only thing I still suggest is:

    • add bitcoin-mine to the foreach(target IN ITEMS list in cmake/module/Maintenance.cmake. IIUC that will cause “the Guix script to run symbol-check.py and security-check.py on them”. Even though this PR doesn’t add bitcoin-mine to the guix build artefacts, it seems better to add it to this list now so we don’t forget.

    ACK 27072547bb22cdd2080d1014eba9c30bc9d47650

    I tested that gen-manpages.py generates something useful (man doc/man/bitcoin-mine.1).

    The new functional test is nice.

  90. fanquake commented at 9:09 am on November 28, 2024: member

    it seems better to add it to this list now so we don’t forget.

    Nack. We can add things to Guix and the release process when they are actually part of a release.

  91. Sjors commented at 5:15 pm on November 29, 2024: member
    In light of https://github.com/Sjors/bitcoin/issues/71#issuecomment-2508157740 I think bitcoin-mine should call all interface methods. But this can wait for a followup, maybe by myself.
  92. DrahtBot added the label Needs rebase on Dec 2, 2024
  93. ryanofsky force-pushed on Dec 3, 2024
  94. ryanofsky commented at 9:01 pm on December 3, 2024: contributor
    Updated 27072547bb22cdd2080d1014eba9c30bc9d47650 -> 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b (pr/mine.14 -> pr/mine.15, compare) just moving a line to avoid a merge conflict.
  95. DrahtBot removed the label Needs rebase on Dec 3, 2024
  96. Sjors commented at 7:18 am on December 16, 2024: member
    re-utACK 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b


ryanofsky DrahtBot Sjors hebasto fanquake

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me