refactor: pull libbitcoin_server code out of wallet code 5/5 by knst · Pull Request #5767 · dashpay/dash

knst · 2023-12-15T19:58:50Z

Issue being fixed or feature implemented

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

This is final PR that includes relevant backport bitcoin#15639 and will unblock multiprocess: bitcoin#18677

With changes in this PR wallet would not use any server (node) code anymore; which includes llmq implementation, mempool, server-side CoinJoin and many other heavy things.

Eventually it may help to integrate dash wallet easier to 3rd party applications as a standalone library, for example in mobile wallet, hardware devices, etc.

Prior work:

What was done?

Refactored Coin Join related code:

general Coin Join code that doesn't depends from implementation moved to a new module coinjoin/common.
wallet uses client-only code
qt code doesn't include more any coinjoin header as before, coinjoin is available through interfaces/node
bunch of static methods +mutex +map from coinjoin.h become a new object CDSTXManager
deglobalized and completely removed ::coinJoinClientManagers in favor to cj_ctx->clientman

Backported related PRs from bitcoin:

build: Do not include server symbols in wallet bitcoin/bitcoin#19331

After adding missing changes from bitcoin#15638 and some extra improvements it was possible to finally backport bitcoin#15639

As a side effect one more circular dependency disappeared: coinjoin/client -> coinjoin/util -> wallet/wallet -> coinjoin/client

How Has This Been Tested?

Built 2 version: with and without these PRs: #5743, #5762 + this PR

-rwxr-xr-x  . . .   2372096  dash-cli
-rwxr-xr-x  . . .  19478944  dashd
-rwxr-xr-x  . . .  45950104  dash-qt
-rwxr-xr-x  . . .   4962688  dash-tx
-rwxr-xr-x  . . .  16049704  dash-wallet <------
-rwxr-xr-x  . . .  28106664  test_dash

and:

-rwxr-xr-x  . . .   2372096  dash-cli
-rwxr-xr-x  . . .  19503520  dashd
-rwxr-xr-x  . . .  45974680  dash-qt
-rwxr-xr-x  . . .   4962688  dash-tx
-rwxr-xr-x  . . .   9946088  dash-wallet <------
-rwxr-xr-x  . . .  28118952  test_dash

The binary dash-wallet lost ~40% of weight on this diet.

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

github-actions · 2023-12-17T18:45:08Z

This pull request has conflicts, please rebase.

github-actions · 2023-12-22T05:07:30Z

This pull request has conflicts, please rebase.

PastaPastaPasta · 2024-01-04T07:12:03Z

Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen.

PastaPastaPasta · 2024-01-04T08:00:04Z

Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen.

PastaPastaPasta · 2024-01-04T08:06:03Z

Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen.

PastaPastaPasta · 2024-01-09T18:24:14Z

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5767.8698bae1. The image should be on dockerhub soon.

PastaPastaPasta

utACK; please state preference for squash / merge commit

I really don't like "Loader" but 🤷 all other comments were addressed or not relevant

knst · 2024-01-10T07:12:39Z

please state preference for squash / merge commit

There are 2 backports, should be Merge commit, not squash

UdjinM6

re-utACK

This code can be used in both server side code and in wallet code

…pendency

Its class CDSTXManager has a lot of ex-static functions and data that are actually meant to be an object

…ide CJ code

…njoin

…keWallet

…nterface method for flush

…ce to unique_ptr

…ace and WalletContext

faca730 ci: Install fixed version of clang-format for linters (MarcoFalke) fa4695d build: Sort Makefile.am after renaming file (MarcoFalke) cccc278 scripted-diff: Move ui_interface to the node lib (MarcoFalke) fa72ca6 qt: Remove unused includes (MarcoFalke) fac96e6 wallet: Do not include server symbols (MarcoFalke) fa0f6c5 Revert "Fix link error with --enable-debug" (MarcoFalke) Pull request description: This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error. The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless `-O0`) because our compilers were smart enough to strip unused symbols. Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server. ACKs for top commit: Sjors: ACK faca730 laanwj: ACK faca730 hebasto: re-ACK faca730, since the [previous](bitcoin#19331 (review)) review: Tree-SHA512: e9731f249425aaea50b6db5fc7622e10078cf006721bb87989cac190a2ff224412f6f8a7dd83efd018835302337611f5839e29e15bef366047ed591cef58dfb4

knst added the guix-build label Dec 15, 2023

knst added this to the 20.1 milestone Dec 15, 2023

knst requested review from PastaPastaPasta and UdjinM6 December 15, 2023 19:58

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

knst changed the title ~~refactor: pull libbitcoin_server code out of wallet code 5/5~~ re\factor: pull libbitcoin_server code out of wallet code 5/5 Dec 15, 2023

knst changed the title ~~re\factor: pull libbitcoin_server code out of wallet code 5/5~~ refactor: pull libbitcoin_server code out of wallet code 5/5 Dec 15, 2023

knst force-pushed the bitcoinserver-15639-p3 branch 2 times, most recently from 9309fd7 to 6444c8c Compare December 16, 2023 20:29

knst removed the guix-build label Dec 16, 2023

knst force-pushed the bitcoinserver-15639-p3 branch 3 times, most recently from ae246f9 to 12636a3 Compare December 17, 2023 06:54

github-actions Bot added the needs rebase label Dec 17, 2023

knst force-pushed the bitcoinserver-15639-p3 branch from 12636a3 to 82a953b Compare December 17, 2023 18:55

github-actions Bot removed the needs rebase label Dec 17, 2023

knst force-pushed the bitcoinserver-15639-p3 branch from 82a953b to d8f4e72 Compare December 18, 2023 09:24

github-actions Bot added the needs rebase label Dec 22, 2023

knst force-pushed the bitcoinserver-15639-p3 branch from d8f4e72 to 0a5912c Compare December 22, 2023 08:19

github-actions Bot removed the needs rebase label Dec 22, 2023

knst force-pushed the bitcoinserver-15639-p3 branch from 0a5912c to fce73c9 Compare December 22, 2023 09:11

knst added the guix-build label Jan 4, 2024

knst force-pushed the bitcoinserver-15639-p3 branch from 9b67633 to 29809a1 Compare January 4, 2024 07:58

knst force-pushed the bitcoinserver-15639-p3 branch from 29809a1 to ab5a01a Compare January 4, 2024 08:05

knst force-pushed the bitcoinserver-15639-p3 branch from ab5a01a to c86999d Compare January 4, 2024 19:12

knst requested a review from PastaPastaPasta January 9, 2024 18:02

PastaPastaPasta approved these changes Jan 10, 2024

View reviewed changes

PastaPastaPasta requested a review from UdjinM6 January 10, 2024 03:02

UdjinM6 approved these changes Jan 10, 2024

View reviewed changes

knst and others added 22 commits January 10, 2024 12:05

fix: follow-up bitcoin#15638 to move AnalyzePSBT related code

28b12e2

refactor: new coinjoin/common module with utils code

b997996

This code can be used in both server side code and in wallet code

refactor: rename namespace CCoinJoin to CoinJoin

ad48a74

fix namespace CCoinjoin

ac1d596

Merge bitcoin#15639: bitcoin-wallet tool: Drop libbitcoin_server.a de…

89496e1

…pendency

refactor: create a new global object dstxManager

e23b07b

Its class CDSTXManager has a lot of ex-static functions and data that are actually meant to be an object

refactor: moved call g_wallet_init_interface.InitCoinJoinSettings ins…

f6f6131

…ide CJ code

refactor: assert of masternodeSync existance moved from wallet to coi…

52153d9

…njoin

refactor: drop global param coinJoinClientManager from interfaces::Ma…

f035708

…keWallet

refactor: rename CJClientManager

b208e91

refactor: introduce and use CoinJoin::Loader interface

1fb184d

refactor: split CoinJoin interfaces out of wallet into their own files

66884d9

refactor: move client-side CoinJoin out of server

a477cdb

chore: drop circular dependency coinjoin/client <-> wallet

fc58293

refactor: remove dependency wallet/load on CoinJoin by creating new i…

4846c31

…nterface method for flush

refactor: pass wallet name instead of wallet itself when possible

60240b1

refactor: use coinjoin interfaces in qt

7fd30b5

refactor: drop global coinJoinWalletManager

dc5152b

fix: removed unused variables from wallet_tests

09207a4

refactor: working with raw pointer of coinjoin_loader instead referen…

0964068

…ce to unique_ptr

chore: add TODO accordingly init order issue for g_wallet_init_interf…

cd9af42

…ace and WalletContext

PastaPastaPasta force-pushed the bitcoinserver-15639-p3 branch from 8698bae to 13188e1 Compare January 10, 2024 18:06

PastaPastaPasta merged commit 09b5833 into dashpay:develop Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

refactor: pull libbitcoin_server code out of wallet code 5/5#5767
PastaPastaPasta merged 22 commits into
dashpay:developfrom
knst:bitcoinserver-15639-p3

knst commented Dec 15, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Dec 17, 2023

Uh oh!

github-actions Bot commented Dec 22, 2023

Uh oh!

PastaPastaPasta commented Jan 4, 2024

Uh oh!

PastaPastaPasta commented Jan 4, 2024

Uh oh!

PastaPastaPasta commented Jan 4, 2024

Uh oh!

PastaPastaPasta commented Jan 9, 2024

Uh oh!

PastaPastaPasta left a comment

Uh oh!

knst commented Jan 10, 2024

Uh oh!

UdjinM6 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

knst commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions Bot commented Dec 17, 2023

Uh oh!

github-actions Bot commented Dec 22, 2023

Uh oh!

PastaPastaPasta commented Jan 4, 2024

Uh oh!

PastaPastaPasta commented Jan 4, 2024

Uh oh!

PastaPastaPasta commented Jan 4, 2024

Uh oh!

PastaPastaPasta commented Jan 9, 2024

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

knst commented Jan 10, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knst commented Dec 15, 2023 •

edited

Loading