Don't call lsn_reset in periodic flush

bvbfan commented at 6:48 am on May 7, 2020: contributor

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

fanquake added the label Wallet on May 7, 2020

bvbfan commented at 6:51 am on May 7, 2020: contributor

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.

laanwj commented at 5:35 pm on May 7, 2020: member

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.

bvbfan commented at 5:08 am on May 8, 2020: contributor

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.

achow101 commented at 6:16 am on May 8, 2020: member

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.

bvbfan commented at 9:34 am on May 8, 2020: contributor

@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.

achow101 commented at 4:20 pm on May 8, 2020: member

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

bvbfan commented at 8:44 am on May 9, 2020: contributor

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

DrahtBot commented at 3:50 am on May 14, 2020: contributor

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

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.

DrahtBot added the label Needs rebase on Jun 17, 2020

DrahtBot removed the label Needs rebase on Jun 17, 2020

DrahtBot added the label Needs rebase on Jul 9, 2020

DrahtBot removed the label Needs rebase on Jul 9, 2020

bvbfan commented at 4:40 pm on July 9, 2020: contributor

@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.

ryanofsky commented at 8:40 pm on July 9, 2020: contributor

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.

bvbfan commented at 4:35 am on July 10, 2020: contributor

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

DrahtBot added the label Needs rebase on Jul 23, 2020

DrahtBot removed the label Needs rebase on Jul 23, 2020

DrahtBot added the label Needs rebase on Jul 29, 2020

DrahtBot removed the label Needs rebase on Jul 29, 2020

achow101 commented at 9:25 pm on August 13, 2020: member

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.

bvbfan commented at 5:38 am on August 25, 2020: contributor

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

DrahtBot added the label Needs rebase on Sep 7, 2020

DrahtBot removed the label Needs rebase on Sep 8, 2020

DrahtBot added the label Needs rebase on Oct 19, 2020

DrahtBot removed the label Needs rebase on Oct 20, 2020

DrahtBot added the label Needs rebase on Dec 2, 2020

maflcko commented at 12:03 pm on October 22, 2021: member

Needs rebase if still relevant

bvbfan commented at 12:16 pm on October 22, 2021: contributor

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.

maflcko commented at 12:25 pm on October 22, 2021: member

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

bvbfan commented at 12:39 pm on October 28, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Oct 28, 2021

DrahtBot added the label Needs rebase on May 3, 2022

DrahtBot removed the label Needs rebase on May 3, 2022

bvbfan commented at 2:24 pm on June 17, 2022: contributor

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.

DrahtBot added the label Needs rebase on Sep 16, 2022

Don't call lsn_reset in periodic flush

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

3011d6c18a

DrahtBot removed the label Needs rebase on Sep 16, 2022

achow101 commented at 8:30 pm on October 12, 2022: member

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.

achow101 closed this on Oct 12, 2022

bitcoin locked this on Oct 12, 2023

Don’t call lsn_reset in periodic flush #18904

Conflicts