utACK

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?

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.

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

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

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.

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

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.

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

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)?

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)

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.

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.

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

utACK d63ff62