Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wallet] Laggard wallet-related backports from btc 0.15 #2351

Merged

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Apr 28, 2021

I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us.
I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files).

This changeset is based on top of

as it keeps going with the multiwallet implementation, adding:

  • RPC endpoint support
  • listwallets RPC
  • wallet name in getwalletinfo RPC
  • wallet_multiwallet.py functional test

As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code.

List of upstream PRs backported/adapted:

@random-zebra
Copy link
Author

Rebased on master, now that #2337 has been merged.

@Fuzzbawls
Copy link
Collaborator

Fuzzbawls commented May 20, 2021

quick thing before doing a more complete review:

I see you've moved a lot of the tier two .cpp files out of the wallet library and into the common library for autotools builds in 206f3ca. would be a good idea to do the same for cmake builds as well to maintain concurrency between the two:

Index: CMakeLists.txt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt	(revision 9a868f8579704920271d0c259a28478c46704041)
+++ b/CMakeLists.txt	(date 1621481390924)
@@ -251,24 +251,10 @@
 endif()
 
 set(WALLET_SOURCES
-        ./src/activemasternode.cpp
         ./src/bip38.cpp
         ./src/wallet/db.cpp
         ./src/addressbook.cpp
         ./src/crypter.cpp
-        ./src/budget/budgetdb.cpp
-        ./src/budget/budgetmanager.cpp
-        ./src/budget/budgetproposal.cpp
-        ./src/budget/budgetvote.cpp
-        ./src/budget/finalizedbudget.cpp
-        ./src/budget/finalizedbudgetvote.cpp
-        ./src/masternode.cpp
-        ./src/masternode-payments.cpp
-        ./src/masternode-sync.cpp
-        ./src/tiertwo_networksync.cpp
-        ./src/masternodeconfig.cpp
-        ./src/masternodeman.cpp
-        ./src/messagesigner.cpp
         ./src/zpiv/mintpool.cpp
         ./src/wallet/hdchain.cpp
         ./src/wallet/rpcdump.cpp
@@ -355,8 +341,15 @@
 target_include_directories(ZEROCOIN_A PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/src)
 
 set(COMMON_SOURCES
+        ./src/activemasternode.cpp
         ./src/base58.cpp
         ./src/bip38.cpp
+        ./src/budget/budgetdb.cpp
+        ./src/budget/budgetmanager.cpp
+        ./src/budget/budgetproposal.cpp
+        ./src/budget/budgetvote.cpp
+        ./src/budget/finalizedbudget.cpp
+        ./src/budget/finalizedbudgetvote.cpp
         ./src/consensus/params.cpp
         ./src/consensus/upgrades.cpp
         ./src/chainparams.cpp
@@ -379,6 +372,12 @@
         ./src/invalid.cpp
         ./src/key.cpp
         ./src/keystore.cpp
+        ./src/masternode.cpp
+        ./src/masternode-payments.cpp
+        ./src/masternode-sync.cpp
+        ./src/masternodeconfig.cpp
+        ./src/masternodeman.cpp
+        ./src/messagesigner.cpp
         ./src/netaddress.cpp
         ./src/netbase.cpp
         ./src/policy/feerate.cpp
@@ -393,6 +392,7 @@
         ./src/script/script_error.cpp
         ./src/spork.cpp
         ./src/sporkdb.cpp
+        ./src/tiertwo_networksync.cpp
         ./src/warnings.cpp
         )
 add_library(COMMON_A STATIC ${BitcoinHeaders} ${COMMON_SOURCES})

@random-zebra
Copy link
Author

Good idea @Fuzzbawls 👍 . Patch squashed into 49c201e

furszy
furszy previously approved these changes May 27, 2021
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 1cb5cb2.
Left two small topic and a nit that can be done in a subsequent PR.

src/miner.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
furszy
furszy previously approved these changes May 28, 2021
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c710b03 after rebase and covered feedback.

@random-zebra
Copy link
Author

Rebased on master, fixing conflicts.

furszy
furszy previously approved these changes Jun 1, 2021
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK e840ab7 after rebase.

random-zebra and others added 20 commits June 2, 2021 10:55
Raise RPC_WALLET_NOT_SPECIFIED instead of RPC_METHOD_NOT_FOUND when a
required
wallet filename was not specified in an RPC call.

Also raise more specific RPC_WALLET_NOT_FOUND error instead of
RPC_INVALID_PARAMETER in case an invalid wallet was specified, for
consistency.
>>> backports bitcoin/bitcoin@d97fe20

This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.

This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.

Another proximate motivation is the wallet process separation work in
parameter parsing and fee estimation are still done in the main process
rather than the wallet process, and having functions that run in
different processes scrambled up throughout wallet.cpp is unnecessarily
confusing.
Changes the errors field to warnings. To maintain compatibility,
the errors field is deprecated and enabled by starting bitcoind with
-deprecatedrpc=getmininginfo
Now using a std::unique_ptr, the Db instance is correctly released
when CDB initialization fails.
The internal CDB state and mapFileUseCount are only mutated when
the CDB initialization succeeds.
Make sure wallet databases have unique fileids. If they don't, throw an
error. BDB caches do not work properly when more than one open database
has the same fileid, because values written to one database may show up
in reads to other databases.

Bitcoin will never create different databases with the same fileid, but
users can create them by manually copying database files.

BDB caching bug was reported by Chris Moore <dooglus@gmail.com>

Fixes bitcoin issue 11429
@random-zebra
Copy link
Author

Rebased on master, fixing conflicts.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-re-utACK d5526bd.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d5526bd

perpetual updating PIVX Core to BTC Core automation moved this from In Progress to Ready Jun 9, 2021
@random-zebra
Copy link
Author

Merging...

@random-zebra random-zebra merged commit 2d50b6e into PIVX-Project:master Jun 9, 2021
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet