refactor: pull libbitcoin_server code out of wallet code 3/N #5743

knst · 2023-11-29T10:38:15Z

Issue being fixed or feature implemented

Wallet library should not depends on server code (libbitcoin_server).

Untie this library will help to finish backport bitcoin#15639 and will unblock multiprocess: bitcoin#18677

Prior work:

merge bitcoin#15638: Pull wallet code out of libbitcoin_server #4560 (1/N)
refactor: remove dependency of dummyWallet in RecoverDatabaseFile on NodeContext #5178 (2/N)

What was done?

using havePruned from interface instead direct fPrune
removed dependency scriptpubkeyman on uiInterface
removed usage fShutdown from scriptpubkeyman
usages llmq::quorumInstantSendManager/llmq::chainLocksHandler are wrapper in interface
dropped direct usages of deterministicMNManager from wallet code

How Has This Been Tested?

Amount of linkage errors is decreased from 196 to 141 if libbitcoin_server is excluded during linkage:

diff --git a/src/Makefile.am b/src/Makefile.am
index fb4850429f..c9c9c331a7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -865,9 +865,7 @@ endif
 # https://eli.thegreenplace.net/2013/07/09/library-order-in-static-linking#circular-dependency)
 dash_wallet_LDADD = \
   $(LIBBITCOIN_WALLET_TOOL) \
-  $(LIBBITCOIN_SERVER) \
   $(LIBBITCOIN_WALLET) \
-  $(LIBBITCOIN_SERVER) \
   $(LIBBITCOIN_COMMON) \
   $(LIBBITCOIN_CONSENSUS) \
   $(LIBBITCOIN_UTIL) \

Breaking Changes

N/A

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

src/wallet/scriptpubkeyman.cpp

UdjinM6 · 2023-11-30T16:20:00Z

tool_wallet.py is crashing... 🙈 this should help

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 206ff59f54..81ac0b7edb 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -5517,7 +5517,7 @@ bool CWallet::GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, co
 
 bool CWallet::ShutdownRequested()
 {
-    return chain().shutdownRequested();
+    return HaveChain() && chain().shutdownRequested();
 }
 
 void CWallet::UpdateProgress(const std::string& title, int nProgress)

UdjinM6

LGTM, light ACK 👍

github-actions · 2023-12-04T02:35:45Z

This pull request has conflicts, please rebase.

UdjinM6

re-utACK

UdjinM6 · 2023-12-11T19:05:27Z

ping @PastaPastaPasta

PastaPastaPasta · 2023-12-11T21:52:28Z

src/wallet/wallet.h

it'd be nice to use string_view here but I think ShowProgress probably wants a std::string

yes, ShowProgress indeed uses std::string even in the latest version of bitcoin core:

boost::signals2::signal<void (const std::string &title, int nProgress)> ShowProgress;

The strMsg that is used in scriptpubkeyman.cpp is also std::string, so, using string_view will add a new allocation here, I will keep it as it is.

PastaPastaPasta · 2023-12-11T21:53:18Z

src/interfaces/chain.h

please rework this comment; doesn't make sense

PastaPastaPasta · 2023-12-11T21:56:56Z

src/wallet/wallet.cpp

refactor to use std::all_of, std::any_of or std::none_of ?

that's not new code, I just moved a function to a new place. Anyway, I provided a refactoring in an extra commit

UdjinM6

re-utACK

PastaPastaPasta

utACK for merging via merge commit

…ted introduced in dashpay#5456

…hain's interface

…global variable in wallet's code

…stMNCollaterials

knst added this to the 20.1 milestone Nov 29, 2023

UdjinM6 reviewed Nov 29, 2023

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

knst force-pushed the bitcoinserver-15639-p1 branch from 71292ec to 3f77c9d Compare November 30, 2023 15:11

knst requested review from PastaPastaPasta and UdjinM6 November 30, 2023 15:12

knst force-pushed the bitcoinserver-15639-p1 branch from 3f77c9d to c234a00 Compare November 30, 2023 15:50

knst force-pushed the bitcoinserver-15639-p1 branch from c234a00 to a2ca4a1 Compare November 30, 2023 17:33

UdjinM6 previously approved these changes Nov 30, 2023

View reviewed changes

github-actions bot added the needs rebase label Dec 4, 2023

knst dismissed UdjinM6’s stale review via 845d5d5 December 4, 2023 07:27

knst force-pushed the bitcoinserver-15639-p1 branch from a2ca4a1 to 845d5d5 Compare December 4, 2023 07:27

github-actions bot removed the needs rebase label Dec 4, 2023

UdjinM6 previously approved these changes Dec 5, 2023

View reviewed changes

knst mentioned this pull request Dec 11, 2023

refactor: pull libbitcoin_server (governance) code out of wallet code 4/N #5762

Merged

5 tasks

PastaPastaPasta reviewed Dec 11, 2023

View reviewed changes

knst dismissed UdjinM6’s stale review via 2e0bd50 December 12, 2023 10:34

knst force-pushed the bitcoinserver-15639-p1 branch from 845d5d5 to 2e0bd50 Compare December 12, 2023 10:34

knst requested a review from PastaPastaPasta December 12, 2023 10:35

UdjinM6 approved these changes Dec 12, 2023

View reviewed changes

knst mentioned this pull request Dec 15, 2023

refactor: pull libbitcoin_server code out of wallet code 5/5 #5767

Merged

5 tasks

knst changed the title ~~refactor: Pull wallet code out of libbitcoin_server 3/N~~ refactor: pull libbitcoin_server code out of wallet code 3/N Dec 15, 2023

PastaPastaPasta approved these changes Dec 17, 2023

View reviewed changes

follow-up bitcoin#15288 - missing fPrune usage

aa48c22

knst added 8 commits December 17, 2023 12:42

refactor: remove dependency scriptpubkeyman on uiInterface

d95d7f6

fix: dirty removal of dependency of scriptpubkeyman on ShutdownReques…

808e631

…ted introduced in dashpay#5456

refactor: replace llmq::quorumInstantSendManager->IsLocked() to new c…

0fea98b

…hain's interface

refactor: removed usage of llmq::chainLocksHandler from wallet

41b71dc

refactor: use std::all_of in isGroupISLocked

2310e35

refactor: use static method CDeterministicMNManager without touching …

4966bf7

…global variable in wallet's code

refactor: new method listMNCollaterials() for chain interface

2c61d84

refactor: replace usage of deterministicMNManager to new interface li…

e3d9327

…stMNCollaterials

PastaPastaPasta force-pushed the bitcoinserver-15639-p1 branch from 2e0bd50 to e3d9327 Compare December 17, 2023 18:42

PastaPastaPasta merged commit 714be63 into dashpay:develop Dec 17, 2023

refactor: pull libbitcoin_server code out of wallet code 3/N #5743

refactor: pull libbitcoin_server code out of wallet code 3/N #5743

Uh oh!

Conversation

knst commented Nov 29, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

Uh oh!

UdjinM6 commented Nov 30, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 4, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Dec 11, 2023

Uh oh!

PastaPastaPasta Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

knst Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

knst Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants