Wallet: Document expectations for AddWalletFlags (now InitWalletFlags) correctly #25784

1. 
   luke-jr commented at 2:57 AM on August 5, 2022: member

   Includes some slight refactoring (return type changed, current status checked)

2. Bugfix: Wallet: Document expectations for AddWalletFlags (now InitWalletFlags) correctly 0cb6d2aec6
3. DrahtBot added the label Wallet on Aug 5, 2022
4. 
   DrahtBot commented at 4:09 AM on August 5, 2022: contributor

   <!--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:

   • #25297 (wallet: speedup transactions sync, rescan and load by grouping all independent db writes on a single batched db transaction by furszy)

   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.

5. w0xlt approved
6. 
   w0xlt commented at 5:29 PM on August 5, 2022: contributor

   ACK https://github.com/bitcoin/bitcoin/pull/25784/commits/0cb6d2aec63aec76a517b8da621a3c53ab432632

   . InitWalletFlags is clearer about the purpose of the function. . The function returns a bool, but it is not used. So void is the correct return type. . assert(m_wallet_flags == 0) ensures that the wallet flags are blank (0).

7. in src/wallet/wallet.cpp:1488 in 0cb6d2aec6

   1484 |  {
   1485 |      LOCK(cs_wallet);
   1486 | +
   1487 |      // We should never be writing unknown non-tolerable wallet flags
   1488 |      assert(((flags & KNOWN_WALLET_FLAGS) >> 32) == (flags >> 32));
   1489 | +    // This should only be used once, when creating a new wallet - so current flags are expected to be blank
   

   ---

   ---

   

   ryanofsky commented at 6:46 PM on August 18, 2022:

   In commit "Bugfix: Wallet: Document expectations for AddWalletFlags (now InitWalletFlags) correctly" (0cb6d2aec63aec76a517b8da621a3c53ab432632)

   Might be better to say "initializing a newly created wallet" instead of "creating a new wallet". Just because of the CWallet::Create function "create" sometimes means load instead of newly create.

8. ryanofsky approved
9. 
   ryanofsky commented at 6:48 PM on August 18, 2022: contributor

   Code review ACK 0cb6d2aec63aec76a517b8da621a3c53ab432632. This is a clarifying change, and should prevent the InitWalletFlags method being called incorrectly. I left a comment suggestion, but feel free to ignore it.

10. fanquake requested review from achow101 on Aug 19, 2022
11. 
    achow101 commented at 4:01 PM on August 19, 2022: member

    ACK 0cb6d2aec63aec76a517b8da621a3c53ab432632

12. achow101 merged this on Aug 19, 2022
13. achow101 closed this on Aug 19, 2022

    ---

14. sidhujag referenced this in commit 8ccdfa3a55 on Aug 19, 2022
15. bitcoin locked this on Aug 19, 2023

Review Requested
achow101