wallet: bugfix, 'wallet_load_ckey' unit test fails with bdb #26644

1. 
   furszy commented at 9:44 PM on December 5, 2022: member

   Currently, the test wallet_load_verif_crypted_key_checksum fails if configured without sqlite.

   There are few reasons for it (chained bugs):

   1. As in the test we encrypt the wallet, and the encryption process requires a db rewrite + environment reload, we cannot use mock dbs (mock dbs are memory-only).

   2. Once the first issue gets solved, the next one is a deadlock inside DeleteRecords due a lack of sync between the opened cursor and each of the Erase calls (they happen on different db txn contexts so while we traverse the db, we aren't able to erase records).

      Note: I do have a pending research here. Because this should be affecting other places as well.. (ideally, read-only operations shouldn't lock the db)

   Extra Info: Have renamed the long and ugly wallet_load_verif_crypted_key_checksum test name to wallet_load_ckey.

2. 
   DrahtBot commented at 9:45 PM on December 5, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Stale ACK	achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26715 (Introduce MockableDatabase for wallet unit tests by achow101)

   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.

3. DrahtBot added the label Wallet on Dec 5, 2022
4. 
   achow101 commented at 6:32 PM on December 8, 2022: member

   ACK a5887086c21908de6e75265c537b9fffd50246ae

5. DrahtBot cross-referenced this on Dec 9, 2022 from issue wallet: Load database records in a particular order by achow101
6. DrahtBot cross-referenced this on Dec 12, 2022 from issue wallet: Refactor database cursor into its own object with proper return codes by achow101
7. achow101 cross-referenced this on Dec 16, 2022 from issue Introduce `MockableDatabase` for wallet unit tests by achow101
8. DrahtBot cross-referenced this on Jan 18, 2023 from issue refactor: wallet, remove global 'ArgsManager' dependency by furszy
9. DrahtBot added the label Needs rebase on Jan 23, 2023
10. furszy force-pushed on Jan 25, 2023
11. DrahtBot removed the label Needs rebase on Jan 25, 2023
12. DrahtBot added the label Needs rebase on Feb 17, 2023
13. furszy force-pushed on Feb 20, 2023
14. DrahtBot removed the label Needs rebase on Feb 20, 2023
15. DrahtBot cross-referenced this on Mar 8, 2023 from issue refactor: Remove CAddressBookData::destdata by ryanofsky
16. DrahtBot cross-referenced this on Mar 21, 2023 from issue wallet: Keep track of the wallet's own transaction outputs in memory by achow101
17. achow101 requested review from achow101 on Apr 25, 2023
18. DrahtBot added the label Needs rebase on May 1, 2023
19. wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor

    If the batch ptr is not passed, the cursor will not use the db active
    txn context which could lead to a deadlock if the code tries to modify
    the db while it is traversing it.
    
    E.g. the 'EraseRecords()' function.

    133e4928c4
20. walletdb: scope bdb::EraseRecords under a single db txn

    so we erase all the records atomically or abort the entire
    procedure.
    
    and, at the same time, we can share the same db txn context
    for the db cursor and the erase functionality.
    
    extra note from the Db.cursor doc:
    "If transaction protection is enabled, cursors must be
    opened and closed within the context of a transaction"
    
    thus why added a `CloseCursor` call before calling to
    `TxnAbort/TxnCommit`.

    9699222fa4
21. test: reorder 'wallet_load_ckey' test to use real db instead of mock

    As in the test we encrypt the wallet, and the encryption process
    requires a db rewrite + environment reload, we cannot use mock dbs
    (mock dbs are memory-only).

    32ffea27ab
22. furszy force-pushed on May 1, 2023
23. DrahtBot removed the label Needs rebase on May 1, 2023
24. furszy cross-referenced this on May 2, 2023 from issue wallet: fix deadlock in bdb read write operation by furszy
25. 
    achow101 commented at 7:16 PM on May 2, 2023: member

    Would it be easier to do this on top of #26715?

26. 
    furszy commented at 10:56 PM on May 2, 2023: member

    Would it be easier to do this on top of #26715?

    Was waiting for #27556 CI to close this in favor of #26715 actually. The heart of this PR is 32ffea2 which is just a test code re-ordering to by-pass the memory-only db limitations in the db rewrite + environment reload events executed in the wallet encryption process. Reality is that we don't care about which db engine the test uses, the test exercises loading invalid data into the wallet.

    With #26715 inclusion, the test db is just a raw map in-memory and not a temp bdb, so the issue does no longer exist.

27. furszy closed this on May 2, 2023

    ---

28. fanquake referenced this in commit d02df7db6b on May 15, 2023
29. achow101 referenced this in commit 6cc136bbd3 on May 18, 2023
30. sidhujag referenced this in commit 33242acf61 on May 19, 2023
31. furszy deleted the branch on May 27, 2023
32. bitcoin locked this on May 26, 2024

Contributors

furszyDrahtBotachow101

Review Requested
achow101