mining: pass missing context to createNewBlock() and checkBlock() #33936
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33936. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
See bitcoin/bitcoin#33936 Note how this is breaking change.
|
Although the change is fine, the original motivation is incorrect. I thought this would fix a serious memory management issue where the I marked the PR draft and updated the description to explain what it achieves and why it's not urgent. |
Sjors
force-pushed
the
2025/11/ipc-context
branch
from
a3a6861 to
c6238c3
Compare
Could be nice to return an explicit error to clients that are too old with a change like the following from #33777 (comment): diff
--- a/src/ipc/capnp/init.capnp
+++ b/src/ipc/capnp/init.capnp
@@ -19,5 +19,8 @@ using Mining = import "mining.capnp";
interface Init $Proxy.wrap("interfaces::Init") {
construct @0 (threadMap: Proxy.ThreadMap) -> (threadMap :Proxy.ThreadMap);
makeEcho @1 (context :Proxy.Context) -> (result :Echo.Echo);
- makeMining @2 (context :Proxy.Context) -> (result :Mining.Mining);
+ makeMining @3 (context :Proxy.Context) -> (result :Mining.Mining);
+
+ # DEPRECATED: no longer supported; server returns an error.
+ makeMiningV2 @2 () -> ();
}
--- a/src/interfaces/init.h
+++ b/src/interfaces/init.h
@@ -39,6 +39,7 @@ public:
virtual Ipc* ipc() { return nullptr; }
virtual bool canListenIpc() { return false; }
virtual const char* exeName() { return nullptr; }
+ virtual void makeMiningV2() { throw std::runtime_error("Old mining interface (@2) not supported. Please update your client!"); }
};
//! Return implementation of Init interface for the node process. If the argv |
|
Bumping the version number would also allow clients that wish to support older Bitcoin Core version to keep two capnp files around. |
Adding a context parameter ensures that these methods are run in their own thread and don't block other calls. They were missing for: - createNewBlock() - checkBlock() This ensures that when checkBlock() receives a slow to validate block from an untrusted origin (the IPC connection itself is assumed to trusted), it will not block the IPC server (though validation itself still blocks cs_main). This is a breaking change both ways: - clients must use the updated capnp file - updated clients can't call these methods on an unupgraded node
Sjors
force-pushed
the
2025/11/ipc-context
branch
from
c6238c3 to
3ea56d6
Compare
3 participants
Adding a
contextparameter ensures that these methods are run in their own thread and don't block other calls.For
createNewBlock()this is not expected to have much impact, because block template creation is fast.For
checkBlock()this ensures that if we receive a slow to validate block from an untrusted origin (the IPC connection itself is assumed to trusted), it will not block the IPC server. But since validation itself blockscs_mainthis is also not a huge performance win.This is a breaking change both ways:
Because it's a breaking change, but not really important, this PR is marked pending some other more important breaking change. See #33777.