Return void instead of bool for functions that cannot fail #13774

1. 
   practicalswift commented at 9:58 pm on July 26, 2018: contributor

   Return void instead of bool for functions that cannot fail:

   • CBlockTreeDB::ReadReindexing(...)
   • CChainState::ResetBlockFailureFlags(...)
   • CTxMemPool::addUnchecked(...)
   • CWallet::CommitTransaction(...)
   • CWallet::LoadDestData(...)
   • CWallet::LoadKeyMetadata(...)
   • CWallet::LoadScriptMetadata(...)
   • CWallet::LoadToWallet(...)
   • CWallet::SetHDChain(...)
   • CWallet::SetHDSeed(...)
   • PendingWalletTx::commit(...)
   • RemoveLocal(...)
   • SetMinVersion(...)
   • StartHTTPServer(...)
   • StartRPC(...)
   • TorControlConnection::Disconnect(...)

   Some of the functions can fail by throwing.

   Found by manually inspecting the following candidate functions:

   0$ git grep -E '(^((static|virtual|inline|friend)[^a-z])*[^a-z]*bool [^=]*\(|return true|return false)' -- "*.cpp" "*.h"
   

2. 
   MarcoFalke commented at 10:03 pm on July 26, 2018: member

   Hmm, could you do the same for all other functions that qualify (e.g. CTxMemPool::addUnchecked)? Would be nice if we could avoid a flood of pulls that do the same. See also #13412.
3. fanquake added the label Refactoring on Jul 27, 2018
4. practicalswift renamed this:
   wallet: SetMinVersion(...) cannot fail. Return void instead of bool.
   Return void instead of bool for functions that cannot fail
   on Jul 27, 2018
5. 
   practicalswift commented at 9:34 am on July 27, 2018: contributor

   @MarcoFalke Done! :-)
6. 
   DrahtBot commented at 10:00 am on July 27, 2018: member

   • #13792 (tx pool: Avoid passing redundant hash into addUnchecked by MarcoFalke)
   • #13786 (refactor: Avoid locking tx pool cs thrice by MarcoFalke)
   • #9381 (Remove CWalletTx merging logic from AddToWallet 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.

7. in src/validation.cpp:2920 in 31d1268f1a outdated

   2918 }
   2919-bool ResetBlockFailureFlags(CBlockIndex *pindex) {
   2920-    return g_chainstate.ResetBlockFailureFlags(pindex);
   2921+
   2922+void ResetBlockFailureFlags(CBlockIndex *pindex) {
   2923+    g_chainstate.ResetBlockFailureFlags(pindex);
   

   ---

   ---

   

   MarcoFalke commented at 11:02 am on July 27, 2018:

   nit: Can keep the return g_ch... here, no?
8. in src/interfaces/wallet.cpp:47 in 31d1268f1a outdated

   48-        if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), std::move(from_account), m_key, g_connman.get(), state)) {
   49-            reject_reason = state.GetRejectReason();
   50-            return false;
   51-        }
   52-        return true;
   53+        m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), std::move(from_account), m_key, g_connman.get(), state);
   

   ---

   ---

   

   MarcoFalke commented at 11:05 am on July 27, 2018:

   CommitTransaction only temporarily returns true in all cases. This is about to change soonTM. Better leave this as is for now, since we pass in a state, which would be confusing if the return type was void
9. 
   MarcoFalke commented at 11:06 am on July 27, 2018: member

   ACK, beside one comment and one nit.
10. 
    MarcoFalke commented at 11:06 am on July 27, 2018: member

    Also, could just squash into one commit?
11. practicalswift force-pushed on Jul 27, 2018
12. practicalswift force-pushed on Jul 27, 2018
13. Return void instead of bool for functions that cannot fail

    * CBlockTreeDB::ReadReindexing(...)
    * CChainState::ResetBlockFailureFlags(...)
    * CTxMemPool::addUnchecked(...)
    * CWallet::LoadDestData(...)
    * CWallet::LoadKeyMetadata(...)
    * CWallet::LoadScriptMetadata(...)
    * CWallet::LoadToWallet(...)
    * CWallet::SetHDChain(...)
    * CWallet::SetHDSeed(...)
    * RemoveLocal(...)
    * SetMinVersion(...)
    * StartHTTPServer(...)
    * StartRPC(...)
    * TorControlConnection::Disconnect(...)

    d78a8dc3e8
14. practicalswift force-pushed on Jul 27, 2018
15. 
    practicalswift commented at 11:19 am on July 27, 2018: contributor

    @MarcoFalke Thanks for reviewing! Feedback addressed. Please re-review :-)
16. 
    MarcoFalke commented at 11:25 am on July 27, 2018: member

    utACK d78a8dc3e82564ca8e56b81f9f21af9295b013dd
17. 
    promag commented at 1:27 pm on July 27, 2018: member

    utACK d78a8dc.
18. in src/txdb.cpp:164 in d78a8dc3e8

    159@@ -160,9 +160,8 @@ bool CBlockTreeDB::WriteReindexing(bool fReindexing) {
    160         return Erase(DB_REINDEX_FLAG);
    161 }
    162 
    163-bool CBlockTreeDB::ReadReindexing(bool &fReindexing) {
    164+void CBlockTreeDB::ReadReindexing(bool &fReindexing) {
    165     fReindexing = Exists(DB_REINDEX_FLAG);
    

    ---

    ---

    

    Empact commented at 2:07 pm on July 27, 2018:

    How about return this instead?
19. in src/validation.cpp:2920 in d78a8dc3e8

    2913@@ -2914,9 +2914,9 @@ bool CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
    2914         }
    2915         pindex = pindex->pprev;
    2916     }
    2917-    return true;
    2918 }
    2919-bool ResetBlockFailureFlags(CBlockIndex *pindex) {
    2920+
    2921+void ResetBlockFailureFlags(CBlockIndex *pindex) {
    2922     return g_chainstate.ResetBlockFailureFlags(pindex);
    

    ---

    ---

    

    Empact commented at 2:12 pm on July 27, 2018:

    return in a void context

    ---

    

    MarcoFalke commented at 3:03 pm on July 27, 2018:

    We want to assert that the return code type is the same for both functions, so this looks correct as is.

    ---

    

    Empact commented at 4:55 pm on July 27, 2018:

    So this is pending future use? If not I don’t follow.

    ---

    

    MarcoFalke commented at 6:07 pm on July 27, 2018:

    Imagine that Chainstate::ResetBlockFailureFlags returns bool in the future. This prevents the code from compiling, whereas removing the return would happily compile and hide a potential bug, no?

    Also, changing it to something else in the future keeps the future diff smaller while keeping the return.

20. in src/wallet/wallet.cpp:4081 in d78a8dc3e8

    4078@@ -4091,9 +4079,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
    4079 
    4080             // generate a new master key
    4081             CPubKey masterPubKey = walletInstance->GenerateNewSeed();
    

    ---

    ---

    

    Empact commented at 2:14 pm on July 27, 2018:

    nit: could inline
21. in src/wallet/wallet.cpp:4118 in d78a8dc3e8

    4115@@ -4130,9 +4116,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
    4116         } else {
    4117             // generate a new seed
    4118             CPubKey seed = walletInstance->GenerateNewSeed();
    

    ---

    ---

    

    Empact commented at 2:14 pm on July 27, 2018:

    nit: could inline
22. 
    MarcoFalke commented at 3:04 pm on July 27, 2018: member

    @Empact I agree with the feedback, but this should be addressed in a separate pull request, if deemed important enough.
23. 
    practicalswift commented at 4:42 pm on July 27, 2018: contributor

    @Empact Does d78a8dc have your ACK? :-)
24. 
    Empact commented at 5:49 pm on July 28, 2018: member

    utACK d78a8dc
25. MarcoFalke merged this on Jul 29, 2018
26. MarcoFalke closed this on Jul 29, 2018

    ---

27. MarcoFalke referenced this in commit ad51e1372b on Jul 29, 2018
28. jasonbcox referenced this in commit a0dd249c18 on Oct 11, 2019
29. jonspock referenced this in commit b10702e679 on Dec 27, 2019
30. jonspock referenced this in commit 44e67211c2 on Dec 29, 2019
31. kiminuo referenced this in commit 4e41cee9f5 on Apr 2, 2021
32. 
    trongham commented at 8:40 am on April 3, 2021: none

    Tr
33. practicalswift deleted the branch on Apr 10, 2021
34. NickIAm referenced this in commit 5b5f31d027 on Apr 18, 2021
35. UdjinM6 referenced this in commit fdc7d6a575 on Jun 29, 2021
36. UdjinM6 referenced this in commit 3ff9ee0926 on Jun 29, 2021
37. UdjinM6 referenced this in commit dccbf7ecfa on Jul 1, 2021
38. UdjinM6 referenced this in commit 36804fb3a1 on Jul 2, 2021
39. UdjinM6 referenced this in commit 570d573786 on Jul 2, 2021
40. gades referenced this in commit fae6911bdd on May 4, 2022
41. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift MarcoFalke DrahtBot promag Empact trongham

Labels
Refactoring