Add waitFeesChanged() to Mining interface #31003

Sjors · 2024-09-30T18:15:40Z

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.

DrahtBot · 2024-09-30T18:15:43Z

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.

src/ipc/capnp/mining.capnp

danielabrozzoni

Code review ACK 9a723c7 - code looks good to me, I don't have an opinion on merging both commits vs waiting for cluster mempool for the second one.

ryanofsky

Code review ACK 9a723c7, 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.

ryanofsky · 2024-10-08T16:26:43Z

src/node/interfaces.cpp


+    bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const BlockCreateOptions& options, MillisecondsDouble timeout) override
+    {
+        if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono


In commit "Introduce waitFeesChanged() mining interface" (a87589a)

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

ryanofsky · 2024-10-08T16:59:06Z

src/node/interfaces.cpp

-            // fees for the next block. This is currently too expensive.
-            if (context()->mempool->GetTransactionsUpdated() > last_mempool_update) return true;
+            // Did anything change at all?
+            if (context()->mempool->GetTransactionsUpdated() != last_mempool_update) {


In commit "Have waitFeesChanged() frequently make a template" (9a723c7)

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.

--- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -959,29 +959,22 @@ public: bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const BlockCreateOptions& options, MillisecondsDouble timeout) override { - if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono auto now{std::chrono::steady_clock::now()}; const auto deadline = now + timeout; const MillisecondsDouble tick{1000}; - unsigned int last_mempool_update{context()->mempool->GetTransactionsUpdated()}; - BlockAssembler::Options assemble_options{options}; ApplyArgsManOptions(*Assert(m_node.args), assemble_options); // It's not necessary to verify the template, since we don't return it. // This is also faster. assemble_options.test_block_validity = false; - while (!chainman().m_interrupt) { - now = std::chrono::steady_clock::now(); - if (now >= deadline) break; - - if (getTip().value().hash != current_tip) { - return false; - } - + unsigned int last_mempool_update{0}; + while (now < deadline && !chainman().m_interrupt && getTip().value_or(BlockRef{}).hash == current_tip) { // Did anything change at all? - if (context()->mempool->GetTransactionsUpdated() != last_mempool_update) { + unsigned int mempool_update{context()->mempool->GetTransactionsUpdated()}; + if (mempool_update != last_mempool_update) { + last_mempool_update = mempool_update; auto block_template{BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(CScript())}; CAmount fees = 0; @@ -994,6 +987,7 @@ public: } std::this_thread::sleep_until(std::min(deadline, now + tick)); + now = std::chrono::steady_clock::now(); } return false; }

tdb3

Approach ACK

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

git cherry-pick aec7e7d897741fe25fbf54995e1825772e0872d7
git cherry-pick 2227afac0372358287fb879f3b0bd07fd653f3f8

make -C depends NO_QT=1 MULTIPROCESS=1
HOST_PLATFORM="x86_64-pc-linux-gnu"
cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake

Added the following to bitcoin-mine.cpp

diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index 278413df86..5ec64fcd31 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -120,5 +120,30 @@ MAIN_FUNCTION
         tfm::format(std::cout, "Tip hash is null.\n");
     }
 
+    CAmount fee{500};
+    tfm::format(std::cout, "waitFeesChanged() with fee_threshold=%u, and max timeout\n", fee);
+    tfm::format(std::cout, "Generate one block to see return=false\n");
+    bool ret = mining->waitFeesChanged(tip->hash, fee); // wait max timeout
+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
+    tip = mining->getTip();
+
+    tfm::format(std::cout, "Add a transaction to the mempool with fee < 500 sats (no return)\n");
+    tfm::format(std::cout, "then add a transaction to the mempool with fee >= 500 sats (return true)\n");
+    ret = mining->waitFeesChanged(tip->hash, fee); // wait max timeout
+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
+
+    MillisecondsDouble timeout{5000};
+    tfm::format(std::cout, "waitFeesChanged() with fee_threshold=%u, and timeout 5000\n", fee);
+    tfm::format(std::cout, "Wait 5 seconds to see return=false\n");
+    ret = mining->waitFeesChanged(tip->hash, fee, {}, timeout);
+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
+
+    tfm::format(std::cout, "waitFeesChanged() with fee_threshold=%u, and timeout 5000\n", fee);
+    tfm::format(std::cout, "Do the following within 5 seconds\n");
+    tfm::format(std::cout, "Add a transaction to the mempool with fee < 500 sats (no return)\n");
+    tfm::format(std::cout, "then add a transaction to the mempool with fee >= 500 sats (return true)\n");
+    ret = mining->waitFeesChanged(tip->hash, fee, {}, timeout);
+    tfm::format(std::cout, "waitFeesChanged() returned %s\n", (ret)? "true" : "false");
+
     return EXIT_SUCCESS;
 }

Compiled and ran bitcoin-node:

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

Created a wallet and generated some coins for it.

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

Ran bitcoin-mine while performing bitcoin-cli actions in another terminal.

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

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

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

dev@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

waitFeesChanged() returned true

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

dev@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

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

bitcoin-node gdb trace

2024-10-06T17:11:13Z [ipc] {bitcoin-node-67256/b-capnp-loop-67260} IPC server post request  #4 {bitcoin-node-67256/b-capnp-loop-67291 (from bitcoin-mine-67289/bitcoin-mine-67289)}

Thread 3 "b-capnp-loop" received signal SIGPIPE, Broken pipe.
[Switching to Thread 0x7ffff6dfe6c0 (LWP 67260)]
__GI___writev (iovcnt=2, iov=0x7ffff6dfc6c0, fd=31) at ../sysdeps/unix/sysv/linux/writev.c:26
26      ../sysdeps/unix/sysv/linux/writev.c: No such file or directory.
(gdb) c
Continuing.
2024-10-06T17:11:23Z [ipc] {bitcoin-node-67256/b-capnp-loop-67260} IPC server: socket disconnected.
2024-10-06T17:11:23Z [ipc] {bitcoin-node-67256/b-capnp-loop-67260} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages4InitEEE
2024-10-06T17:11:26Z CreateNewBlock(): block weight: 888 txs: 0 fees: 0 sigops 400
2024-10-06T17:11:26Z Saw new header hash=7227516712ea6f9f8fb24763a3ba8eeb3ecee4d0640f1227a41320b66cf6605a height=111
2024-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)
2024-10-06T17:11:26Z [test] AddToWallet 20a7fcb543900a59764414197f5aaf36e85454dc916c4049c4fe4f0eb31381e4  new Confirmed (block=7227516712ea6f9f8fb24763a3ba8eeb3ecee4d0640f1227a41320b66cf6605a, height=111, index=0)

Thread 30 "b-capnp-loop" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff9d7fa6c0 (LWP 67291)]
0x00007ffff7cc0193 in std::_Rb_tree_rebalance_for_erase(std::_Rb_tree_node_base*, std::_Rb_tree_node_base&) ()
   from /lib/x86_64-linux-gnu/libstdc++.so.6
(gdb) bt
#0  0x00007ffff7cc0193 in std::_Rb_tree_rebalance_for_erase(std::_Rb_tree_node_base*, std::_Rb_tree_node_base&) ()
   from /lib/x86_64-linux-gnu/libstdc++.so.6
#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>)
    at /usr/include/c++/12/bits/stl_tree.h:2488
#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
#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
#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>)
    at ../../../depends/x86_64-pc-linux-gnu/include/mp/proxy-types.h:156
#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>)
    at ../../../depends/x86_64-pc-linux-gnu/include/mp/proxy-types.h:156
#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)
    at ../../../depends/x86_64-pc-linux-gnu/include/kj/common.h:2001
#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--
elds::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>)
    at ../../../depends/x86_64-pc-linux-gnu/include/kj/common.h:1990
#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
#9  0x0000555555d5145f in mp::ProxyServer<mp::ThreadMap>::makeThread(capnp::CallContext<mp::ThreadMap::MakeThreadParams, mp::ThreadMap::MakeThreadResults>)::{lambda()#1}::operator()() const ()
#10 0x00007ffff7cd44a3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#11 0x00007ffff7aa8144 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#12 0x00007ffff7b287dc in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb)

tdb3 · 2024-10-05T14:48:28Z

src/interfaces/mining.h

+    /**
+     * Waits for fees in the next block to rise, a new tip or the timeout.
+     *
+     * @param[in] current_tip   block hash that the most recent template builds on
+     * @param[in] fee_threshold how far total fees for the next block should rise (currently ignored)
+     * @param[in] options       options for creating the block, should match those
+     *                          passed to createNewBlock (currently ignored)
+     *
+     * @returns true if fees increased, false if a new tip arrives or the timeout occurs
+     */


non-blocking nitty nit: Could include all function arguments as params in the doxygen comments. Feel free to ignore.

tdb3 · 2024-10-05T14:51:40Z

src/interfaces/mining.h

+     * @param[in] options       options for creating the block, should match those
+     *                          passed to createNewBlock (currently ignored)
+     *
+     * @returns true if fees increased, false if a new tip arrives or the timeout occurs


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

…uring long function call Fix issue where if a client disconnects in the middle of a long running IPC method call, the server will segfault when the method eventually returns. This issue was reported This behavior was reported by tdb3 bitcoin/bitcoin#31003 (review) including a stack trace showing where the crash happens in the PassField mp.Context overload while calling request_threads.erase.

ryanofsky · 2024-10-08T20:26:53Z

re: #31003 (review)

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 bitcoin-core/libmultiprocess#118. If you want to test out the fix you can try cherry-picking 41d94fa on top of this PR.

Sjors · 2024-10-10T09:32:56Z

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

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

…uring long function call Fix issue where if a client disconnects in the middle of a long running IPC method call, the server will segfault when the method eventually returns. This behavior was reported by tdb3 in bitcoin/bitcoin#31003 (review) including a stack trace showing where the crash happens in the PassField mp.Context overload while calling request_threads.erase.

…s during method call Fix issue where if a client disconnects in the middle of a long running IPC method call, the server will segfault when the method eventually returns. This behavior was reported by tdb3 in bitcoin/bitcoin#31003 (review) including a stack trace showing where the crash happens in the PassField mp.Context overload while calling request_threads.erase.

… is broken during long function call 196e6fc bugfix: prevent null pointer dereference in server if client disconnects during method call (Ryan Ofsky) 9d11042 bugfix: prevent double delete segfault in server if client disconnects during method call (Ryan Ofsky) Pull request description: Fix issue where if a client disconnects in the middle of a long running IPC method call, the server will segfault when the method eventually returns. This issue was reported tdb3 in bitcoin/bitcoin#31003 (review) with a stack trace showing where the crash happens in the `PassField` `mp.Context` overload while calling `request_threads.erase`. Top commit has no ACKs. Tree-SHA512: a49d3d6b46a319944ad54883d390a248bb9a3ece34bbb424222f8d5bfa015b8392e4fb7b830b4ae69c90d257d16c2122c4b8405a1078d7b3a979dbdd2c3d07be

TheBlueMatt · 2024-10-17T13:24:52Z

src/interfaces/mining.h

+     *
+     * @returns true if fees increased, false if a new tip arrives or the timeout occurs
+     */
+    virtual bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const node::BlockCreateOptions& options = {}, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0;


Shouldn't this return a blocktemplate instead? Requiring a second round-trip after getting a notification is just unnecessary latency.

That might make sense, though I'm not sure how much these round trips matter when running on the same machine.

#31283 returns a block template

TheBlueMatt · 2024-10-17T13:29:59Z

src/node/interfaces.cpp

        return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
    }

+    bool waitFeesChanged(uint256 current_tip, CAmount fee_threshold, const BlockCreateOptions& options, MillisecondsDouble timeout) override


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

I don't think we should be trying to make the IPC interface DoS resistant. We don't do that for RPC either.

Sjors · 2024-11-08T15:29:51Z

Marking draft because I might drop this method in favor of the approach suggested in #31109 (comment)

Sjors · 2024-11-13T12:31:05Z

I'll close this PR if #31283 gets enough concept ACKs.

Sjors · 2024-11-14T13:36:12Z

Actually I'm convinced the waitNext() approach is better.

c991cea Remove processNewBlock() from mining interface (Sjors Provoost) 9a47852 Remove getTransactionsUpdated() from mining interface (Sjors Provoost) bfc4e02 Remove testBlockValidity() from mining interface (Sjors Provoost) Pull request description: There are three methods in the mining interface that can be dropped. The Template Provider doesn't need them and other application should probably not use them either. 1. `processNewBlock()` was added in 7b4d324, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d324. Dropping it was suggested in #30200 (comment) 2. `getTransactionsUpdated()`: this is used in the implementation of #31003 `waitFeesChanged`. It's not very useful generically because the mempool updates very frequently. 3. `testBlockValidity()`: it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn't do that, and this method is a bit brittle (e.g. the block needs to build on the tip). ACKs for top commit: TheCharlatan: Re-ACK c991cea ryanofsky: Code review ACK c991cea. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message tdb3: code review ACK c991cea Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67

cadbd41 miner: have waitNext return after 20 min on testnet (Sjors Provoost) d4020f5 Add waitNext() to BlockTemplate interface (Sjors Provoost) Pull request description: This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat. On testnet3 and testnet4 the difficulty drops after 20 minutes, so the second ensures that a new template is returned in that case. Alternative approach to #31003, suggested in #31109 (comment) ACKs for top commit: ryanofsky: Code review ACK cadbd41. Main change since last review is adding back a missing `m_interrupt` check in the waitNext loop. Also made various code cleanups in both commits. ismaelsadeeq: Code review ACK cadbd41 vasild: ACK cadbd41 Tree-SHA512: c5a40053723c1c1674449ba1e4675718229a2022c8b0a4853b12a2c9180beb87536a1f99fde969a0ef099bca9ac69ca14ea4f399d277d2db7f556465ce47de95

Sjors added 2 commits September 30, 2024 20:12

Introduce waitFeesChanged() mining interface

a87589a

Have waitFeesChanged() frequently make a template

9a723c7

This was referenced Sep 30, 2024

Introduce waitFeesChanged() mining interface Sjors/bitcoin#52

Closed

Stratum v2 Template Provider via IPC interface (multiprocess) Sjors/bitcoin#48

Closed

Sjors changed the title ~~Introduce waitFeesChanged() mining interface~~ Add waitFeesChanged() to Mining interface Sep 30, 2024

Sjors commented Sep 30, 2024

View reviewed changes

src/ipc/capnp/mining.capnp Show resolved Hide resolved

Sjors mentioned this pull request Oct 1, 2024

ci: build multiprocess on most jobs #30975

Closed

danielabrozzoni reviewed Oct 1, 2024

View reviewed changes

Sjors mentioned this pull request Oct 4, 2024

Stratum v2 Template Provider common functionality Sjors/bitcoin#49

Closed

15 tasks

ryanofsky approved these changes Oct 8, 2024

View reviewed changes

tdb3 reviewed Oct 8, 2024

View reviewed changes

ryanofsky mentioned this pull request Oct 8, 2024

shutdown bugfix: Prevent segfault in server if connection is broken during long function call bitcoin-core/libmultiprocess#118

Merged

fanquake mentioned this pull request Oct 10, 2024

Stratum v2 Template Provider (take 3) #29432

Closed

24 tasks

Sjors mentioned this pull request Oct 16, 2024

Stratum v2 via IPC Mining Interface tracking issue #31098

Closed

33 tasks

TheBlueMatt reviewed Oct 17, 2024

View reviewed changes

laanwj added the Mining label Oct 20, 2024

This was referenced Oct 31, 2024

Prune mining interface #31196

Merged

Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants #31109

Closed

ryanofsky mentioned this pull request Nov 7, 2024

Multiprocess tracking issue #28722

Open

Sjors marked this pull request as draft November 8, 2024 15:29

Sjors mentioned this pull request Nov 13, 2024

Add waitNext() to BlockTemplate interface #31283

Merged

Sjors closed this Nov 14, 2024

Sjors mentioned this pull request Jan 8, 2025

mining: bugfix: Fix duplicate coinbase tx weight reservation #31384

Merged

bitcoin locked and limited conversation to collaborators Nov 14, 2025

Add waitFeesChanged() to Mining interface #31003

Add waitFeesChanged() to Mining interface #31003

Uh oh!

Conversation

Sjors commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdb3 Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 8, 2024

Uh oh!

Sjors commented Oct 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 8, 2024

Uh oh!

Sjors commented Nov 13, 2024

Uh oh!

Sjors commented Nov 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Sjors commented Sep 30, 2024 •

edited

Loading

DrahtBot commented Sep 30, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading

tdb3 Oct 5, 2024 •

edited

Loading