refactor: pull libbitcoin_server code out of wallet code 3/N by knst · Pull Request #5743 · dashpay/dash

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

refactor: pull libbitcoin_server code out of wallet code 3/N#5743
PastaPastaPasta merged 9 commits intodashpay:developfrom
knst:bitcoinserver-15639-p1

knst commented Nov 29, 2023

Uh oh!

Uh oh!

UdjinM6 commented Nov 30, 2023

Uh oh!

UdjinM6 left a comment

Uh oh!

github-actions bot commented Dec 4, 2023

Uh oh!

UdjinM6 left a comment

Uh oh!

UdjinM6 commented Dec 11, 2023

Uh oh!

PastaPastaPasta Dec 11, 2023

Uh oh!

knst Dec 12, 2023

Uh oh!

PastaPastaPasta Dec 11, 2023

Uh oh!

PastaPastaPasta Dec 11, 2023

Uh oh!

knst Dec 12, 2023

Uh oh!

UdjinM6 left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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