Backport 12610 + PrivateSend multiwallet support. #3604

PastaPastaPasta · 2020-07-12T06:05:44Z

Currently when trying to mix, it fails at assert(mixingWallet != nullptr); in GetMixingWallet() with below stack trace. However, I don't really understand how that happens... Because, mixingWallet and fPrivateSendRunning should be changed in tandem, when fPrivateSendRunning gets set to false mixingWallet gets set to nullPtr and anytime fPrivateSendRunning gets set to true mixingWallet gets set to a valid wallet pointer. Yet somehow, GetMixingWallet() is getting called in DoAutomaticDenominating when mixingWallet == nullptr yet that is protected behind if (!fEnablePrivateSend || !fPrivateSendRunning) return false;

I'm done trying to figure it out for today. So if you have any thoughts (on either the backport or the wip privatesend stuff) let me know.

ubuntu@ubuntu-Ryzen:~/workspace/dash$ ./src/qt/dash-qt --testnet --wallet=wallet.dat --wallet=wallet2
dash-qt: privatesend/privatesend-client.cpp:378: CWallet* CPrivateSendClientManager::GetMixingWallet() const: Assertion `mixingWallet != nullptr' failed.
Posix Signal: Aborted
   0#: (0x564D98F93765) stl_vector.h:466            - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&)
   1#: (0x564D98F93765) stacktraces.cpp:775         - HandlePosixSignal
   2#: (0x7F03B87DC8A0) <unknown-file>              - ???
   3#: (0x7F03B7889F47) <unknown-file>              - ???
   4#: (0x7F03B788B8B1) <unknown-file>              - ???
   5#: (0x7F03B787B42A) <unknown-file>              - ???
   6#: (0x7F03B787B4A2) <unknown-file>              - ???
   7#: (0x564D98DA5B76) privatesend-client.cpp:378  - ???
   8#: (0x564D98DC9571) std_mutex.h:232             - CPrivateSendClientManager::GetMixingWallet() const
   9#: (0x564D98DC9571) privatesend-client.cpp:957  - CPrivateSendClientManager::DoAutomaticDenominating(CConnman&, bool)
  10#: (0x564D98DC9BE5) privatesend-client.cpp:1759 - CPrivateSendClientManager::DoMaintenance(CConnman&)
  11#: (0x564D98F5C024) scheduler.cpp:123           - Repeat
  12#: (0x564D98F5DCD5) std_function.h:275          - std::_Function_base::~_Function_base()
  13#: (0x564D98F5DCD5) std_function.h:389          - std::function<void ()>::~function()
  14#: (0x564D98F5DCD5) bind.hpp:398                - void boost::_bi::list3<boost::_bi::value<CScheduler*>, boost::_bi::value<std::function<void ()> >, boost::_bi::value<long> >::operator()<void (*)(CScheduler*, std::function<void ()>, long), boost::_bi::list0>(boost::_bi::type<void>, void (*&)(CScheduler*, std::function<void ()>, long), boost::_bi::list0&, int)
  15#: (0x564D98F5DCD5) bind.hpp:1294               - boost::_bi::bind_t<void, void (*)(CScheduler*, std::function<void ()>, long), boost::_bi::list3<boost::_bi::value<CScheduler*>, boost::_bi::value<std::function<void ()> >, boost::_bi::value<long> > >::operator()()
  16#: (0x564D98F5DCD5) std_function.h:316          - std::_Function_handler<void (), boost::_bi::bind_t<void, void (*)(CScheduler*, std::function<void ()>, long), boost::_bi::list3<boost::_bi::value<CScheduler*>, boost::_bi::value<std::function<void ()> >, boost::_bi::value<long> > > >::_M_invoke(std::_Any_data const&)
  17#: (0x564D98F5CC1C) lock_types.hpp:336          - boost::unique_lock<boost::mutex>::lock()
  18#: (0x564D98F5CC1C) reverselock.h:22            - reverse_lock<boost::unique_lock<boost::mutex> >::~reverse_lock()
  19#: (0x564D98F5CC1C) scheduler.cpp:82            - CScheduler::serviceQueue()
  20#: (0x564D989B0EDD) util.h:445                  - void TraceThread<std::function<void ()> >(char const*, std::function<void ()>)
  21#: (0x564D989B2EF7) std_function.h:275          - std::_Function_base::~_Function_base()
  22#: (0x564D989B2EF7) std_function.h:389          - std::function<void ()>::~function()
  23#: (0x564D989B2EF7) bind.hpp:319                - void boost::_bi::list2<boost::_bi::value<char const*>, boost::_bi::value<std::function<void ()> > >::operator()<void (*)(char const*, std::function<void ()>), boost::_bi::list0>(boost::_bi::type<void>, void (*&)(char const*, std::function<void ()>), boost::_bi::list0&, int)
  24#: (0x564D989B2EF7) bind.hpp:1294               - boost::_bi::bind_t<void, void (*)(char const*, std::function<void ()>), boost::_bi::list2<boost::_bi::value<char const*>, boost::_bi::value<std::function<void ()> > > >::operator()()
  25#: (0x564D989B2EF7) thread.hpp:116              - boost::detail::thread_data<boost::_bi::bind_t<void, void (*)(char const*, std::function<void ()>), boost::_bi::list2<boost::_bi::value<char const*>, boost::_bi::value<std::function<void ()> > > > >::run()
  26#: (0x564D990CD77D) <unknown-file>              - ???
Aborted (core dumped)

xdustinface · 2020-07-12T13:54:01Z

Haven't looked a lot at the whole picture or reviewed the other changes but here are some things i would apply to make things running again https://github.com/xdustinface/dash/commits/backport-12610 Need to go now but will give this a better review later.

PastaPastaPasta · 2020-07-13T02:49:30Z

Branch looks good. Cherry-picked

xdustinface · 2020-07-14T00:34:06Z

One more here to fix the empty space in the toolbar, place the selector properly between the last tab button and the logo, and remove the "Wallet" label as i would say thats not really needed there? 400070b

Result looks like

There are still some issues.. can you check if thats something you will fix with backports? If not, i can fix it if you want me to.

The wallet selectors do not appear dynamically if you load a wallet in runtime with loadwallet. That should work imo.
If the wallet selectors are already there due to running with two -wallet args they do not update if you load additional wallets with loadwallet
The selector in the rpc console has a (none) entry.. does this make any sense? If you select this entry and run commands this just leads to Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path). (code -19)

PastaPastaPasta · 2020-07-15T17:36:54Z

Cherry-picked commit. Looks good

The wallet selectors do not appear dynamically if you load a wallet in runtime with loadwallet. That should work imo.
If the wallet selectors are already there due to running with two -wallet args they do not update if you load additional wallets with loadwallet

wallets loaded dynamicly in the GUI is done in bitcoin#13097

The selector in the rpc console has a (none) entry.. does this make any sense? If you select this entry and run commands this just leads to Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path). (code -19)

Compiling 0.17 of bitcoin it appears they have it too. Probably makes sense if you are doing commands that shouldn't be touching any wallet you set it as none and then it wont touch a wallet

xdustinface · 2020-07-15T20:01:11Z

wallets loaded dynamicly in the GUI is done in bitcoin#13097

ok cool! will you add it to this PR or separate one?

The selector in the rpc console has a (none) entry.. does this make any sense? If you select this entry and run commands this just leads to Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path). (code -19)
Compiling 0.17 of bitcoin it appears they have it too. Probably makes sense if you are doing commands that shouldn't be touching any wallet you set it as none and then it wont touch a wallet

Hm, still weird imo and i would choose "oh yeah, please remove it!" in a poll, but ok 😅

PastaPastaPasta · 2020-07-15T22:47:16Z

It'll be done in a separate PR.

Also, Udjin, after you review, I'm going to squash PrivateSend changes so yeah. DO NOT MERGE

PastaPastaPasta · 2020-07-16T02:43:06Z

Okay, I changed my mind and squashed those commits before Udjin reviews.

UdjinM6 · 2020-07-16T12:32:59Z

I think more internal changes/refactoring is needed to make PS really work with the multiwallet setup, pls see https://github.com/UdjinM6/dash/commits/pr3604_2 (this branch seems to be working but might need more fixes, testing it atm).

PastaPastaPasta · 2020-07-16T16:41:25Z

Cherry-picked. They look good and generally make sense

UdjinM6 · 2020-07-16T23:38:00Z

Added couple more fixes to my branch: deb4491 to fix getprivatesendinfo (this one fixes privatesend.py failure) and d1451d5 as a general cleanup.

xdustinface · 2020-07-17T00:32:13Z

What about this e46c2f0 on top of udjin's last suggestions? (The locks are probably not really needed but i guess they also don't hurt there?)

UdjinM6 · 2020-07-17T10:48:08Z

e46c2f0 makes sense and looks good too imo 👍

PastaPastaPasta · 2020-07-17T18:43:43Z

All looks good, cherry-picked

PastaPastaPasta · 2020-07-17T18:45:19Z

Should I squash those PS commits into aadb111 ?

UdjinM6

ACK

(re squashing: it's fine to merge as is imo)

UdjinM6 · 2020-07-20T15:05:00Z

Needs rebase

779c5f9 Qt: hide RPCConsole wallet selector when no wallets are present (Jonas Schnelli) dc6f150 Qt: show wallet name in request dlg in case of multiwallet (Jonas Schnelli) 4826ca4 Qt: show wallet name in send confirmation dlg in case of multiwallet (Jonas Schnelli) cfa4133 GUI: RPCConsole: Log wallet changes (Luke Dashjr) b6d04fc Qt: Get wallet name from WalletModel rather than passing it around (Luke Dashjr) 12d8d26 Qt: When multiple wallets are used, include in notifications the name (Jonas Schnelli) d1ec34a Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed (Luke Dashjr) d49cc70 Qt: Add wallet selector to debug console (Jonas Schnelli) d558f44 Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ (Luke Dashjr) 85d5319 Qt: Ensure UI updates only come from the currently selected walletView (Luke Dashjr) e449f9a Qt: Add a combobox to toolbar to select from multiple wallets (Luke Dashjr) 3dba3c3 Qt: Load all wallets into WalletModels (Luke Dashjr) Pull request description: This is an overhaul of bitcoin#11383 (plus some additions). It avoids unnecessary coupling of httpserver/jsonrpc and the wallet as well as it avoids pointer pure passing (and pointer deletion) of `CWallet` (plus other minor design changes). Additionally it adds the wallet name to the sendconfirmation and request dialog (in case multiwallet is active) Tree-SHA512: 3d06e18badbc5d1821e488bf1dae463bb0be544cf11b2b618e025812bfdd13c5f39604bb93b4c705313930e7dc4e66f4848b9469ba14871bade58e7a027246a1

Signed-off-by: pasta <pasta@dashboost.org>

With the introduction of better mutliwallet support, privatesend only worked for your first wallet. This commit implements multiwallet for privatesend such that the wallet you start the mixing from is the wallet mixing will occur in. Signed-off-by: pasta <pasta@dashboost.org>

Signed-off-by: pasta <pasta@dashboost.org>

…ssion` via ctor

Just verify that it's of a proper type instead

- Makes it singleton to make sure we always only have one instance of it - Protects its members by making them private and adding set/getters - Makes it thread safe

PastaPastaPasta · 2020-07-20T15:28:21Z

Rebased, QT unit tests now failing

UdjinM6 · 2020-07-20T16:24:52Z

0a6b5c5 should fix it

PastaPastaPasta · 2020-07-20T16:29:26Z

cherry-picked. Fixed tests, however, attached is still a problem.

xdustinface · 2020-07-20T16:33:04Z

~~@UdjinM6 thoughts about cdfeb27 as replacement for 0a6b5c5 ?~~

Has been settled!

xdustinface · 2020-07-20T17:01:14Z

cherry-picked. Fixed tests, however, attached is still a problem.

I can't verify right now as i don't have the issue on here on macOS but this one should fix it ba17d30

Showing it instantly may lead to the selector showing up in an unexpected place because it only gets added to a proper layout when a second wallet gets added.

PastaPastaPasta · 2020-07-20T17:05:30Z

appears to fix the issue and works. Cherry-picked and pushed

UdjinM6

ACK

xdustinface · 2020-07-21T00:00:36Z

ACK, seems to work and look as expected now.

- BitcoinGUI::updateWalletStatus() - Make WalletFrame::currentWalletView public

- Add BitcoinGUI::updateWalletStatus() - Add WalletModel::getWalletModel() - Make WalletFrame::currentWalletView public

UdjinM6 added this to the 17 milestone Jul 13, 2020

PastaPastaPasta force-pushed the backport-12610 branch from d95c23c to c123399 Compare July 16, 2020 02:42

PastaPastaPasta force-pushed the backport-12610 branch from c123399 to de1a4cc Compare July 16, 2020 16:41

PastaPastaPasta requested a review from UdjinM6 July 19, 2020 07:12

UdjinM6 changed the title ~~Backport 12610 + WIP privatesend multiwallet support.~~ Backport 12610 + PrivateSend multiwallet support. Jul 19, 2020

UdjinM6 previously approved these changes Jul 19, 2020

View reviewed changes

jonasschnelli and others added 9 commits July 20, 2020 10:09

fix incomingTransaction notifications

a670374

Signed-off-by: pasta <pasta@dashboost.org>

qt: Add the wallet selector widget before the dash logo in the toolbar

c240e70

Split mixing options out of mixing manager

167e0f4

Split dsq processing out of mixing manager

59bc2ca

Signed-off-by: pasta <pasta@dashboost.org>

Drop GetMixingWallet()s, pass CWallet* into `CPrivateSendClientSe…

11255fd

…ssion` via ctor

Create one PS manager per wallet

a7e0957

Do not clear the object pased into CPrivateSendClient*::GetJsonInfo()

4ac00e5

Just verify that it's of a proper type instead

UdjinM6 and others added 2 commits July 20, 2020 10:10

Add CPrivateSendClientOptions::GetJsonInfo()

50b09b8

privatesend|qt|rpc|wallet: Improve CPrivateSendClientOptions

8fedc08

- Makes it singleton to make sure we always only have one instance of it - Protects its members by making them private and adding set/getters - Makes it thread safe

PastaPastaPasta dismissed UdjinM6’s stale review via 8fedc08 July 20, 2020 15:28

PastaPastaPasta force-pushed the backport-12610 branch from b086f4e to 8fedc08 Compare July 20, 2020 15:28

Fix qt tests

3d5c35c

qt: Hide m_wallet_selector until a second wallet gets added.

449b4a8

Showing it instantly may lead to the selector showing up in an unexpected place because it only gets added to a proper layout when a second wallet gets added.

UdjinM6 approved these changes Jul 20, 2020

View reviewed changes

xdustinface approved these changes Jul 21, 2020

View reviewed changes

UdjinM6 merged commit 95f925f into dashpay:develop Jul 21, 2020

PastaPastaPasta deleted the backport-12610 branch July 29, 2020 19:59

xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020

Partially merge dashpay#3604: Only BitcoinGUI::updateWalletStatus()

5052aa4

xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020

Partially merge dashpay#3604:

55fec55

- BitcoinGUI::updateWalletStatus() - Make WalletFrame::currentWalletView public

xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020

Partially merge dashpay#3604:

9e425d6

- Add BitcoinGUI::updateWalletStatus() - Add WalletModel::getWalletModel() - Make WalletFrame::currentWalletView public

xdustinface mentioned this pull request Sep 13, 2020

[v0.16.x] More UI + PrivateSend/MN-Sync improvements #3705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport 12610 + PrivateSend multiwallet support. #3604

Backport 12610 + PrivateSend multiwallet support. #3604

PastaPastaPasta commented Jul 12, 2020

xdustinface commented Jul 12, 2020

PastaPastaPasta commented Jul 13, 2020

xdustinface commented Jul 14, 2020

PastaPastaPasta commented Jul 15, 2020

xdustinface commented Jul 15, 2020

PastaPastaPasta commented Jul 15, 2020

PastaPastaPasta commented Jul 16, 2020

UdjinM6 commented Jul 16, 2020

PastaPastaPasta commented Jul 16, 2020

UdjinM6 commented Jul 16, 2020 •

edited

xdustinface commented Jul 17, 2020 •

edited

UdjinM6 commented Jul 17, 2020

PastaPastaPasta commented Jul 17, 2020

PastaPastaPasta commented Jul 17, 2020

UdjinM6 left a comment

UdjinM6 commented Jul 20, 2020

PastaPastaPasta commented Jul 20, 2020

UdjinM6 commented Jul 20, 2020

PastaPastaPasta commented Jul 20, 2020

xdustinface commented Jul 20, 2020 •

edited

xdustinface commented Jul 20, 2020

PastaPastaPasta commented Jul 20, 2020

UdjinM6 left a comment

xdustinface commented Jul 21, 2020

Backport 12610 + PrivateSend multiwallet support. #3604

Backport 12610 + PrivateSend multiwallet support. #3604

Conversation

PastaPastaPasta commented Jul 12, 2020

xdustinface commented Jul 12, 2020

PastaPastaPasta commented Jul 13, 2020

xdustinface commented Jul 14, 2020

PastaPastaPasta commented Jul 15, 2020

xdustinface commented Jul 15, 2020

PastaPastaPasta commented Jul 15, 2020

PastaPastaPasta commented Jul 16, 2020

UdjinM6 commented Jul 16, 2020

PastaPastaPasta commented Jul 16, 2020

UdjinM6 commented Jul 16, 2020 • edited

xdustinface commented Jul 17, 2020 • edited

UdjinM6 commented Jul 17, 2020

PastaPastaPasta commented Jul 17, 2020

PastaPastaPasta commented Jul 17, 2020

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jul 20, 2020

PastaPastaPasta commented Jul 20, 2020

UdjinM6 commented Jul 20, 2020

PastaPastaPasta commented Jul 20, 2020

xdustinface commented Jul 20, 2020 • edited

xdustinface commented Jul 20, 2020

PastaPastaPasta commented Jul 20, 2020

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface commented Jul 21, 2020

UdjinM6 commented Jul 16, 2020 •

edited

xdustinface commented Jul 17, 2020 •

edited

xdustinface commented Jul 20, 2020 •

edited