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

knst · 2023-12-11T18:51:03Z

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:

What was done?

The class CGovernanceObject is doing too many things and doesn't have any public interface that can be used outside of governance module. This PR move publicly used functionality of CGovernanceObject to a new simple Governance::Object that can be used outside of governance module as a public interface for serialization/deserialization and can be used inside wallet.

As side effect have been removed 2 circular dependency:

-    "governance/object -> governance/validators -> governance/object"
-    "governance/object -> validationinterface -> governance/object"

How Has This Been Tested?

Amount of linkage errors is decreased from 141 to 66 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

…d governance

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

This pull request has conflicts, please rebase.

…d governance

PastaPastaPasta · 2023-12-19T04:09:01Z

src/util/governance.h

+
+namespace Governance
+{
+struct Object


this should be a class; while basically the same; we should stick to structs simply store some values; while classes also have methods and are generally more complex

agree, here's no complex logic but still 2 constructors and several helpers

PastaPastaPasta · 2023-12-19T04:11:44Z

src/util/governance.cpp

@@ -0,0 +1,73 @@
+// Copyright (c) 2014-2023 The Dash Core developers


why is this in the util/ folder?

moved to governance/common.h

PastaPastaPasta · 2023-12-19T04:13:18Z

src/util/governance.h

+        type{GovernanceObject::UNKNOWN},
+        hashParent{},
+        revision{0},
+        time{0},
+        collateralHash{},
+        vchData{}


this constructor should be replaced with a =default and then place all of these default values on the actual variables for example int64_t time{0};

PastaPastaPasta · 2023-12-19T04:24:04Z

src/rpc/governance.cpp

+    for (auto it = vecObjects.begin() + std::max<int>(0, vecObjects.size() - nCount); it != vecObjects.end(); ++it) {
+        jsonArray.push_back((*it)->ToJson());


changed from reverse iterators to non-reverse?

it's changed to reverse because std::sort is changed also order of sorting from A > B to A < B

PastaPastaPasta

utACK for either squash or merge via merge commit

knst · 2023-12-19T14:37:38Z

I'd prefer this one get merged as merge-commit - changes all related to governance, but not are same

UdjinM6

utACK

…/object

…alidators

knst added this to the 20.1 milestone Dec 11, 2023

knst changed the title ~~refactor: Pull wallet code out of libbitcoin_server 4/N~~ refactor: Pull wallet code out of libbitcoin_server 4/N [governance] Dec 11, 2023

knst changed the title ~~refactor: Pull wallet code out of libbitcoin_server 4/N [governance]~~ refactor(governance): Pull wallet code out of libbitcoin_server 4/N Dec 11, 2023

knst changed the title ~~refactor(governance): Pull wallet code out of libbitcoin_server 4/N~~ refactor: (governance) Pull wallet code out of libbitcoin_server 4/N Dec 11, 2023

knst changed the title ~~refactor: (governance) Pull wallet code out of libbitcoin_server 4/N~~ refactor(governance): (governance) Pull wallet code out of libbitcoin_server 4/N Dec 11, 2023

knst changed the title ~~refactor(governance): (governance) Pull wallet code out of libbitcoin_server 4/N~~ refactor: (governance) Pull wallet code out of libbitcoin_server 4/N Dec 11, 2023

knst force-pushed the bitcoinserver-15639-p2 branch from 58b517d to 41a76d8 Compare December 11, 2023 21:08

knst requested review from PastaPastaPasta and UdjinM6 and removed request for PastaPastaPasta December 12, 2023 10:49

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: (governance) Pull wallet code out of libbitcoin_server 4/N~~ refactor: pull libbitcoin_server (governance) code out of wallet code 4/N Dec 15, 2023

knst added a commit to knst/dash that referenced this pull request Dec 16, 2023

refactor: cummulative changes from dashpay#5762: libbitcoin_server an…

53b56fb

…d governance

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

knst force-pushed the bitcoinserver-15639-p2 branch from 41a76d8 to 15a1fb9 Compare December 17, 2023 18:50

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

knst added a commit to knst/dash that referenced this pull request Dec 17, 2023

refactor: cummulative changes from dashpay#5762: libbitcoin_server an…

44fed80

…d governance

PastaPastaPasta reviewed Dec 19, 2023

View reviewed changes

knst force-pushed the bitcoinserver-15639-p2 branch from 15a1fb9 to e352e21 Compare December 19, 2023 09:02

PastaPastaPasta approved these changes Dec 19, 2023

View reviewed changes

UdjinM6 approved these changes Dec 19, 2023

View reviewed changes

knst added 6 commits December 21, 2023 23:04

refactor: untie governance/object and wallet implementation

64a153d

refactor: a default constructor for Governance::Object

4de30f4

refactor: drop circular dependency validationinterface <-> governance…

7e13727

…/object

cleanup: removed unused definitions from governance/object.h

2bb6361

refactor: drop circular dependency governance/object <-> governance/v…

4083fff

…alidators

refactor: simplify comparator in rpc/governance

b2ad530

PastaPastaPasta force-pushed the bitcoinserver-15639-p2 branch from e352e21 to b2ad530 Compare December 22, 2023 05:04

PastaPastaPasta merged commit 9bdb746 into dashpay:develop Dec 22, 2023
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

knst commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 17, 2023

PastaPastaPasta Dec 19, 2023

knst Dec 19, 2023

PastaPastaPasta Dec 19, 2023

knst Dec 19, 2023

PastaPastaPasta Dec 19, 2023

PastaPastaPasta Dec 19, 2023

knst Dec 19, 2023

PastaPastaPasta left a comment

knst commented Dec 19, 2023

UdjinM6 left a comment

		@@ -0,0 +1,73 @@
		// Copyright (c) 2014-2023 The Dash Core developers

		for (auto it = vecObjects.begin() + std::max<int>(0, vecObjects.size() - nCount); it != vecObjects.end(); ++it) {
		jsonArray.push_back((*it)->ToJson());

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

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

Conversation

knst commented Dec 11, 2023 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

github-actions bot commented Dec 17, 2023

PastaPastaPasta Dec 19, 2023

Choose a reason for hiding this comment

knst Dec 19, 2023

Choose a reason for hiding this comment

PastaPastaPasta Dec 19, 2023

Choose a reason for hiding this comment

knst Dec 19, 2023

Choose a reason for hiding this comment

PastaPastaPasta Dec 19, 2023

Choose a reason for hiding this comment

PastaPastaPasta Dec 19, 2023

Choose a reason for hiding this comment

knst Dec 19, 2023

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst commented Dec 19, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

knst commented Dec 11, 2023 •

edited

Loading