Conversation
f098c30 to
28e721f
Compare
enirox001
left a comment
enirox001
left a comment
There was a problem hiding this comment.
Code Review 28e721f
supporting both mining interfaces and detecting compatibility at startup makes sense. and using the rejection reasons from the newer interface is also nice to have
had a first look through the code, would look at it again, but left some suggestions and nits below
| auto log_fn = [](bool /*raise*/, std::string message) { | ||
| // Mirror IpcLogFn(): an error reported by the IPC layer, such as a | ||
| // method the other side does not implement, is thrown to the caller. | ||
| auto log_fn = [](bool raise, std::string message) { |
There was a problem hiding this comment.
In commit a45d15d: test: throw IPC errors in the sv2-tp test harness
I think we can use mp::LogMessage here instead of having a boolean and a string, this would match IpcLogFn as well. I think something like this?
index 946e810..5b14364 100644
--- a/src/test/sv2_tp_tester.cpp
+++ b/src/test/sv2_tp_tester.cpp
@@ -89,9 +89,9 @@ TPTester::TPTester(Sv2TemplateProviderOptions opts, MockNodeVersion version)
m_loop_thread = std::thread([&] {
// Mirror IpcLogFn(): an error reported by the IPC layer, such as a
// method the other side does not implement, is thrown to the caller.
- auto log_fn = [](bool raise, std::string message) {
- if (G_TEST_LOG_FUN) G_TEST_LOG_FUN(message);
- if (raise) throw ipc::Exception(message);
+ auto log_fn = [](mp::LogMessage message) {
+ if (G_TEST_LOG_FUN) G_TEST_LOG_FUN(message.message);
+ if (message.level == mp::Log::Raise) throw ipc::Exception(message.message);
};
mp::EventLoop loop("sv2-tp-test", log_fn);
m_loop = &loop;Also perhaps we could have a regression test verifying that an ipc failure reaches the caller as ipc::Exception through this? I’m unsure of the cleanest way to trigger that in the tests.
| * @param[out] debug more detailed rejection reason | ||
| * @returns true if the block was accepted as a new block | ||
| */ | ||
| virtual bool submitBlock(const CBlock& block, std::string& reason, std::string& debug) = 0; |
There was a problem hiding this comment.
In commit 42142da: ipc: detect the node mining interface version
Just a thought, but I am thinking about the overhead of having to define ordinals just to keep a downstream client in sync with bitcoin core.
Would be nice if there was a way to skip an ordinal for a downstream user while bitcoin core still has such an ordinal defined. I guess this might be possible to implement in libmultiprocess since I has the @skip annotation. Wonder if it could be expanded to support this.
There was a problem hiding this comment.
If libmultiprocess implements something like that, we can use it here. But in practice it's not a big deal.
| return gArgs.GetDataDirNet() / "sv2_authority_key"; | ||
| } | ||
|
|
||
| void Sv2TemplateProvider::DetectNodeVersion() |
There was a problem hiding this comment.
In commit 42142da#r3991995134: ipc: detect the node mining interface version
right now there this would categorise any exception while running getTransactionByTxId would be categorised as an 'old node'
I don't think this should be much of a concern because I am unsure there is a code path that could cause this right now. But perhaps we could check the kind of exception and then check the node version based on the kind of that exception
This only extends runtime_error so we do not have methods to check this, so perhaps we could implement a way to preserve the error categories, perhaps in a followup
There was a problem hiding this comment.
I'm now specifically checking for "Method not implemented."
| } | ||
|
|
||
| // The node version is determined when the template provider starts. | ||
| BOOST_AUTO_TEST_CASE(node_version_detection) |
There was a problem hiding this comment.
In commit 42142da#r3992088876: ipc: detect the node mining interface version
OldNodeMining directly throws the exception that detection expects. This verifies the fallback branch, but doesn’t verify that a genuinely missing remote method produces that exception.
We should have an integration test that adding that coverage against older interfaces. Something like this
index 50f2beb..b14e564 100755
--- a/ci/test/04_run_stratum_v2_scenario.sh
+++ b/ci/test/04_run_stratum_v2_scenario.sh
@@ -367,6 +367,16 @@ run_phase()
grep -q "Connected to bitcoin-node via IPC" "${LOG_DIR}/sv2-tp.log"
+ # Verify legacy interface detection through the real Bitcoin Core v31 IPC connection.
+ if ! grep -q 'getTransactionsByTxID() is not available: kj::Exception:' "${LOG_DIR}/sv2-tp.log"; then
+ echo "Bitcoin Core v31.0 did not trigger the expected missing-method IPC exception" >&2
+ return 1
+ fi
+ if ! grep -q 'The IPC error above is expected when connecting to Bitcoin Core v31' "${LOG_DIR}/sv2-tp.log"; then
+ echo "sv2-tp did not select the legacy mining interface" >&2
+ return 1
+ fi
+
echo "Verifying BIP54 compliance of SRI-mined blocks (heights 18..${count})"
bip54_passed=0
for ((h = 18; h <= count; ++h)); doThere was a problem hiding this comment.
Added, plus a CI matrix to cover v31.1, v32.0(rc1, built from source for now) and master.
28e721f to
7bdd8f9
Compare
|
Rebased just in case. Addressed @enirox001's feedback. |
|
I opened #122 to first address the inconsistency in |
The event loop logging callback ignored the raise flag, unlike IpcLogFn(), so an error reported by the IPC layer, such as a call to a method that the other side does not implement, was silently swallowed in tests. Throw the same ipc::Exception that IpcLogFn() throws, because that is what the template provider catches.
Bitcoin Core changed the mining interface after v31, and sv2-tp needs to keep working with both. Determine which version the node has when the template provider starts, rather than when a call fails, so that time critical calls are not delayed. getTransactionsByTxID() is used for this because it was added after v31 and has no side effects when passed an empty list. submitBlock() is added as well because capnp ordinals must be sequential; it is not used. TPTester can now simulate a node without getTransactionsByTxID(). It does so by wrapping the mining proxy, because the mock server implements every method of the current interface. A real v31 node does not throw either, capnp simply tells the client that the method does not exist.
Bitcoin Core moved submitSolution() to mining.capnp @10 and gave it reason and debug output arguments, keeping the old method as @7 so that clients which were not updated get a clear error: bitcoin/bitcoin#34672 Use whichever method matches the version found by DetectNodeVersion(), so that Bitcoin Core v31 keeps working, and log the rejection reason when the block is not accepted.
Extend the v31.1 matrix with source builds of v32.0rc1 and master to cover both mining interface versions. Switch v32.0rc1 to a download once binaries are available. Assert that v31 releases select the legacy mining interface.
7bdd8f9 to
7312b0b
Compare
|
Rebased after #122. Made the v31 check a more targeted. |
2 participants
I need sv2-tp to work with both master and v31 of Bitcoin Core, in order to make Windows testing easier. This PR makes that possible.
This PR also adds a matrix to the integration test job, so it's run against Bitcoin Core v31.1, v32.0rc1 and master.