Make nWalletDBUpdated atomic to avoid a potential race.

pstratem commented at 3:41 am on November 27, 2016: contributor

Title

luke-jr commented at 3:42 am on November 27, 2016: member

utACK

TheBlueMatt commented at 3:51 am on November 27, 2016: member

If you’re gonna touch this, might as well fix it - will it interact negatively with multiwallet support as written? Also, should be easy to make it only exist in walletdb.cpp, I’d think?

fanquake added the label Wallet on Nov 27, 2016

laanwj commented at 7:11 am on November 28, 2016: member

will it interact negatively with multiwallet support as written?

Right - this should probably exist on the CWalletDB object instead of globally, otherwise multiple wallets will react on each other’s changes. (and then you may not even need an atomic because they will be holding their respective wallet lock) This is not entirely trivial to fix because the ‘wallet flush’ thread (which IIRC uses this) is decidedly single-wallet. Maybe one of the multiwallet pulls addresses this though.

jonasschnelli commented at 7:40 am on November 28, 2016: contributor

Agree with @laanwj. Also, #8776 would be a step towards a better – multiwallet safe – wallet flush.

luke-jr commented at 9:13 am on November 28, 2016: member

multiwallet makes the flush thread go over all the wallets and flush them all when this is updated. AFAIK extraneous flushing is mostly harmless…?

laanwj commented at 9:25 am on November 28, 2016: member

multiwallet makes the flush thread go over all the wallets and flush them all when this is updated. AFAIK extraneous flushing is mostly harmless…?

It’s would be a performance sink. Remember the thread is named badly. Our ‘flushing’ isn’t really flushing, it is a full fledged database consolidation.

Edit: so the idea of having a single flush thread visit all wallets is good, but the write tracking should be per-wallet and not global.

TheBlueMatt commented at 7:06 pm on November 28, 2016: member

This should probably also move to CScheduler to avoid having Yet Another Thread when we dont need it.

gmaxwell commented at 8:05 pm on November 29, 2016: contributor

The db flush here doesn’t really flush a wallet at all, it flushes the whole database environment, so I don’t believe it can be made per-wallet.

pstratem commented at 1:55 am on November 30, 2016: contributor

Indeed this cannot be made per wallet, this is flushing the entire database environment.

jonasschnelli commented at 8:00 am on November 30, 2016: contributor

Right now we are flushing the DB environment (https://docs.oracle.com/cd/E17276_01/html/api_reference/C/envclose.html) with DBEnv->CloseDb(filename). Is there a reason why we wouldn’t want to use DB->Sync() (https://docs.oracle.com/cd/E17276_01/html/api_reference/C/dbsync.html)?

laanwj commented at 8:36 am on November 30, 2016: member

Is there a reason why we wouldn’t want to use DB->Sync() (https://docs.oracle.com/cd/E17276_01/html/api_reference/C/dbsync.html)?

The wallet code is set up to flush after every (important) write immediately, that’s not what the thread is for.

What the flush thread does is not really flushing. The method you propose would be a less hassle solution to do flushing, but what the thread wants to do is consolidate the database files: When writing, BerkeleyDB writes to log files and merges these into the main database only incidentally. The whole circuitous exercise in the thread is to force this to happen, so that the wallet is one file.

(can we please rename this thread and add a comment somewhere, everyone reading this code is confused about this, and this has been the case for years)

laanwj commented at 8:37 am on November 30, 2016: member

The db flush here doesn’t really flush a wallet at all, it flushes the whole database environment, so I don’t believe it can be made per-wallet.

Doesn’t every wallet get its own database environment? I guess not.

gmaxwell commented at 9:04 am on November 30, 2016: contributor

No– well every database environment needs it’s own database directory, cache overheads, etc. So I’m guessing that wouldn’t be a great move.

pstratem force-pushed on Dec 1, 2016

Make nWalletDBUpdated atomic to avoid a potential race. d63ff6265b

in src/wallet/walletdb.h: in f820ac2ae6 outdated

10@@ -11,6 +11,7 @@
11 #include "wallet/db.h"
12 #include "key.h"
13 
14+#include <atomic>

TheBlueMatt commented at 2:39 am on December 1, 2016:

nit: move this to the .cpp.

pstratem commented at 9:50 pm on December 21, 2016:

nit picked

TheBlueMatt commented at 3:39 pm on February 3, 2017:

Not it wasnt?

pstratem force-pushed on Feb 3, 2017

TheBlueMatt commented at 9:54 pm on February 3, 2017: member

Can this get an 0.14 tag, since its a simple bugfix?

TheBlueMatt commented at 9:57 pm on February 3, 2017: member

utACK d63ff6265b0c6ae30efcbb9120d4db419606198a

sipa commented at 0:44 am on February 4, 2017: member

utACK d63ff6265b0c6ae30efcbb9120d4db419606198a

sipa added this to the milestone 0.14.0 on Feb 4, 2017

laanwj merged this on Feb 6, 2017

laanwj closed this on Feb 6, 2017

laanwj referenced this in commit 02464da5e4 on Feb 6, 2017

codablock referenced this in commit 8e43aa3ab5 on Jan 19, 2018

codablock referenced this in commit eb4c5bac6d on Jan 23, 2018

andvgal referenced this in commit 9c381b2503 on Jan 6, 2019

CryptoCentric referenced this in commit 56a6922177 on Feb 27, 2019

random-zebra referenced this in commit 7db7724cff on Aug 23, 2020

LarryRuane referenced this in commit b1aeb0595c on Feb 24, 2021

LarryRuane referenced this in commit 0658e2d0d3 on Apr 1, 2021

zkbot referenced this in commit 1b5f17c900 on Apr 1, 2021

zkbot referenced this in commit 80e66e7daa on Apr 2, 2021

MarcoFalke locked this on Sep 8, 2021

Make nWalletDBUpdated atomic to avoid a potential race. #9227