Don't call lsn_reset in periodic flush #18904

Signed-off-by: Anthony Fieroni bvbfan@abv.bg

lsn_reset on big wallet (actually wallet can became bigger) takes a minutes on GB file. It shuffles the priv key location but it does not make sense to be so frequently.

Shuffling private key location is not the point here. Does it still concatenate the bdb logs after this, so that everything becomes one file? That's the point of the periodic flush.

Does it still concatenate the bdb logs after this, so that everything becomes one file?

https://docs.oracle.com/cd/E17276_01/html/api_reference/C/txncheckpoint.html

If there has been database environment activity since the last checkpoint, the DB_ENV->txn_checkpoint method flushes the underlying memory pool, writes a checkpoint record to the log, and then flushes the log.

https://docs.oracle.com/cd/E17275_01/html/api_reference/C/envlsn_reset.html

The DB_ENV->lsn_reset() method allows database files to be moved from one transactional database environment to another.

There is no need to be called on every flush.

I don't think this is safe without #18907. Otherwise there could be a condition where the log files are removed and the database opened again which could cause corruption. As I understand it, lsn_reset is required if we're going to do any log file removals.

@achow101 you should be more verbose on that, i don't see anything related. On periodic flush there is no db moving or changing, lsn_reset still present on backup and app close.

During BerkeleyEnvironment::Open, if dbenv->open fails, it will move the transaction logs to a backup directory and retry dbenv->open. If the LSNs in the wallet file were not reset when this occurs, then there could be corruption issues due to mismatched LSNs. This situation could occur if Core has an unclean exit and if there is something that causes the environment to fail to open (e.g. a different BDB version).

When batch is using db is open without retry or i miss something https://github.com/bitcoin/bitcoin/blob/master/src/wallet/db.cpp#L533

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
• #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
• #10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@achow101 i do some code refactor as well. In periodic flush it isn't needed lsn_reset to be called, to close the db as well. It just needs to trigger writing of log checkpoints. Looking at docs https://docs.oracle.com/cd/E17276_01/html/api_reference/C/txncheckpoint.html https://docs.oracle.com/cd/E17275_01/html/api_reference/C/envlsn_reset.html lsn_reset is needed when we want to move db from one environment to another, it rewrites entire file, which is crucial slow when wallet file is large.

In periodic flush it isn't needed lsn_reset to be called, to close the db as well. It just needs to trigger writing of log checkpoints.

If the point is to avoid data loss it makes sense to only require writing a new log checkpoint. If the point is to allow deleting the logs entirely (#2558)

https://github.com/bitcoin/bitcoin/blob/2aaff4813cc340764c99846513d58fc3553fcb6a/src/wallet/bdb.cpp#L609

maybe it does make sense to call lsn_reset.

I'm a little confused both about the behavior and the design goals. Maybe there should be an option to not delete log files.

If the point is to avoid data loss it makes sense to only require writing a new log checkpoint. If the point is to allow deleting the logs entirely (#2558)

https://github.com/bitcoin/bitcoin/blob/2aaff4813cc340764c99846513d58fc3553fcb6a/src/wallet/bdb.cpp#L609

maybe it does make sense to call lsn_reset.

Sure, here it's needed, as well in backup and rewrite. The problem in periodic flush is rewriting entire file (say it can be couple of GiB) for nothing, it blocks the UI, daemon as well. #18886

I'm still not convinced that this is safe to do. I think it would be better to ensure that after each write, the database is portable. That would remove the need for PeriodicFlush in general which would resolve this issue. But at the same time, doing that is probably not performant.

At this point, it may be better to just leave this as is and merge sqlite wallets which don't have this issue. Then provide an upgrade path from bdb to sqlite (i.e. a straight record to record conversion) so that people who are experiencing these performance issues can just upgrade to sqlite.

Hi, @achow101 i don't think it's needed to remove PeriodicFlush at all, because it prevents data loss by writing txs to backup db.

https://www.mit.edu/afs.new/sipb/project/subversion/docs/ref/transapp/checkpoint.html

Needs rebase if still relevant

Hi @MarcoFalke, it speed-up periodic flush since lsn_reset rewrites entire file, high unlikely to periodic action. I've using across other forks without issue, but i've not follow will bdb backend entire replaced via sqlite one or they will keep operate in cooperation? If bdb is still well used backend, so the changes is good to be pushed. Thanks.

See https://github.com/bitcoin/bitcoin/issues/20160

Rebased

I think it would be better to ensure that after each write, the database is portable.

That's look very aggressive. Bigger wallets will suffer even more, that's a ton of GiB rewritten. LSN changes the key position thus entire file is regenerated.

Given that future efforts in the wallet are aimed at descriptor wallets, and legacy wallets and BDB are slated for removal in the future, we're no longer investing time into non-critical issues (i.e. money losing) for the legacy wallet and BDB.