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

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:SetMinVersion-return-value changing 17 files +41 −74
  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:

    $ 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. practicalswift cross-referenced this on Jul 27, 2018 from issue Make ReceivedBlockTransactions return void by MarcoFalke
  7. DrahtBot commented at 10:00 AM on July 27, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #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.

  8. 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?

  9. 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

  10. MarcoFalke commented at 11:06 AM on July 27, 2018: member

    ACK, beside one comment and one nit.

  11. MarcoFalke commented at 11:06 AM on July 27, 2018: member

    Also, could just squash into one commit?

  12. practicalswift force-pushed on Jul 27, 2018
  13. practicalswift force-pushed on Jul 27, 2018
  14. 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
  15. practicalswift force-pushed on Jul 27, 2018
  16. practicalswift commented at 11:19 AM on July 27, 2018: contributor

    @MarcoFalke Thanks for reviewing! Feedback addressed. Please re-review :-)

  17. MarcoFalke commented at 11:25 AM on July 27, 2018: member

    utACK d78a8dc3e82564ca8e56b81f9f21af9295b013dd

  18. promag commented at 1:27 PM on July 27, 2018: member

    utACK d78a8dc.

  19. 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?

  20. 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.

  21. 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

  22. 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

  23. 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.

  24. practicalswift commented at 4:42 PM on July 27, 2018: contributor

    @Empact Does d78a8dc have your ACK? :-)

  25. DrahtBot cross-referenced this on Jul 28, 2018 from issue refactor: Avoid locking tx pool cs thrice by MarcoFalke
  26. Empact commented at 5:49 PM on July 28, 2018: member

    utACK d78a8dc

  27. DrahtBot cross-referenced this on Jul 29, 2018 from issue tx pool: Avoid passing redundant hash into addUnchecked (scripted-diff) by MarcoFalke
  28. MarcoFalke merged this on Jul 29, 2018
  29. MarcoFalke closed this on Jul 29, 2018
  30. MarcoFalke referenced this in commit ad51e1372b on Jul 29, 2018
  31. MarcoFalke cross-referenced this on Aug 10, 2018 from issue refactoring: Cleanup StartRest() by DesWurstes
  32. Bushstar cross-referenced this on Aug 13, 2018 from issue commits from bitcoin/master by Bushstar
  33. ryanofsky cross-referenced this on Nov 12, 2018 from issue Remove CWalletTx merging logic from AddToWallet by ryanofsky
  34. jasonbcox referenced this in commit a0dd249c18 on Oct 11, 2019
  35. jonspock referenced this in commit b10702e679 on Dec 27, 2019
  36. jonspock referenced this in commit 44e67211c2 on Dec 29, 2019
  37. kiminuo referenced this in commit 4e41cee9f5 on Apr 2, 2021
  38. kiminuo cross-referenced this on Apr 2, 2021 from issue refactor: Remove superfluous "return" from "addUnchecked" in txmempool.cpp by kiminuo
  39. trongham commented at 8:40 AM on April 3, 2021: none

    Tr

  40. practicalswift deleted the branch on Apr 10, 2021
  41. NickIAm referenced this in commit 5b5f31d027 on Apr 18, 2021
  42. UdjinM6 referenced this in commit fdc7d6a575 on Jun 29, 2021
  43. UdjinM6 referenced this in commit 3ff9ee0926 on Jun 29, 2021
  44. UdjinM6 referenced this in commit dccbf7ecfa on Jul 1, 2021
  45. UdjinM6 referenced this in commit 36804fb3a1 on Jul 2, 2021
  46. UdjinM6 referenced this in commit 570d573786 on Jul 2, 2021
  47. gades referenced this in commit fae6911bdd on May 4, 2022
  48. bitcoin locked this on Aug 16, 2022
Contributors
practicalswiftMarcoFalkeDrahtBotpromagEmpacttrongham
Labels
Refactoring
Linked (view graph)
#9381 Remove CWalletTx merging logic from AddToWallet#13412 Make ReceivedBlockTransactions return void#13938 refactoring: Cleanup StartRest()#21577 refactor: Remove superfluous "return" from "addUnchecked" in txmempool.cpp
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC