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

pstratem · 2016-06-06T17:48:25Z

This is the last method to pass a CWalletDB

jonasschnelli · 2016-06-06T18:04:02Z

src/wallet/wallet.cpp

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?

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.

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

sipa · 2016-06-07T07:00:47Z

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 · 2016-06-07T07:33:12Z

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.

maflcko · 2016-06-07T07:40:01Z

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 · 2016-06-07T09:20:07Z

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 · 2016-06-07T13:32:34Z

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 · 2016-06-07T14:01:21Z

introduce a temporary (but serious) performance regression.

At least then you'll have users reminding you 😆

pstratem · 2016-06-08T02:08:00Z

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

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

pstratem · 2016-06-08T04:42:20Z

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

22fa63e74150139c7c71108ee46ed87d31a4a2f3c171d16c34530f76ecc79f82

pstratem · 2016-06-08T23:32:02Z

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

there isn't actually

jonasschnelli · 2016-06-16T07:04:03Z

src/wallet/wallet.cpp

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

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

jonasschnelli · 2016-06-16T07:08:54Z

utACK 44d5267bfcd8abae5f0ae2a0f4052fc5a88acd2e

maflcko · 2016-06-17T10:28:55Z

utACK 44d5267

instagibbs · 2016-07-21T18:56:09Z

tACK 6246add89a46523526a57b66bb87daf9413c69d7

maflcko · 2016-07-21T19:09:20Z

Seems 6246add is just a plain rebase of 44d5267

re-utACK 6246add

pstratem · 2016-07-21T21:38:49Z

@MarcoFalke indeed it is

jonasschnelli · 2016-07-22T09:43:27Z

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

pstratem · 2016-07-23T03:22:04Z

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

laanwj · 2016-07-27T09:11:43Z

src/wallet/wallet.cpp

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?

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 · 2016-07-29T20:05:32Z

utACK c04225e7fdb9651ae2f97ed0e3f9601205ea858b

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

pstratem · 2016-07-30T00:18:45Z

@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 · 2016-07-30T00:39:39Z

reutACK 5723bb4

pstratem · 2016-07-31T18:48:22Z

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" [email protected] 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
.

maflcko · 2016-07-31T19:07:29Z

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

reACK 5723bb4

…Wallet 5723bb4 Remove unused pwalletdb from CWallet::AddToWallet (Patrick Strateman) 867f842 Remove CWalletDB* parameter from CWallet::AddToWallet (Patrick Strateman) 00f09c9 Split CWallet::AddToWallet into AddToWallet and LoadToWallet. (Patrick Strateman)

…::AddToWallet 5723bb4 Remove unused pwalletdb from CWallet::AddToWallet (Patrick Strateman) 867f842 Remove CWalletDB* parameter from CWallet::AddToWallet (Patrick Strateman) 00f09c9 Split CWallet::AddToWallet into AddToWallet and LoadToWallet. (Patrick Strateman)

jonasschnelli reviewed Jun 6, 2016
View reviewed changes

jonasschnelli added the Wallet label Jun 6, 2016

pstratem force-pushed the 2016-06-06-cwallet-addtowallet-remove-cwalletdb branch from b456663 to 175dcdc Compare June 8, 2016 04:41

pstratem force-pushed the 2016-06-06-cwallet-addtowallet-remove-cwalletdb branch from 175dcdc to 2a20b89 Compare June 8, 2016 09:53

pstratem force-pushed the 2016-06-06-cwallet-addtowallet-remove-cwalletdb branch from 2a20b89 to 072571c Compare June 8, 2016 23:34

pstratem force-pushed the 2016-06-06-cwallet-addtowallet-remove-cwalletdb branch from 072571c to 44d5267 Compare June 16, 2016 01:17

jonasschnelli reviewed Jun 16, 2016
View reviewed changes

maflcko added the Refactoring label Jun 17, 2016

pstratem mentioned this pull request Jul 14, 2016

send BTC is very slow with huge wallet #7475

Closed

pstratem force-pushed the 2016-06-06-cwallet-addtowallet-remove-cwalletdb branch from 44d5267 to 6246add Compare July 19, 2016 21:35

pstratem changed the title ~~Remove CWalletDB* parameter from CWallet::AddToWallet~~ [Wallet] Remove CWalletDB* parameter from CWallet::AddToWallet Jul 20, 2016

pstratem mentioned this pull request Jul 20, 2016

[Wallet][Trivial] Fix exception message to reference actual thrower. #8376

Closed

laanwj reviewed Jul 27, 2016
View reviewed changes

pstratem added 3 commits July 29, 2016 17:04

Split CWallet::AddToWallet into AddToWallet and LoadToWallet.

00f09c9

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

Remove CWalletDB* parameter from CWallet::AddToWallet

867f842

Remove unused pwalletdb from CWallet::AddToWallet

5723bb4

pstratem force-pushed the 2016-06-06-cwallet-addtowallet-remove-cwalletdb branch from 3c6a33e to 5723bb4 Compare July 30, 2016 00:05

sipa merged commit 5723bb4 into bitcoin:master Aug 1, 2016

random-zebra mentioned this pull request Aug 2, 2020

[Validation] Reduce cs_main locks during ConnectTip/SyncWithWallets PIVX-Project/PIVX#1721

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

pstratem commented Jun 6, 2016

Uh oh!

jonasschnelli Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 7, 2016 • 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!

pstratem Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 7, 2016

Uh oh!

dcousens commented Jun 7, 2016

Uh oh!

maflcko commented Jun 7, 2016

Uh oh!

jonasschnelli commented Jun 7, 2016

Uh oh!

laanwj commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens commented Jun 7, 2016

Uh oh!

pstratem commented Jun 8, 2016

Uh oh!

pstratem commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstratem commented Jun 8, 2016

Uh oh!

jonasschnelli Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

pstratem Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 16, 2016

Uh oh!

maflcko commented Jun 17, 2016

Uh oh!

instagibbs commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 21, 2016

Uh oh!

pstratem commented Jul 21, 2016

Uh oh!

jonasschnelli commented Jul 22, 2016

Uh oh!

pstratem commented Jul 23, 2016

Uh oh!

laanwj Jul 27, 2016 • 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!

pstratem Jul 28, 2016

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 29, 2016

Uh oh!

pstratem commented Jul 30, 2016

Uh oh!

sipa commented Jul 30, 2016

Uh oh!

pstratem commented Jul 31, 2016

Uh oh!

maflcko commented Jul 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

laanwj Jun 7, 2016 •

edited

Loading

laanwj commented Jun 7, 2016 •

edited

Loading

pstratem commented Jun 8, 2016 •

edited

Loading

instagibbs commented Jul 21, 2016 •

edited

Loading

laanwj Jul 27, 2016 •

edited

Loading