This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR #10102, but have been moved here to make that PR smaller.
All of these changes are refactoring changes which do not affect behavior of current code
This PR is part of the process separation project.
Needed to deserialize some types from spans like CScripts
Needed to fix new wallet_groups.py and wallet_resendwallettransactions.py tests
with multiprocess bitcoin-node executable.
Coin serialize method segfaults if IsSpent condition is true. This caused
multiprocess code to segfault when serializing the Coin& output argument to of
the Node::getUnspentOutput method if the coin was not found. Segfault could be
triggered by double clicking and viewing transaction details in the GUI
transaction list.
Fix this by replacing Coin& output argument with optional<Coin> return value to
avoid trying to serializing spent coins.
Needed by multiprocess support code to pass parsed configuration to a spawned process.
Also add test to make sure this doesn't get broken in the future.
This was breaking vector<bool> serialization in multiprocess code because
template current deduction guides would make it appear like vector<bool> could
be converted to a span, but then the actual conversion to span would fail.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | achow101, naumenkogs, maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
149 }
150
151 // Schedule periodic wallet flushes and tx rebroadcasts
152 if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
153- scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
154+ context.scheduler->scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
500ms while touching this?
593+ return StartWallets(m_context);
594+ }
595 void flush() override { return FlushWallets(m_context); }
596 void stop() override { return StopWallets(m_context); }
597 void setMockTime(int64_t time) override { return SetMockTime(time); }
598+ void schedulerMockForward(std::chrono::seconds delta) override { m_context.scheduler->MockForward(delta); }
Assert() to turn UB into a crash?
359@@ -360,12 +360,11 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
360 {
361 COutPoint prevout = txin.prevout;
362
363- Coin prev;
364- if(node.getUnspentOutput(prevout, prev))
365+ if (auto prev{node.getUnspentOutput(prevout)})
366 {
re: #28721 (review)
nit in 22edd79: Move to the above line while touching? (clang-format)
Thanks, updated
365+ if (auto prev{node.getUnspentOutput(prevout)})
366 {
367 {
368 strHTML += "<li>";
369- const CTxOut &vout = prev.out;
370+ const CTxOut &vout = prev->out;
235+// object instantiation would fail, resulting in a hard error, rather than a
236+// SFINAE error.
237+// https://stackoverflow.com/questions/68759148/sfinae-to-detect-the-explicitness-of-a-ctad-deduction-guide
238+// https://stackoverflow.com/questions/16568986/what-happens-when-you-call-data-on-a-stdvectorbool
239+template<typename T>
240+using DataResult = std::remove_pointer_t<decltype(std::declval<T&>().data())>;
using Span = std::span?
re: #28721 (review)
1fba885: I wonder if it is easier to just delete the content of this file and replace it with
using Span = std::span?
Switching from Span to std::span might work, but it’s a bigger change and I’d expect it to require other fixes and not just be a drop-in replacement.
If replacing span gets implemented and merged first, that’s great and this commit can be dropped. But otherwise the actual code change here is small. I wouldn’t want multiprocess code to depend on replacing span when there is a direct fix here which is 3 lines and is just adding two !std::is_void_v checks.
I didn’t review the Span commit yet.
ACK 28bddf90ccbb3a58d30210b6102220fa6253229f 🍍
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 28bddf90ccbb3a58d30210b6102220fa6253229f 🍍
35sCvwzO8Gj/WV/7QAif56fxTc3QT8+G/qXnBh/ZHH/rKxUb25SXW7I3/m26dnr0KfVVDVQGfar78kL1Yc4E5Dg==
Appreciate the review
Updated 28bddf90ccbb3a58d30210b6102220fa6253229f -> 3b70f7b6156cb110c47a6e482791cf337bb6ad6d (pr/ipcfix.2 -> pr/ipcfix.3, compare) with review suggestions.
re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d 🎆
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d 🎆
34vrBaTA+bS9c7lucywAakQWMvizh66HrffzpcXOPgLDVDBI4wkCQUPE94XqhE6S6GuepGDGFjHp5tecIYIoABA==