multiprocess compatibility updates #28721

ryanofsky · 2023-10-24T18:16:54Z

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

…work across processes Needed to fix new wallet_groups.py and wallet_resendwallettransactions.py tests with multiprocess bitcoin-node executable.

… segfault 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.

…pn'proto

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.

DrahtBot · 2023-10-24T18:16:58Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10102 (Multiprocess bitcoin by ryanofsky)

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.

maflcko

I didn't review the Span commit yet.

ACK 28bddf9 🍍

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 28bddf90ccbb3a58d30210b6102220fa6253229f 🍍
5sCvwzO8Gj/WV/7QAif56fxTc3QT8+G/qXnBh/ZHH/rKxUb25SXW7I3/m26dnr0KfVVDVQGfar78kL1Yc4E5Dg==

maflcko · 2023-10-25T07:16:37Z

src/wallet/load.cpp

 {
    for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
        pwallet->postInitProcess();
    }

    // Schedule periodic wallet flushes and tx rebroadcasts
    if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
+        context.scheduler->scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});


nit in 62a0cea: Use 500ms while touching this?

re: #28721 (comment)

nit in 62a0cea: Use 500ms while touching this?

Thanks, updated

maflcko · 2023-10-25T07:17:03Z

src/wallet/interfaces.cpp

    void flush() override { return FlushWallets(m_context); }
    void stop() override { return StopWallets(m_context); }
    void setMockTime(int64_t time) override { return SetMockTime(time); }
+    void schedulerMockForward(std::chrono::seconds delta) override { m_context.scheduler->MockForward(delta); }


nit in 62a0cea: Missing Assert() to turn UB into a crash?

re: #28721 (comment)

nit in 62a0cea: Missing Assert() to turn UB into a crash?

Thanks, updated

maflcko · 2023-10-25T07:20:47Z

src/qt/transactiondesc.cpp

@@ -360,12 +360,11 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
        {
            COutPoint prevout = txin.prevout;

-            Coin prev;
-            if(node.getUnspentOutput(prevout, prev))
+            if (auto prev{node.getUnspentOutput(prevout)})
            {


nit in 22edd79: Move to the above line while touching? (clang-format)

re: #28721 (comment)

nit in 22edd79: Move to the above line while touching? (clang-format)

Thanks, updated

maflcko · 2023-10-25T07:21:02Z

src/qt/transactiondesc.cpp

            {
                {
                    strHTML += "<li>";
-                    const CTxOut &vout = prev.out;
+                    const CTxOut &vout = prev->out;


nit in 22edd79: Clang-format while touching this line?

re: #28721 (comment)

nit in 22edd79: Clang-format while touching this line?

Thanks, updated

maflcko · 2023-10-25T07:24:39Z

src/span.h

+// https://stackoverflow.com/questions/68759148/sfinae-to-detect-the-explicitness-of-a-ctad-deduction-guide
+// https://stackoverflow.com/questions/16568986/what-happens-when-you-call-data-on-a-stdvectorbool
+template<typename T>
+using DataResult = std::remove_pointer_t<decltype(std::declval<T&>().data())>;


1fba885: I wonder if it is easier to just delete the content of this file and replace it with using Span = std::span?

re: #28721 (comment)

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.

ryanofsky

Appreciate the review

Updated 28bddf9 -> 3b70f7b (pr/ipcfix.2 -> pr/ipcfix.3, compare) with review suggestions.

ryanofsky · 2023-10-25T13:12:53Z

src/span.h

+// https://stackoverflow.com/questions/68759148/sfinae-to-detect-the-explicitness-of-a-ctad-deduction-guide
+// https://stackoverflow.com/questions/16568986/what-happens-when-you-call-data-on-a-stdvectorbool
+template<typename T>
+using DataResult = std::remove_pointer_t<decltype(std::declval<T&>().data())>;


re: #28721 (comment)

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.

ryanofsky · 2023-10-25T13:17:27Z

src/qt/transactiondesc.cpp

@@ -360,12 +360,11 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
        {
            COutPoint prevout = txin.prevout;

-            Coin prev;
-            if(node.getUnspentOutput(prevout, prev))
+            if (auto prev{node.getUnspentOutput(prevout)})
            {


re: #28721 (comment)

nit in 22edd79: Move to the above line while touching? (clang-format)

Thanks, updated

ryanofsky · 2023-10-25T13:17:33Z

src/qt/transactiondesc.cpp

            {
                {
                    strHTML += "<li>";
-                    const CTxOut &vout = prev.out;
+                    const CTxOut &vout = prev->out;


re: #28721 (comment)

nit in 22edd79: Clang-format while touching this line?

Thanks, updated

ryanofsky · 2023-10-25T13:17:49Z

src/wallet/interfaces.cpp

    void flush() override { return FlushWallets(m_context); }
    void stop() override { return StopWallets(m_context); }
    void setMockTime(int64_t time) override { return SetMockTime(time); }
+    void schedulerMockForward(std::chrono::seconds delta) override { m_context.scheduler->MockForward(delta); }


re: #28721 (comment)

nit in 62a0cea: Missing Assert() to turn UB into a crash?

Thanks, updated

ryanofsky · 2023-10-25T13:17:57Z

src/wallet/load.cpp

 {
    for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
        pwallet->postInitProcess();
    }

    // Schedule periodic wallet flushes and tx rebroadcasts
    if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
+        context.scheduler->scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});


re: #28721 (comment)

nit in 62a0cea: Use 500ms while touching this?

Thanks, updated

achow101 · 2023-11-07T21:27:36Z

ACK 3b70f7b

naumenkogs · 2023-11-09T08:07:34Z

ACK 3b70f7b

maflcko · 2023-11-09T18:51:34Z

re-ACK 3b70f7b 🎆

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d  🎆
4vrBaTA+bS9c7lucywAakQWMvizh66HrffzpcXOPgLDVDBI4wkCQUPE94XqhE6S6GuepGDGFjHp5tecIYIoABA==

ryanofsky · 2023-11-16T14:15:14Z

Thanks for the reviews! I'll rebase #10102 now that this is merged, and create a new PR with the next set of changes from #10102

ryanofsky and others added 8 commits October 20, 2023 09:30

streams: Add SpanReader ignore method

82a379e

Needed to deserialize some types from spans like CScripts

interfaces: Fix const virtual method that breaks multiprocess support

924327e

interfaces: Add schedulerMockForward method so mockscheduler RPC can …

4978754

…work across processes Needed to fix new wallet_groups.py and wallet_resendwallettransactions.py tests with multiprocess bitcoin-node executable.

interfaces: Rename CalculateBumpFees methods to be compatible with ca…

441d00c

…pn'proto

util: Add ArgsManager SetConfigFilePath method

8062c3b

Needed by multiprocess support code to pass parsed configuration to a spawned process.

doc: fix broken doc/design/multiprocess.md links after bitcoin#24352

3b70f7b

This was referenced Oct 24, 2023

Multiprocess bitcoin #10102

Draft

Multiprocess tracking issue #28722

Open

DrahtBot added the CI failed label Oct 24, 2023

DrahtBot mentioned this pull request Oct 25, 2023

Remove the legacy wallet and BDB dependency #28710

Draft

maflcko reviewed Oct 25, 2023

View reviewed changes

DrahtBot removed the CI failed label Oct 25, 2023

ryanofsky force-pushed the pr/ipcfix branch from 28bddf9 to 3b70f7b Compare October 25, 2023 20:49

ryanofsky commented Oct 25, 2023

View reviewed changes

achow101 requested a review from maflcko November 7, 2023 21:09

DrahtBot removed the request for review from maflcko November 9, 2023 18:51

fanquake merged commit 29c2c90 into bitcoin:master Nov 13, 2023
16 checks passed

hebasto mentioned this pull request Nov 15, 2023

Rebased cmake-staging branch (post PRs 49 and 50) hebasto/bitcoin#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multiprocess compatibility updates #28721

multiprocess compatibility updates #28721

ryanofsky commented Oct 24, 2023 •

edited

DrahtBot commented Oct 24, 2023 •

edited

maflcko left a comment

maflcko Oct 25, 2023

ryanofsky Oct 25, 2023

maflcko Oct 25, 2023

ryanofsky Oct 25, 2023

maflcko Oct 25, 2023

ryanofsky Oct 25, 2023

maflcko Oct 25, 2023

ryanofsky Oct 25, 2023

maflcko Oct 25, 2023

ryanofsky Oct 25, 2023

ryanofsky left a comment

ryanofsky Oct 25, 2023

ryanofsky Oct 25, 2023

ryanofsky Oct 25, 2023

ryanofsky Oct 25, 2023

ryanofsky Oct 25, 2023

achow101 commented Nov 7, 2023

naumenkogs commented Nov 9, 2023

maflcko commented Nov 9, 2023

ryanofsky commented Nov 16, 2023

multiprocess compatibility updates #28721

multiprocess compatibility updates #28721

Conversation

ryanofsky commented Oct 24, 2023 • edited

DrahtBot commented Oct 24, 2023 • edited

Code Coverage

Reviews

Conflicts

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Nov 7, 2023

naumenkogs commented Nov 9, 2023

maflcko commented Nov 9, 2023

ryanofsky commented Nov 16, 2023

ryanofsky commented Oct 24, 2023 •

edited

DrahtBot commented Oct 24, 2023 •

edited