backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403 #3682

xdustinface · 2020-08-30T18:49:06Z

This is mainly the backport of bitcoin#10244 (replaces #3638) which is actually more than enough for one PR but i added bitcoin#18123 because it fixes a bug introduced in bitcoin#10244 and its just a simple diff.

I added the dash related changes into the related commits of the original PR where it made sense.. if i would do this PR again i would separate them probably lol.. but well, its done like this now..hope its not too hard to review 🙈

352e7d3 unfortunately ended up being a bit too much imo.. besides removing the direct calls it also contains some refactoring.. should have done it in multiple steps but didn't expect it to become that big in the beginning.. i hope you can still get the reasonings for the changes.. if not, just ask :)

~~Based on #3680~~

PastaPastaPasta · 2020-09-20T22:12:55Z

Can you rebase/provide guidance on if this is ready for review?

xdustinface · 2020-09-20T22:17:28Z

Based on #3680

^^^ waiting for that to get your review before i rebase here.

xdustinface · 2020-09-29T23:24:10Z

Ok i guess this one is ready now! Happy reviewing 🙈

UdjinM6 · 2020-10-01T11:50:12Z

should revert/drop 9a07ca7 and backport bitcoin#12906 instead

xdustinface · 2020-10-01T13:34:45Z

should revert/drop 9a07ca7 and backport bitcoin#12906 instead

Done 👍

UdjinM6 · 2020-10-01T13:47:36Z

hmm.. would prefer it backported as a scripted-diff commit tbh

src/wallet/wallet.cpp

src/qt/paymentserver.cpp

src/interface/wallet.cpp

src/qt/transactiondesc.cpp

src/qt/transactiontablemodel.cpp

src/qt/addresstablemodel.cpp

UdjinM6 · 2020-10-01T13:54:42Z

src/interfaces/node.h

+};
+
+//! Interface for the src/masternode part of a dash node (dashd process).
+class Masternode


This is confusing imo. Would probably make sense to make Masternode a namespace and introduce Sync class inside of it or smth like that and then Masternode& masternode() -> Masternode::Sync& masternodeSync() etc. And tbh, the whole "masternode sync" name which comes from old non-DML times doesn't make sense anymore because we simply don't sync mn registrations/pings since ages 🙈 it's just governance and sporks now. We should probably refactor this in some future PR into smth like "extended sync" or whatever and move it out of masternode/.

I totally agree, this "masternode sync" related stuff needs to be refactored 😄 Thoughts about just moving

virtual bool isBlockchainSynced() = 0; virtual bool isSynced() = 0; virtual std::string getSyncStatus() = 0;

into interfaces::Node ? Like having calls like node.isSynced()

Kind of breaks with my initial idea i.e. having an interface for each subdirectory stuff but it somehow sounds more reasonable to have those methods in the Node interface..

If there are any concerns about moving it into Node i would probably just do the Masternode::Sync thing for now and revisit this in a future PR.

I'd say Masternode::Sync is the way to go.

src/interfaces/node.cpp

PastaPastaPasta · 2020-10-02T04:21:29Z

src/qt/addressbookpage.cpp

@@ -241,9 +241,7 @@ void AddressBookPage::on_showAddressQRCode_clicked()



changes in 4374d05 in this file aren't in the bitcoin commit. Stuff like this should be minimized (the changes in say optionsmodel.cpp are okay since they are all of the same kind as upstream commit as well as in the same file) and should instead imo be in a different commit to make review easier

Please explain these additional changes a bit

Hmm long time ago 🙈 but looks like its just because QDialog doesn't need it for anything, OptionsModel's constructor changed, so i just dropped the related stuff it seems. Also a7024cf now.

src/qt/qrdialog.cpp

src/qt/qrdialog.h

src/qt/transactionview.cpp

src/qt/splashscreen.cpp

PastaPastaPasta · 2020-10-02T06:19:30Z

src/wallet/wallet.cpp

@@ -5120,12 +5118,23 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
    // wallet creation fails.
    auto temp_wallet = MakeUnique<CWallet>(name, WalletDatabase::Create(path));
    CWallet *walletInstance = temp_wallet.get();
-    DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);
+    AddWallet(walletInstance);


src/interfaces/node.cpp

src/interfaces/wallet.cpp

PastaPastaPasta · 2020-10-02T06:24:18Z

src/qt/addresstablemodel.cpp

@@ -8,7 +8,7 @@
 #include <qt/guiutil.h>
 #include <qt/walletmodel.h>

-#include <interfaces/node.h>
+#include <interfaces/wallet.h>


~~Not sure where this came from but~~ Seems like that was a suggestion but it does not longer exist this way

src/qt/rpcconsole.cpp

src/interfaces/wallet.cpp

UdjinM6 · 2020-10-03T15:44:05Z

src/qt/overviewpage.cpp

-        const CAmount nMinAmount = CPrivateSend::GetSmallestDenomination() + CPrivateSend::GetMaxCollateralAmount();
+    if (!walletModel->privateSend().isMixing()) {
+        auto& options = walletModel->node().privateSendOptions();
+        const CAmount nMinAmount = options.getSmallestDenomination() + options.getMaxCollateralAmount();


Good question 👍 should probably be fixed by using tx builder instead (in a separate PR).

src/qt/overviewpage.cpp

UdjinM6

0fb1a05 works for me :) Maybe add a comment that we only do some simple checks here which should be enough to reliably detect mixing txes for this specific wallet and that this relies on PS subsystem verifying the tx earlier (during mixing) and wallet not participating in txes that won't fit more strict rules or smth like that.

see one tiny nit below

src/interfaces/wallet.cpp

xdustinface · 2020-10-09T14:34:02Z

I think 3433ec6 should fix it.

That looks better to me jup :)

UdjinM6

Everything seems to be working as expected now.

ACK

PastaPastaPasta · 2020-10-14T03:42:37Z

overall ACK, however I would like to see some commits merged. IMO all commits directly from 10244 should be one commit, then dash specific 10244 as one commit, then 18123 (then any associated review commits), then 12906 (then any associated review commits), then 11403 (then any associated review commits). It's difficult for me to tell at this point which review commits correspond to which actual backports

UdjinM6 · 2020-10-14T10:55:11Z

I don't think squashing multiple focused 10244 commits into one huge commit is a good idea, would rather prefer them as is. Squashing performance fixes after 18123 (b1bc1f1 + b46e65b + 34a146c) makes sense though. Squashing aa2ebe1 + 60a8584 might also be a good idea. 11403 and 12906 need no additional work imo.

Refactored into interface::PrivateSend::Options and interface::PrivateSend::Client

bf36a3c gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky) Pull request description: Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling. This bug could cause balances to appear out of date, and was first introduced bitcoin@a0704a8#diff-2e3836af182cfb375329c3463ffd91f8L117. Before that commit, there wasn't a problem because cs_main was held during the poll update. Currently, the problem should be rare. But if 8937d99 from bitcoin#17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification. MarcoFalke also points out that a0704a8 could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI. Thanks to John Newbery for finding this bug this while reviewing bitcoin#17954. ACKs for top commit: Empact: utACK bitcoin@bf36a3c jonasschnelli: utACK bf36a3c Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4

…windows gitian build 17780d6 scripted-diff: Avoid `interface` keyword to fix windows gitian build (Russell Yanofsky) Pull request description: Rename `interface` to `interfaces` Build failure reported by ken2812221 in bitcoin#10244 (comment) Tree-SHA512: e02c97c728540f344202c13b036f9f63af23bd25e25ed7a5cfe9e2c2f201a12ff232cc94a93fbe37ef6fb6bf9e036fe62210ba798ecd30de191d09338754a8d0 -BEGIN VERIFY SCRIPT- git mv src/interface src/interfaces ren() { git grep -l "$1" | xargs sed -i "s,$1,$2,g"; } ren interface/ interfaces/ ren interface:: interfaces:: ren BITCOIN_INTERFACE_ BITCOIN_INTERFACES_ ren "namespace interface" "namespace interfaces" -END VERIFY SCRIPT-

- Call `statusUpdateNeeded` before `tryGetTxStatus`. This allows to run the latter only if really required i.e. if `statusUpdateNeeded` returns true. - Reuse `cachedNumBlocks` from `WalletModel` in `TransactionTablePriv::index()` to _actually_ avoid redundant tx status updates - Initialize `cachedNumBlocks` and `cachedChainLockHeight` with `-1` to avoid extra `pollBalanceChanged` call

This is required to fix a (potential) deadlock

xdustinface · 2020-10-14T12:07:28Z

I don't think squashing multiple focused 10244 commits into one huge commit is a good idea would rather prefer them as is.

Agreed, right now i honestly don't see any benefit by doing this @PastaPastaPasta

Squashing performance fixes after 18123 (b1bc1f1 + b46e65b + 34a146c) makes sense though.

Done in 5020a68

Squashing aa2ebe1 + 60a8584 might also be a good idea.

Done

UdjinM6

utACK

PastaPastaPasta

ACK

The issue was introduced in dashpay#3682

* qt: Fix labels in transaction list The issue was introduced in #3682 * qt: Always use labels from TransactionStatus for transaction list Missed this in #3155 * Update src/qt/transactiontablemodel.cpp Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

* qt: Fix labels in transaction list The issue was introduced in dashpay#3682 * qt: Always use labels from TransactionStatus for transaction list Missed this in dashpay#3155 * Update src/qt/transactiontablemodel.cpp Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

, bitcoin#20591, bitcoin#23596, bitcoin#22868, bitcoin#24225 (wallet backports: part 3) 6958a5b merge bitcoin#24225: Add sanity checks to DiscourageFeeSniping (Kittywhiskers Van Gogh) 63dc779 merge bitcoin#22868: Call load handlers without cs_wallet locked (Kittywhiskers Van Gogh) 698e01e merge bitcoin#23596: fix `wallet_transactiontime_rescan.py --descriptors` and add to test runner (Kittywhiskers Van Gogh) 72322ce merge bitcoin#20591: fix ComputeTimeSmart function during rescanning process (Kittywhiskers Van Gogh) 99c9880 merge bitcoin#22941: inline functions `{Read,Write}OrderPos` (Kittywhiskers Van Gogh) e2ae504 merge bitcoin#25148: Remove `NO_THREAD_SAFETY_ANALYSIS` from non-test/benchmarking code (Kittywhiskers Van Gogh) f637516 merge bitcoin#22100: Clean up new wallet spend, receive files added bitcoin#21207 (Kittywhiskers Van Gogh) 9a5dc62 partial bitcoin#22100: Clean up new wallet spend, receive files added bitcoin#21207 (Kittywhiskers Van Gogh) 258cdc2 refactor: move and rename `GetBudgetSystemCollateralTX()` to `spend.cpp` (Kittywhiskers Van Gogh) e03fdff refactor: move ISLock and ChainLock wtx check to CWallet (Kittywhiskers Van Gogh) 460c187 refactor: move some CoinJoin-specific logic out of CWalletTx and CWallet (Kittywhiskers Van Gogh) ed12dab move-only: section CoinJoin-specific code to separate source file (Kittywhiskers Van Gogh) bc0a1b7 refactor: remove unused `CWalletTx::GetWallet()` (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * `CWalletTx::GetWallet()` was introduced in [dash#3155](#3155) ([commit](3b44576)) and its use was curtailed in [dash#3682](#3682) ([commit](155324e)) but it has lingered around. As [bitcoin#22100](bitcoin#22100) will get rid of `CWalletTx::pwallet` and it is unused, it is removed. * As [bitcoin#22100](bitcoin#22100) has a substantial diff, it has been split into two commits for ease of review, the former centers around `wallet/receive.cpp` and the latter around `wallet/spend.cpp`. The former has additional changes that are supplanted/removed in the latter to ensure they can be independently validated. To compare the diff against upstream, squashing the two halves may be desirable. * As the goal of the backport to remove the circular dependencies between `wallet.cpp` and both `spend.cpp` and `receive.cpp`, Dash-specific code that contributes to this dependency had to be moved. To this effect, CoinJoin-specific code has been moved to its own source file as it relies on code in both `spend.cpp` and `receive.cpp`. `GetBudgetSystemCollateralTX()` was moved to `spend.cpp` as it used `CreateTransaction()` and given a better representative name. * ~~`CWalletTx::Is{ChainLocked, LockedByInstantSend}()` was moved to `CWallet` and renamed to match the convention of `CWallet::GetTxDepthInMainChain()` in preparation for [bitcoin#22100](bitcoin#22100). A drawback of the new implementation is that both calls (but especially `IsTxLockedByInstantSend()`) is more expensive as per-transaction result caching is no longer available.~~ Resolved by d29b5ee, thanks Udjin! * Portions of [bitcoin#25148](bitcoin#25148) were included in previous commits to ensure that `-Wthread-safety` is satisfied (e.g. annotations for `MakeWalletTxStatus()` and `WalletTxToJSON()` were needed when `IsTxLockedByInstantSend()` and `IsTxChainLocked()` were introduced) ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 6958a5b Tree-SHA512: 4b9702c5976643d63874f245b4543c068523c0c87cc8298f8fbf0826426088397167dc775fd34aa27ce7d8decc8bb592e3ae11c6342193d757d285fad1f8d010

xdustinface force-pushed the backport-10244-18123 branch 3 times, most recently from d875e09 to 564302c Compare August 31, 2020 08:45

xdustinface marked this pull request as draft September 1, 2020 17:34

PastaPastaPasta mentioned this pull request Sep 27, 2020

[WIP] Backport 10244 #3638

Closed

24 tasks

xdustinface force-pushed the backport-10244-18123 branch 2 times, most recently from 7143863 to 63840e1 Compare September 29, 2020 23:20

xdustinface marked this pull request as ready for review September 29, 2020 23:24

xdustinface changed the title ~~backport: bitcoin#10244 and bitcoin#18123~~ backport: bitcoin#10244, bitcoin#18123 and bitcoin#12906 Oct 1, 2020

xdustinface force-pushed the backport-10244-18123 branch from 63840e1 to 3a8a0e1 Compare October 1, 2020 13:34

xdustinface force-pushed the backport-10244-18123 branch from 3a8a0e1 to bf15977 Compare October 1, 2020 14:09

UdjinM6 requested changes Oct 1, 2020

View reviewed changes

xdustinface force-pushed the backport-10244-18123 branch 2 times, most recently from ddc841a to 05720a6 Compare October 2, 2020 01:05

PastaPastaPasta reviewed Oct 2, 2020

View reviewed changes

xdustinface force-pushed the backport-10244-18123 branch 3 times, most recently from 2399dc2 to e9d108b Compare October 2, 2020 14:32

xdustinface changed the title ~~backport: bitcoin#10244, bitcoin#18123 and bitcoin#12906~~ backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403 Oct 2, 2020

UdjinM6 requested changes Oct 3, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

UdjinM6 reviewed Oct 3, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

UdjinM6 reviewed Oct 3, 2020

View reviewed changes

xdustinface force-pushed the backport-10244-18123 branch from ac3a8a0 to 0fb1a05 Compare October 3, 2020 17:19

UdjinM6 reviewed Oct 3, 2020

View reviewed changes

src/interfaces/wallet.cpp Show resolved Hide resolved

UdjinM6 previously approved these changes Oct 9, 2020

View reviewed changes

PastaPastaPasta mentioned this pull request Oct 11, 2020

[qt] Abnormal non-deterministic scaling of Settings text #3767

Closed

UdjinM6 mentioned this pull request Oct 12, 2020

Backports 0.17 pr19 #3741

Merged

UdjinM6 requested a review from PastaPastaPasta October 12, 2020 08:35

xdustinface and others added 10 commits October 14, 2020 12:09

interface/qt: Remove direct src/privatesend calls

1a5a1ca

Refactored into interface::PrivateSend::Options and interface::PrivateSend::Client

test: Add boost/signals2/connection.hpp to lint-inlcudes.sh

6088650

test: Fix wallet tests

79d32b5

Partially backport bitcoin#11403

7ab1051

Add mempool lock for Create/CommitTransaction in wallet interface

12c840f

This is required to fix a (potential) deadlock

wallet: Add CL/IS caches in CMerkleTx

4dca83b

wallet: Respect chainlocks in CMerkleTx::IsLockedByInstantSend

7cf7b53

xdustinface dismissed UdjinM6’s stale review via 7cf7b53 October 14, 2020 11:57

xdustinface force-pushed the backport-10244-18123 branch from 34a146c to 7cf7b53 Compare October 14, 2020 11:57

UdjinM6 approved these changes Oct 14, 2020

View reviewed changes

PastaPastaPasta approved these changes Oct 14, 2020

View reviewed changes

UdjinM6 merged commit 74f4d2a into dashpay:develop Oct 14, 2020

UdjinM6 mentioned this pull request Oct 14, 2020

qt: Fix --disable-wallet build and --disablewallet mode #3762

Merged

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Feb 7, 2021

qt: Fix labels in transaction list

a7f5420

The issue was introduced in dashpay#3682

knst mentioned this pull request Mar 14, 2023

backport: merge bitcoin#21523, #17954, #17905, #17993, #19132, #20494, #19425, #17513, bitcoin-gui#8, partial bitcoin#17938, #18587 (auxiliary backports: part 9) #5246

Merged

kwvg mentioned this pull request Apr 19, 2025

backport: merge bitcoin#22100, #25148, #22941, #20591, #23596, #22868, #24225 (wallet backports: part 3) #6633

Merged

5 tasks

		@@ -241,9 +241,7 @@ void AddressBookPage::on_showAddressQRCode_clicked()

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403 #3682

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403 #3682

Uh oh!

Conversation

xdustinface commented Aug 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PastaPastaPasta commented Sep 20, 2020

Uh oh!

xdustinface commented Sep 20, 2020

Uh oh!

xdustinface commented Sep 29, 2020

Uh oh!

UdjinM6 commented Oct 1, 2020

Uh oh!

xdustinface commented Oct 1, 2020

Uh oh!

UdjinM6 commented Oct 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xdustinface Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xdustinface commented Oct 9, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Oct 14, 2020

Uh oh!

UdjinM6 commented Oct 14, 2020

Uh oh!

xdustinface commented Oct 14, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface commented Aug 30, 2020 •

edited

Loading

xdustinface Oct 2, 2020 •

edited

Loading