Add waitFeesChanged() to Mining interface #31003

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2024/07/mining-fees-changed changing 3 files +58 −4
  1. Sjors commented at 6:15 pm on September 30, 2024: member

    The Stratum v2 protocol allows pushing out templates as fees in the mempool increase. This interface lets us know when it’s time to do so.

    I split the implementation into two parts because I’m not sure if we should include the second commit now, or wait until cluster mempool #30289 is merged.

    The first commit contains a mock implementation very similiar to how longpolling in getblocktemplate works. It calls getTransactionsUpdated once per second. This returns true anytime a transaction is added to the mempool, even if it has no impact on fees in the best block.

    The second commit creates a new block template so it can actually measure the fees. It’s slightly faster getNewBlock() because it skips verification.

    On my 2019 Intel MacBook Pro it takes about 20ms to generate a block template. The waitFeesChanged() waits 1 second between each CreateNewBlock call so as to not burden the node too much. But on (very) low end hardware this may still be problematic.

    The second commit does not change the interface, so it could be left out if people prefer that. I’m not sure what performance increase to expect from cluster mempool.

    The interface intentionally does not return the resulting template, so that the implementation can be optimized further. Downside is that the caller needs to do another getNewBlock().

    The changes here can use to make longpolling getblocktemplate more efficient, by only returning the RPC call when template fees actually increased. However this PR does not touch that RPC code.

  2. Introduce waitFeesChanged() mining interface a87589abfd
  3. Have waitFeesChanged() frequently make a template 9a723c784a
  4. DrahtBot commented at 6:15 pm on September 30, 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/31003.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK danielabrozzoni, ryanofsky
    Approach ACK tdb3

    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:

    • #31196 (Prune mining interface by Sjors)

    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.

  5. Sjors renamed this:
    Introduce waitFeesChanged() mining interface
    Add waitFeesChanged() to Mining interface
    on Sep 30, 2024
  6. in src/ipc/capnp/mining.capnp:20 in a87589abfd outdated 
    16@@ -17,10 +17,11 @@ interface Mining $Proxy.wrap("interfaces::Mining") {
17     isInitialBlockDownload @1 (context :Proxy.Context) -> (result: Bool);
18     getTip @2 (context :Proxy.Context) -> (result: Common.BlockRef, hasResult: Bool);
19     waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef);
20-    createNewBlock @4 (scriptPubKey: Data, options: BlockCreateOptions) -> (result: BlockTemplate);
21-    processNewBlock @5 (context :Proxy.Context, block: Data) -> (newBlock: Bool, result: Bool);
22-    getTransactionsUpdated @6 (context :Proxy.Context) -> (result: UInt32);
23-    testBlockValidity @7 (context :Proxy.Context, block: Data, checkMerkleRoot: Bool) -> (state: BlockValidationState, result: Bool);
24+    waitFeesChanged @4 (context :Proxy.Context, currentTip: Data, feeThreshold: Int64, options: BlockCreateOptions, timeout: Float64) -> (result: Bool);
    Sjors commented at 6:31 pm on September 30, 2024:
    a87589abfde4f2f1dfba0f3f1f5745f7acfef365: @ryanofsky or should I make this @9 and leave the other numbers untouched?
    ryanofsky commented at 7:39 pm on September 30, 2024:

    a87589a: @ryanofsky or should I make this @9 and leave the other numbers untouched?

    You can but it shouldn’t matter, as long as nothing outside the build is depending on the interface. So far I’ve just renumbered everything like your current PR does. I even have a hacky renumbering script capnp-renum, that I pipe iinto from vim to renumber structs and interfaces:

     0#!/usr/bin/env python
 1
 2import sys
 3import re
 4
 5next = 0 if len(sys.argv) < 2 else int(sys.argv[1])
 6
 7def f(m):
 8  global next
 9  next += 1
10  return "@" + str(next - 1)
11
12sys.stdout.write(re.sub(r"@\d+", f, sys.stdin.read()))
    Sjors commented at 7:22 am on October 1, 2024:
    Ok, well the numbering here doesn’t conflict with #30955 and there’s no other proposed interface change pending. So I’ll keep the renumbering here.
  7. danielabrozzoni commented at 1:37 pm on October 1, 2024: contributor
    Code review ACK 9a723c784ad5170b1e01e91f018e5794a51dd038 - code looks good to me, I don’t have an opinion on merging both commits vs waiting for cluster mempool for the second one.
  8. in src/interfaces/mining.h:83 in a87589abfd outdated 
    78+     * @param[in] fee_threshold how far total fees for the next block should rise (currently ignored)
79+     * @param[in] options       options for creating the block, should match those
80+     *                          passed to createNewBlock (currently ignored)
81+     *
82+     * @returns true if fees increased, false if a new tip arrives or the timeout occurs
83+     */
    tdb3 commented at 2:48 pm on October 5, 2024:
    non-blocking nitty nit: Could include all function arguments as params in the doxygen comments. Feel free to ignore.
  9. in src/interfaces/mining.h:82 in a87589abfd outdated 
    77+     * @param[in] current_tip   block hash that the most recent template builds on
78+     * @param[in] fee_threshold how far total fees for the next block should rise (currently ignored)
79+     * @param[in] options       options for creating the block, should match those
80+     *                          passed to createNewBlock (currently ignored)
81+     *
82+     * @returns true if fees increased, false if a new tip arrives or the timeout occurs
    tdb3 commented at 2:51 pm on October 5, 2024:

    calls getTransactionsUpdated once per second. This returns true anytime a transaction is added to the mempool, even if it has no impact on fees in the best block.

    non-blocking nit: The description in the first commit could be updated to be more accurate (so even if the 2nd commit isn’t included, the interface is documented accurately). Maybe just add a note, e.g. (current interim behavior is to return true for any mempool change, not just fee increased).

  10. in src/node/interfaces.cpp:962 in a87589abfd outdated 
    956@@ -957,6 +957,35 @@ class MinerImpl : public Mining
957         return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
958     }
959 
960+    bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const BlockCreateOptions& options, MillisecondsDouble timeout) override
961+    {
962+        if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
    ryanofsky commented at 4:26 pm on October 8, 2024:

    In commit “Introduce waitFeesChanged() mining interface” (a87589abfde4f2f1dfba0f3f1f5745f7acfef365)

    Would suggest deleting this. I don’t think this can do anything because the value passed to the sleep function is already capped by now + tick

  11. in src/node/interfaces.cpp:984 in 9a723c784a 
    979@@ -977,9 +980,18 @@ class MinerImpl : public Mining
980                 return false;
981             }
982 
983-            // TODO: when cluster mempool is available, actually calculate
984-            // fees for the next block. This is currently too expensive.
985-            if (context()->mempool->GetTransactionsUpdated() > last_mempool_update) return true;
986+            // Did anything change at all?
987+            if (context()->mempool->GetTransactionsUpdated() != last_mempool_update) {
    ryanofsky commented at 4:59 pm on October 8, 2024:

    In commit “Have waitFeesChanged() frequently make a template” (9a723c784ad5170b1e01e91f018e5794a51dd038)

    It seems like there is a bug here (also present in the previous commit) where last_mempool_update variable is not updated during the loop, so once the value changes a single time, this check will always be true, and the same block could be assembled wastefully every tick.

    Also, it seems like there is also a race condition in the opposite direction that could lead to stale results, where a client could call waitFeesChanged at the same time as a transaction is added to the mempool reaching the fee_threshold. In this case there is a race because if GetTransactionsUpdated on line 967 is called too late, it willl cause this function to sleep and wait for more transactions even though the fees are already over the threshold. Giving last_mempool_update an initial value of 0 would avoid this race.

    Separately it also looks like this will throw a std::bad_optional_access exception if getTip returns nullopt.

    Would suggest the following simplifications and fixes to make this function have more reliable behavior.

     0--- a/src/node/interfaces.cpp
 1+++ b/src/node/interfaces.cpp
 2@@ -959,29 +959,22 @@ public:
 3 
 4     bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const BlockCreateOptions& options, MillisecondsDouble timeout) override
 5     {
 6-        if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
 7         auto now{std::chrono::steady_clock::now()};
 8         const auto deadline = now + timeout;
 9         const MillisecondsDouble tick{1000};
10 
11-        unsigned int last_mempool_update{context()->mempool->GetTransactionsUpdated()};
12-
13         BlockAssembler::Options assemble_options{options};
14         ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
15         // It's not necessary to verify the template, since we don't return it.
16         // This is also faster.
17         assemble_options.test_block_validity = false;
18 
19-        while (!chainman().m_interrupt) {
20-            now = std::chrono::steady_clock::now();
21-            if (now >= deadline) break;
22-
23-            if (getTip().value().hash != current_tip) {
24-                return false;
25-            }
26-
27+        unsigned int last_mempool_update{0};
28+        while (now < deadline && !chainman().m_interrupt && getTip().value_or(BlockRef{}).hash == current_tip) {
29             // Did anything change at all?
30-            if (context()->mempool->GetTransactionsUpdated() != last_mempool_update) {
31+            unsigned int mempool_update{context()->mempool->GetTransactionsUpdated()};
32+            if (mempool_update != last_mempool_update) {
33+                last_mempool_update = mempool_update;
34                 auto block_template{BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(CScript())};
35 
36                 CAmount fees = 0;
37@@ -994,6 +987,7 @@ public:
38             }
39 
40             std::this_thread::sleep_until(std::min(deadline, now + tick));
41+            now = std::chrono::steady_clock::now();
42         }
43         return false;
44     }
  12. ryanofsky approved
  13. ryanofsky commented at 5:20 pm on October 8, 2024: contributor

    Code review ACK 9a723c784ad5170b1e01e91f018e5794a51dd038, but implementation has a few rough edges described below, which would recommend fixing.

    Also I think it would be a lot clearer to squash the two commits. If second commit is a problem for “(very) low end hardware” as described, I think it might make sense to make fee_threshold a std::optional parameter so callers running on slow hardware can skip the fee calculation. Alternately could tweak sleep_until to make sure the function is sleeping a minimum amount of time.

  14. tdb3 commented at 5:43 pm on October 8, 2024: contributor

    Approach ACK

    Cherry-picked bitcoin-mine from #30437 to exercise some of the interface (on top of 9a723c784ad5170b1e01e91f018e5794a51dd038).

    0git cherry-pick aec7e7d897741fe25fbf54995e1825772e0872d7
1git cherry-pick 2227afac0372358287fb879f3b0bd07fd653f3f8
2
3make -C depends NO_QT=1 MULTIPROCESS=1
4HOST_PLATFORM="x86_64-pc-linux-gnu"
5cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake

    Added the following to bitcoin-mine.cpp

     0diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
 1index 278413df86..5ec64fcd31 100644
 2--- a/src/bitcoin-mine.cpp
 3+++ b/src/bitcoin-mine.cpp
 4@@ -120,5 +120,30 @@ MAIN_FUNCTION
 5         tfm::format(std::cout, "Tip hash is null.\n");
 6     }
 7 
 8+    CAmount fee{500};
 9+    tfm::format(std::cout, "waitFeesChanged() with fee_threshold=%u, and max timeout\n", fee);
10+    tfm::format(std::cout, "Generate one block to see return=false\n");
11+    bool ret = mining->waitFeesChanged(tip->hash, fee); // wait max timeout
12+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
13+    tip = mining->getTip();
14+
15+    tfm::format(std::cout, "Add a transaction to the mempool with fee < 500 sats (no return)\n");
16+    tfm::format(std::cout, "then add a transaction to the mempool with fee >= 500 sats (return true)\n");
17+    ret = mining->waitFeesChanged(tip->hash, fee); // wait max timeout
18+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
19+
20+    MillisecondsDouble timeout{5000};
21+    tfm::format(std::cout, "waitFeesChanged() with fee_threshold=%u, and timeout 5000\n", fee);
22+    tfm::format(std::cout, "Wait 5 seconds to see return=false\n");
23+    ret = mining->waitFeesChanged(tip->hash, fee, {}, timeout);
24+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
25+
26+    tfm::format(std::cout, "waitFeesChanged() with fee_threshold=%u, and timeout 5000\n", fee);
27+    tfm::format(std::cout, "Do the following within 5 seconds\n");
28+    tfm::format(std::cout, "Add a transaction to the mempool with fee < 500 sats (no return)\n");
29+    tfm::format(std::cout, "then add a transaction to the mempool with fee >= 500 sats (return true)\n");
30+    ret = mining->waitFeesChanged(tip->hash, fee, {}, timeout);
31+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
32+
33     return EXIT_SUCCESS;
34 }

    Compiled and ran bitcoin-node:

    0cmake --build build -j18
1build/src/bitcoin-node -regtest -printtoconsole -debug=ipc -ipcbind=unix

    Created a wallet and generated some coins for it.

    0build/src/bitcoin-cli createwallet test
1build/src/bitcoin-cli -generate 105

    0dev@bdev02:~/bitcoin$ build/src/bitcoin-mine 
1Connected to bitcoin-node
2Tip hash is 7fb7fad5993fbbf52a4623a7cc7fedfd34f1190e4a44e8d67614a10d3490be3d.
3waitFeesChanged() with fee_threshold=500, and max timeout
4Generate one block to see return=false

    0dev@bdev02:~/bitcoin$ build/src/bitcoin-cli -generate 1

    0waitFeesChanged() returned false
1Add a transaction to the mempool with fee < 500 sats (no return)
2then add a transaction to the mempool with fee >= 500 sats (return true)

    0dev@bdev02:~/bitcoin$ build/src/bitcoin-cli -named sendtoaddress amount=3 address=bcrt1qhxcq73xjhtl7eyqkzuwemwpn2ljnala4mc6uwj fee_rate=1 && sleep 1 && build/src/bitcoin-cli -named sendtoaddress amount=3 address=bcrt1qhxcq73xjhtl7eyqkzuwemwpn2ljnala4mc6uwj fee_rate=5

    0waitFeesChanged() returned true

    0waitFeesChanged() with fee_threshold=500, and timeout 5000
1Wait 5 seconds to see return=false
2waitFeesChanged() returned false
3waitFeesChanged() with fee_threshold=500, and timeout 5000
4Do the following within 5 seconds
5Add a transaction to the mempool with fee < 500 sats (no return)
6then add a transaction to the mempool with fee >= 500 sats (return true)

    0dev@bdev02:~/bitcoin$ build/src/bitcoin-cli -named sendtoaddress amount=3 address=bcrt1qhxcq73xjhtl7eyqkzuwemwpn2ljnala4mc6uwj fee_rate=1 && sleep 1 && build/src/bitcoin-cli -named sendtoaddress amount=3 address=bcrt1qhxcq73xjhtl7eyqkzuwemwpn2ljnala4mc6uwj fee_rate=5

    0waitFeesChanged() returned true

    Generally seemed to work well.

    I did, however, run into a repeatable segfault that appears to be occurring with bitcoin_node when running bitcoin-mine (which calls waitFeesChanged()), interrupting bitcoin-mine (CTRL-C while it waits for change), then generating a block (with bitcoin-cli). Feels a bit DoSsy. Could be something I’m overlooking/messed up on my end. Not sure. Please request more details if needed.

     02024-10-06T17:11:13Z [ipc] {bitcoin-node-67256/b-capnp-loop-67260} IPC server post request  [#4](/bitcoin-bitcoin/4/) {bitcoin-node-67256/b-capnp-loop-67291 (from bitcoin-mine-67289/bitcoin-mine-67289)}
 1
 2Thread 3 "b-capnp-loop" received signal SIGPIPE, Broken pipe.
 3[Switching to Thread 0x7ffff6dfe6c0 (LWP 67260)]
 4__GI___writev (iovcnt=2, iov=0x7ffff6dfc6c0, fd=31) at ../sysdeps/unix/sysv/linux/writev.c:26
 526      ../sysdeps/unix/sysv/linux/writev.c: No such file or directory.
 6(gdb) c
 7Continuing.
 82024-10-06T17:11:23Z [ipc] {bitcoin-node-67256/b-capnp-loop-67260} IPC server: socket disconnected.
 92024-10-06T17:11:23Z [ipc] {bitcoin-node-67256/b-capnp-loop-67260} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages4InitEEE
102024-10-06T17:11:26Z CreateNewBlock(): block weight: 888 txs: 0 fees: 0 sigops 400
112024-10-06T17:11:26Z Saw new header hash=7227516712ea6f9f8fb24763a3ba8eeb3ecee4d0640f1227a41320b66cf6605a height=111
122024-10-06T17:11:26Z UpdateTip: new best=7227516712ea6f9f8fb24763a3ba8eeb3ecee4d0640f1227a41320b66cf6605a height=111 version=0x20000000 log2_work=7.807355 tx=118 date='2024-10-06T17:11:26Z' progress=1.000000 cache=0.3MiB(8txo)
132024-10-06T17:11:26Z [test] AddToWallet 20a7fcb543900a59764414197f5aaf36e85454dc916c4049c4fe4f0eb31381e4  new Confirmed (block=7227516712ea6f9f8fb24763a3ba8eeb3ecee4d0640f1227a41320b66cf6605a, height=111, index=0)
14
15Thread 30 "b-capnp-loop" received signal SIGSEGV, Segmentation fault.
16[Switching to Thread 0x7fff9d7fa6c0 (LWP 67291)]
170x00007ffff7cc0193 in std::_Rb_tree_rebalance_for_erase(std::_Rb_tree_node_base*, std::_Rb_tree_node_base&) ()
18   from /lib/x86_64-linux-gnu/libstdc++.so.6
19(gdb) bt
20[#0](/bitcoin-bitcoin/0/)  0x00007ffff7cc0193 in std::_Rb_tree_rebalance_for_erase(std::_Rb_tree_node_base*, std::_Rb_tree_node_base&) ()
21   from /lib/x86_64-linux-gnu/libstdc++.so.6
22[#1](/bitcoin-bitcoin/1/)  0x0000555555a889d9 in std::_Rb_tree<mp::Connection*, std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> >, std::_Select1st<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > >, std::less<mp::Connection*>, std::allocator<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > > >::_M_erase_aux (__position=..., this=<optimized out>)
23    at /usr/include/c++/12/bits/stl_tree.h:2488
24[#2](/bitcoin-bitcoin/2/)  std::_Rb_tree<mp::Connection*, std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> >, std::_Select1st<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > >, std::less<mp::Connection*>, std::allocator<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > > >::erase[abi:cxx11](std::_Rb_tree_iterator<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > >) (__position=..., this=<optimized out>) at /usr/include/c++/12/bits/stl_tree.h:1209
25[#3](/bitcoin-bitcoin/3/)  std::map<mp::Connection*, mp::ProxyClient<mp::Thread>, std::less<mp::Connection*>, std::allocator<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > > >::erase[abi:cxx11](std::_Rb_tree_iterator<std::pair<mp::Connection* const, mp::ProxyClient<mp::Thread> > >) (__position=..., this=<optimized out>) at /usr/include/c++/12/bits/stl_map.h:1086
26[#4](/bitcoin-bitcoin/4/)  mp::PassField<mp::Accessor<mp::mining_fields::Context, 17>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > >, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > > >(mp::Priority<1>, mp::TypeList<>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > > const&, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > >&&)::{lambda()#1}::operator()()::{lambda()#2}::operator()() const (__closure=<synthetic pointer>)
27    at ../../../depends/x86_64-pc-linux-gnu/include/mp/proxy-types.h:156
28[#5](/bitcoin-bitcoin/5/)  mp::PassField<mp::Accessor<mp::mining_fields::Context, 17>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > >, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > > >(mp::Priority<1>, mp::TypeList<>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > > const&, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > >&&)::{lambda()#1}::operator()()::{lambda()#2}::operator()() const (__closure=<synthetic pointer>)
29    at ../../../depends/x86_64-pc-linux-gnu/include/mp/proxy-types.h:156
30[#6](/bitcoin-bitcoin/6/)  kj::_::Deferred<mp::PassField<mp::Accessor<mp::mining_fields::Context, 17>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > >, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > > >(mp::Priority<1>, mp::TypeList<>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > > const&, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > >&&)::{lambda()#1}::operator()()::{lambda()#2}>::run() (this=0x7fff9d7f9250)
31    at ../../../depends/x86_64-pc-linux-gnu/include/kj/common.h:2001
32[#7](/bitcoin-bitcoin/7/)  kj::_::Deferred<mp::PassField<mp::Accessor<mp::mining_fields::Context, 17>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fi--Type <RET> for more, q to quit, c to continue without paging--
33elds::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > >, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > > >(mp::Priority<1>, mp::TypeList<>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > > const&, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > >&&)::{lambda()#1}::operator()()::{lambda()#2}>::~Deferred() (this=0x7fff9d7f9250, __in_chrg=<optimized out>)
34    at ../../../depends/x86_64-pc-linux-gnu/include/kj/common.h:1990
35[#8](/bitcoin-bitcoin/8/)  mp::PassField<mp::Accessor<mp::mining_fields::Context, 17>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > >, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > > >(mp::Priority<1>, mp::TypeList<>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::WaitFeesChangedParams, ipc::capnp::messages::Mining::WaitFeesChangedResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::CurrentTip, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::FeeThreshold, 1>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerField<1, mp::Accessor<mp::mining_fields::Timeout, 1>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 2>, mp::ServerCall> > > > > const&, mp::TypeList<uint256, long, node::BlockCreateOptions const&, std::chrono::duration<double, std::ratio<1l, 1000l> > >&&)::{lambda()#1}::operator()() (__closure=0x7fffe801b640) at ../../../depends/x86_64-pc-linux-gnu/include/mp/proxy-types.h:161
36[#9](/bitcoin-bitcoin/9/)  0x0000555555d5145f in mp::ProxyServer<mp::ThreadMap>::makeThread(capnp::CallContext<mp::ThreadMap::MakeThreadParams, mp::ThreadMap::MakeThreadResults>)::{lambda()#1}::operator()() const ()
37[#10](/bitcoin-bitcoin/10/) 0x00007ffff7cd44a3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
38[#11](/bitcoin-bitcoin/11/) 0x00007ffff7aa8144 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
39[#12](/bitcoin-bitcoin/12/) 0x00007ffff7b287dc in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
40(gdb)
  15. ryanofsky referenced this in commit 0bc674186d on Oct 8, 2024
  16. ryanofsky commented at 8:26 pm on October 8, 2024: contributor

    re: #31003#pullrequestreview-2349619672

    I did, however, run into a repeatable segfault that appears to be occurring with bitcoin_node when running bitcoin-mine (which calls waitFeesChanged()), interrupting bitcoin-mine (CTRL-C while it waits for change), then generating a block (with bitcoin-cli). Feels a bit DoSsy. Could be something I’m overlooking/messed up on my end. Not sure. Please request more details if needed. @tdb3 Thanks for reporting this and providing the stack trace. I think I was able to figure out the problem based on your description and implemented a potential fix in https://github.com/chaincodelabs/libmultiprocess/pull/118. If you want to test out the fix you can try cherry-picking 41d94fa0c31ce0c48401dffa73b523859dcd8df8 on top of this PR.

  17. Sjors commented at 9:32 am on October 10, 2024: member

    Thanks for the feedback, and great job finding that bug @tdb3.

    (It might be two weeks before I get to updating this PR)

  18. ryanofsky referenced this in commit 7f1d33f81c on Oct 15, 2024
  19. ryanofsky referenced this in commit 9d11042772 on Oct 16, 2024
  20. ryanofsky referenced this in commit 245581d3e3 on Oct 16, 2024
  21. in src/interfaces/mining.h:84 in 9a723c784a 
    79+     * @param[in] options       options for creating the block, should match those
80+     *                          passed to createNewBlock
81+     *
82+     * @returns true if fees increased, false if a new tip arrives or the timeout occurs
83+     */
84+    virtual bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const node::BlockCreateOptions& options = {}, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0;
    TheBlueMatt commented at 1:24 pm on October 17, 2024:
    Shouldn’t this return a blocktemplate instead? Requiring a second round-trip after getting a notification is just unnecessary latency.
    Sjors commented at 3:17 pm on October 21, 2024:
    That might make sense, though I’m not sure how much these round trips matter when running on the same machine.
    Sjors commented at 12:36 pm on November 13, 2024:
    #31283 returns a block template
  22. in src/node/interfaces.cpp:960 in 9a723c784a 
    956@@ -957,6 +957,47 @@ class MinerImpl : public Mining
957         return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
958     }
959 
960+    bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const BlockCreateOptions& options, MillisecondsDouble timeout) override
    TheBlueMatt commented at 1:29 pm on October 17, 2024:
    It’d be nice if bitcoin.conf could specify some options to restrict the fee threshold/timeout to some window. That way (once we drop CNB from the interface, see #31109) the protocol is at least kinda robust against trivial DoS attacks, which would be nice for teeing it up to be served over TCP in the future (or the inevitable exposing it over netcat that’ll happen).
    Sjors commented at 3:18 pm on October 21, 2024:
    I don’t think we should be trying to make the IPC interface DoS resistant. We don’t do that for RPC either.
  23. laanwj added the label Mining on Oct 20, 2024
  24. Sjors referenced this in commit 0226922bfb on Oct 31, 2024
  25. Sjors commented at 3:29 pm on November 8, 2024: member
    Marking draft because I might drop this method in favor of the approach suggested in #31109 (comment)
  26. Sjors marked this as a draft on Nov 8, 2024
  27. Sjors referenced this in commit 1114e14c28 on Nov 10, 2024
  28. Sjors commented at 12:31 pm on November 13, 2024: member
    I’ll close this PR if #31283 gets enough concept ACKs.
  29. Sjors commented at 1:36 pm on November 14, 2024: member
    Actually I’m convinced the waitNext() approach is better.
  30. Sjors closed this on Nov 14, 2024


Sjors DrahtBot ryanofsky danielabrozzoni tdb3 TheBlueMatt

Labels
Mining

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-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me