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

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

   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.

   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 added the label Needs rebase on Jan 23, 2023
6. furszy force-pushed on Jan 25, 2023
7. DrahtBot removed the label Needs rebase on Jan 25, 2023
8. DrahtBot added the label Needs rebase on Feb 17, 2023
9. furszy force-pushed on Feb 20, 2023
10. DrahtBot removed the label Needs rebase on Feb 20, 2023
11. achow101 requested review from achow101 on Apr 25, 2023
12. DrahtBot added the label Needs rebase on May 1, 2023
13. 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
14. 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
15. 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
16. furszy force-pushed on May 1, 2023
17. DrahtBot removed the label Needs rebase on May 1, 2023
18. 
    achow101 commented at 7:16 pm on May 2, 2023: member

    Would it be easier to do this on top of #26715?
19. 
    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.

20. furszy closed this on May 2, 2023

    ---

21. fanquake referenced this in commit d02df7db6b on May 15, 2023
22. achow101 referenced this in commit 6cc136bbd3 on May 18, 2023
23. sidhujag referenced this in commit 33242acf61 on May 19, 2023
24. furszy deleted the branch on May 27, 2023
25. bitcoin locked this on May 26, 2024

Contributors
furszy DrahtBot achow101

Review Requested
achow101

Labels
Wallet