[Wallet] Remove CWalletDB* parameter from CWallet::AddToWallet

pstratem commented at 5:48 pm on June 6, 2016: contributor

This is the last method to pass a CWalletDB

in src/wallet/wallet.cpp: in 27eb7fd7b0 outdated

686@@ -687,8 +687,10 @@ void CWallet::MarkDirty()
687     }
688 }
689 
690-bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletDB* pwalletdb)
691+bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet)
692 {
693+    CWalletDB walletdb(strWalletFile);

jonasschnelli commented at 6:04 pm on June 6, 2016:

just curious: what is the reason to not keep a instance variable (within CWallet) that holds walletdb object? What are the benefits of constantly opening/instantiating the CWalletDB?

laanwj commented at 6:50 am on June 7, 2016:

The scope is used to manage the lifetime of the (database) transaction.

So having a single CWalletDB object would change semantics significantly. Some people have had this idea before but it was always rejected.

pstratem commented at 1:57 am on June 8, 2016:

@jonasschnelli I will eventually get to that point, but it’s going to take a significant amount of work to get there.

in src/wallet/walletdb.cpp: in 27eb7fd7b0 outdated

399@@ -400,7 +400,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
400             if (wtx.nOrderPos == -1)
401                 wss.fAnyUnordered = true;
402 
403-            pwallet->AddToWallet(wtx, true, NULL);
404+            pwallet->AddToWallet(wtx, true);

jonasschnelli commented at 6:05 pm on June 6, 2016:

If you have 10'000 wallet transaction, this would now result in 10'000 x CWalletDB walletdb(strWalletFile);. What about the performance impacts?

jonasschnelli added the label Wallet on Jun 6, 2016

sipa commented at 7:00 am on June 7, 2016: member

I think the persistent CWalletDb object here is just there to prevent a flush to disk in between every added transaction, for performance reasons. Based on earlier pull requests by @pstratem around this, I think he intends to re-introduce a single CWalletDb* inside CWallet rather than reopening one all the time, after cleaning things up. Perhaps that will need to be done in the same PR, though?

dcousens commented at 7:33 am on June 7, 2016: contributor

Perhaps that will need to be done in the same PR, though?

I think provided it is done before a release, there is no issue.

MarcoFalke commented at 7:40 am on June 7, 2016: member

I think there is no advantage merging this small refactoring early. Might as well just add the second commit to address @jonasschnelli s and @sipa s feedback.

jonasschnelli commented at 9:20 am on June 7, 2016: contributor

Regardless of the persistent CWalletDB * I think this PR has value (refactoring). But I guess we should not change the ReadKeyValue/AddToWallet in that way that it instantiate a new CWalletDB on each wallet transaction load from the wallet.dat (which this PR currently does).

laanwj commented at 1:32 pm on June 7, 2016: member

A refactoring shouldn’t result in behavioral changes. I’d rather stick with ugly code somewhat longer than introduce a temporary (but serious) performance regression.

dcousens commented at 2:01 pm on June 7, 2016: contributor

introduce a temporary (but serious) performance regression.

At least then you’ll have users reminding you :laughing:

pstratem commented at 2:08 am on June 8, 2016: contributor

I’ve split up AddToWallet into AddToWallet and LoadToWallet around the fFromLoadWallet flag.

This addresses the performance @jonasschnelli noticed and improves the API.

pstratem force-pushed on Jun 8, 2016

pstratem commented at 4:42 am on June 8, 2016: contributor

I swapped the order of the commits such that there’s no performance issue at any point.

22fa63e74150139c7c71108ee46ed87d31a4a2f3c171d16c34530f76ecc79f82

pstratem force-pushed on Jun 8, 2016

pstratem commented at 11:32 pm on June 8, 2016: contributor

import hashlib;hashlib.sha256(“there is still a performance issue”).hexdigest()

there isn’t actually

pstratem force-pushed on Jun 8, 2016

pstratem force-pushed on Jun 16, 2016

in src/wallet/wallet.cpp: in 44d5267bfc outdated

2434@@ -2435,9 +2435,6 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey)
2435                 coin.BindWallet(this);
2436                 NotifyTransactionChanged(this, coin.GetHash(), CT_UPDATED);
2437             }
2438-
2439-            if (fFileBacked)

jonasschnelli commented at 7:04 am on June 16, 2016:

IMO the non-file-backend state is only used in UnitTests, right? We should also consider dropping it completely (in a later PR).

pstratem commented at 7:42 am on June 16, 2016:

I’ll be able to easily remove it once more of the logic is encapsulated.

in src/wallet/rpcdump.cpp: in 44d5267bfc outdated

301@@ -302,8 +302,7 @@ UniValue importprunedfunds(const UniValue& params, bool fHelp)
302     LOCK2(cs_main, pwalletMain->cs_wallet);
303 
304     if (pwalletMain->IsMine(tx)) {
305-        CWalletDB walletdb(pwalletMain->strWalletFile, "r+", false);

jonasschnelli commented at 7:08 am on June 16, 2016:

The third argument fFlushOnClose (false here) will be true after this PR (default value is true). = before this PR importprunedfunds will not flush on close. After this PR it will. But I guess this is not a problem.

pstratem commented at 7:39 am on June 16, 2016:

Indeed this does change the default. There doesn’t seem to be a good reason not to flush here.

jonasschnelli commented at 7:08 am on June 16, 2016: contributor

utACK 44d5267bfcd8abae5f0ae2a0f4052fc5a88acd2e

MarcoFalke commented at 10:28 am on June 17, 2016: member

utACK 44d5267

MarcoFalke added the label Refactoring on Jun 17, 2016

pstratem force-pushed on Jul 19, 2016

pstratem renamed this:
~~Remove CWalletDB* parameter from CWallet::AddToWallet~~
[Wallet] Remove CWalletDB* parameter from CWallet::AddToWallet
on Jul 20, 2016

instagibbs commented at 6:56 pm on July 21, 2016: member

tACK 6246add89a46523526a57b66bb87daf9413c69d7

MarcoFalke commented at 7:09 pm on July 21, 2016: member

Seems 6246add is just a plain rebase of 44d5267

re-utACK 6246add

pstratem commented at 9:38 pm on July 21, 2016: contributor

@MarcoFalke indeed it is

jonasschnelli commented at 9:43 am on July 22, 2016: contributor

Looks good now! Extensive Code Review ACK (6246add89a46523526a57b66bb87daf9413c69d7), nit: maybe squash commit.

pstratem commented at 3:22 am on July 23, 2016: contributor

@jonasschnelli I’d rather not squash, each commit is logically independent.

in src/wallet/wallet.cpp: in 6246add89a outdated

908@@ -904,7 +909,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl
909             // this is safe, as in case of a crash, we rescan the necessary blocks on startup through our SetBestChain-mechanism
910             CWalletDB walletdb(strWalletFile, "r+", false);

laanwj commented at 9:11 am on July 27, 2016:

This can go

Edit: Note that the parameters under which walletdb are opened change here - it used to be specifically overriding the flush to false, but after this change, the walletdb will be opened inside AddToWallet with default parameters. What is the impact of this?

pstratem commented at 0:50 am on July 28, 2016:

Indeed this can go. I’ll run a benchmark of master and this pr doing a rescan with a wallet that has an enormous number of transactions and keys.

sipa commented at 8:05 pm on July 29, 2016: member

utACK c04225e7fdb9651ae2f97ed0e3f9601205ea858b

Split CWallet::AddToWallet into AddToWallet and LoadToWallet.

This removes the fFromLoadWallet flag in AddToWallet.  These were already
effectively two methods.

00f09c920c

Remove CWalletDB* parameter from CWallet::AddToWallet 867f842f1e

Remove unused pwalletdb from CWallet::AddToWallet 5723bb44ce

pstratem force-pushed on Jul 30, 2016

pstratem commented at 0:18 am on July 30, 2016: contributor

@laanwj I was having trouble getting consistent benchmark results from -rescan (+-30 seconds on runs taking 160-200 seconds).

So I just added an fFlushOnClose parameter so it’s now clearly doing the same thing as before the change.

sipa commented at 0:39 am on July 30, 2016: member

reutACK 5723bb44ce2c6bb14114aa7f211160702a47ac91

pstratem commented at 6:48 pm on July 31, 2016: contributor

Indeed I generally agree, but I was already squashing adding the fFlushOnClose flag.

Whenever possible I strongly prefer that each commit produces a complete working build.

On Jul 31, 2016 3:56 AM, “MarcoFalke” notifications@github.com wrote:

indeed it is

I prefer no rebase when it is not necessary. Reviewers always have to reconstruct the history locally each time a rebase is done.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #8152 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAl4Q5FO3twJijzI_pOno3iNIKzaHvLfks5qbH8-gaJpZM4IvJOB .

MarcoFalke commented at 7:07 pm on July 31, 2016: member

With rebase I meant changing the commit which the pull is based on, not squashing.

reACK 5723bb44ce2c6bb14114aa7f211160702a47ac91

sipa merged this on Aug 1, 2016

sipa closed this on Aug 1, 2016

sipa referenced this in commit b9c1cd8184 on Aug 1, 2016

codablock referenced this in commit 8a425682f4 on Sep 19, 2017

codablock referenced this in commit e2a7ba1c2a on Dec 29, 2017

codablock referenced this in commit 9589e24485 on Jan 8, 2018

andvgal referenced this in commit 4f90f1eb85 on Jan 6, 2019

DrahtBot locked this on Sep 8, 2021

[Wallet] Remove CWalletDB* parameter from CWallet::AddToWallet #8152