test: Skip copying BDB database directory in wallet backwards compatibility test #26915

1. 
   john-moffett commented at 8:23 pm on January 18, 2023: contributor

   Fixes #26869

   Version 5.x BerkeleyDB wallet.dat files are backward compatible with the version of BDB that Bitcoin uses, but the database transaction log files (eg - database/log.0000000001) are not.

   The legacy (BDB) backwards compatibility test fails intermittently because those log files aren’t always deleted when the wallet gets unloaded. It appears that DB_LOG_AUTO_REMOVE is the source of the non-determinism since there aren’t guarantees when it’ll run.

   Instead, just skip the logs when copying the wallet files to the older nodes. Alternative solutions include 1) restarting node_master instead of unloading so that the logs are guaranteed to be deleted or 2) manually deleting the logs before copying the tree, but those are slightly less time efficient.

2. Skip copying BDB database directory in test

   Version 5.x BerkeleyDB wallet.dat files are
   backward compatible with version 4.8 of BDB, but the
   database environment files (eg - log.0000000001) are not.
   
   The backwards compatibility test fails intermittently because
   the logs aren't always deleted when the wallet gets unloaded.
   
   Instead, just skip them when copying the wallet files to the
   older nodes.

   c4bb5c0d08
3. 
   DrahtBot commented at 8:23 pm on January 18, 2023: 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. A summary of reviews will appear here.

4. DrahtBot added the label Tests on Jan 18, 2023
5. 
   achow101 commented at 9:34 pm on January 18, 2023: member

   This might be safe, but I’m not convinced the tests will always be implemented correctly. I would rather that the CI that runs these tests would just use bdb 4.8 as we do in the releases to avoid these errors.
6. 
   john-moffett commented at 9:48 pm on January 18, 2023: contributor

   Sounds good. Will look into fixing the CI.
7. john-moffett closed this on Jan 18, 2023

   ---

8. 
   maflcko commented at 3:34 pm on February 7, 2023: member

   @john-moffett are you still working on this? Let us know if not, or if you have questions about the CI.
9. 
   john-moffett commented at 4:29 pm on February 7, 2023: contributor

   I’m not quite sure how to proceed. Some CI instances use --with-incompatible-bdb (specifically, valgrind, asan, and tidy). These are the only ones potentially susceptible to this specific error, though it’s unlikely to happen in normal use because they don’t download the old releases by default (ie - DOWNLOAD_PREVIOUS_RELEASES isn’t set). And it looks like tidy doesn’t even run any tests.

   I’m not sure if they use --with-incompatible-bdb just for convenience or whether there’s another reason. If the former, then I guess this PR should be fully dropped and (optionally) a new one opened to just make those two use the canonical BDB. Otherwise, I think this PR ought to be reopened, as it could be valuable for those wishing to build Bitcoin with a semi-compatible BDB.

10. 
    maflcko commented at 4:38 pm on February 7, 2023: member

    DOWNLOAD_PREVIOUS_RELEASES isn’t set

    Good point. I think the reason I am running into this more often locally is that the download folder is preserved between CI tasks and that the presence of the folder is enough to trigger the tests. (git grep PREVIOUS_RELEASES_DIR)

    So I have several thoughts:

    1. The releases dir could be a volume, similar to #27033
    2. The releases dir could remain unset for the other tasks?
    3. --with-incompatible-bdb could be removed?

    Currently I lean toward (1)

11. 
    john-moffett commented at 4:55 pm on February 7, 2023: contributor

    Any of those three seem sufficient, though maybe the combination of 1. and 3. is the best choice, since they’re independent small and sensible improvements.
12. bitcoin locked this on Feb 7, 2024

Contributors
john-moffett DrahtBot achow101 maflcko

Labels
Tests