refactor: pull libbitcoin_server code out of wallet code 3/N #5743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
UdjinM6
reviewed
Nov 29, 2023
knst
force-pushed
the
bitcoinserver-15639-p1
branch
from
71292ec
to
3f77c9d
Compare
knst
force-pushed
the
bitcoinserver-15639-p1
branch
from
3f77c9d
to
c234a00
Compare
|
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) |
knst
force-pushed
the
bitcoinserver-15639-p1
branch
from
c234a00
to
a2ca4a1
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
bitcoinserver-15639-p1
branch
from
a2ca4a1
to
845d5d5
Compare
knst
added a commit
to knst/dash
that referenced
this pull request
Dec 11, 2023
…of libbitcoin_server 3/N
5 tasks
|
ping @PastaPastaPasta |
| @@ -855,6 +855,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati | |||
| bool IsLocked(bool fForMixing = false) const override; | |||
| bool Lock(bool fForMixing = false); | |||
|
|
|||
| void UpdateProgress(const std::string& title, int nProgress) override; | |||
There was a problem hiding this comment.
it'd be nice to use string_view here but I think ShowProgress probably wants a std::string
There was a problem hiding this comment.
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.
src/interfaces/chain.h
Outdated
| @@ -135,6 +135,9 @@ class Chain | |||
| //! Check if transaction will be final given chain height current time. | |||
| virtual bool checkFinalTx(const CTransaction& tx) = 0; | |||
|
|
|||
| //! Check if transaction is locked as the instant tx. | |||
There was a problem hiding this comment.
please rework this comment; doesn't make sense
src/wallet/wallet.cpp
Outdated
| for (const auto& output : group.m_outputs) { | ||
| if (!chain.isInstantSendLockedTx(output.outpoint.hash)) return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
refactor to use std::all_of, std::any_of or std::none_of ?
There was a problem hiding this comment.
that's not new code, I just moved a function to a new place. Anyway, I provided a refactoring in an extra commit
knst
force-pushed
the
bitcoinserver-15639-p1
branch
from
845d5d5
to
2e0bd50
Compare
knst
added a commit
to knst/dash
that referenced
this pull request
Dec 15, 2023
…of libbitcoin_server 3/N
knst
added a commit
to knst/dash
that referenced
this pull request
Dec 15, 2023
5 tasks
knst
changed the title
knst
added a commit
to knst/dash
that referenced
this pull request
Dec 16, 2023
…of libbitcoin_server 3/N
PastaPastaPasta
approved these changes
Dec 17, 2023
…global variable in wallet's code
PastaPastaPasta
force-pushed
the
bitcoinserver-15639-p1
branch
from
2e0bd50
to
e3d9327
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
havePrunedfrom interface instead directfPruneuiInterfacefShutdownfrom scriptpubkeymanllmq::quorumInstantSendManager/llmq::chainLocksHandlerare wrapper in interfacedeterministicMNManagerfrom wallet codeHow Has This Been Tested?
Amount of linkage errors is decreased from 196 to 141 if
libbitcoin_serveris excluded during linkage:Breaking Changes
N/A
Checklist: