Return void instead of bool for functions that cannot fail by practicalswift · Pull Request #13774 · bitcoin/bitcoin

practicalswift · 2018-07-26T21:58:19Z

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"

maflcko · 2018-07-26T22:03:04Z

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.

practicalswift · 2018-07-27T09:34:36Z

@MarcoFalke Done! :-)

DrahtBot · 2018-07-27T10:00:50Z

Note to reviewers: This pull request conflicts with the following ones:

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

maflcko

ACK, beside one comment and one nit.

maflcko · 2018-07-27T11:02:18Z

src/validation.cpp

nit: Can keep the return g_ch... here, no?

maflcko · 2018-07-27T11:05:25Z

src/interfaces/wallet.cpp

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

maflcko · 2018-07-27T11:06:59Z

Also, could just squash into one commit?

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

practicalswift · 2018-07-27T11:19:34Z

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

maflcko · 2018-07-27T11:25:21Z

utACK d78a8dc

promag · 2018-07-27T13:27:02Z

utACK d78a8dc.

Empact · 2018-07-27T14:07:43Z

src/txdb.cpp


-bool CBlockTreeDB::ReadReindexing(bool &fReindexing) {
+void CBlockTreeDB::ReadReindexing(bool &fReindexing) {
    fReindexing = Exists(DB_REINDEX_FLAG);


How about return this instead?

Empact · 2018-07-27T14:12:21Z

src/validation.cpp

-bool ResetBlockFailureFlags(CBlockIndex *pindex) {
+
+void ResetBlockFailureFlags(CBlockIndex *pindex) {
    return g_chainstate.ResetBlockFailureFlags(pindex);


return in a void context

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

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

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.

Empact · 2018-07-27T14:14:12Z

src/wallet/wallet.cpp

@@ -4091,9 +4079,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,

            // generate a new master key
            CPubKey masterPubKey = walletInstance->GenerateNewSeed();


nit: could inline

Empact · 2018-07-27T14:14:20Z

src/wallet/wallet.cpp

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


nit: could inline

maflcko · 2018-07-27T15:04:08Z

@Empact I agree with the feedback, but this should be addressed in a separate pull request, if deemed important enough.

practicalswift · 2018-07-27T16:42:54Z

@Empact Does d78a8dc have your ACK? :-)

Empact · 2018-07-28T17:49:04Z

utACK d78a8dc

d78a8dc Return void instead of bool for functions that cannot fail (practicalswift) Pull request description: 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" ``` Tree-SHA512: c0014e045362dbcd1a0cc8f69844e7b8cbae4f538e7632028daeca3a797ac11d8d3d86ebc480bedcb8626df3e96779d592747d52a12556fc49921b114fa0ccc6

Summary: 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(...)` Backport of Bitcoin Core PR13774 bitcoin/bitcoin#13774 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, deadalnix, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4179

Follow-up for bitcoin#13774.

trongham

Tr

…annot fail d78a8dc Return void instead of bool for functions that cannot fail (practicalswift) Pull request description: 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" ``` Tree-SHA512: c0014e045362dbcd1a0cc8f69844e7b8cbae4f538e7632028daeca3a797ac11d8d3d86ebc480bedcb8626df3e96779d592747d52a12556fc49921b114fa0ccc6

fanquake added the Refactoring label Jul 27, 2018

practicalswift changed the title ~~wallet: SetMinVersion(...) cannot fail. Return void instead of bool.~~ Return void instead of bool for functions that cannot fail Jul 27, 2018

practicalswift mentioned this pull request Jul 27, 2018

Make ReceivedBlockTransactions return void #13412

Merged

maflcko reviewed Jul 27, 2018

View reviewed changes

practicalswift force-pushed the SetMinVersion-return-value branch 2 times, most recently from 519cb9e to d7f114a Compare July 27, 2018 11:17

practicalswift force-pushed the SetMinVersion-return-value branch from d7f114a to d78a8dc Compare July 27, 2018 11:19

Empact reviewed Jul 27, 2018

View reviewed changes

DrahtBot mentioned this pull request Jul 28, 2018

refactor: Avoid locking tx pool cs thrice #13786

Merged

DrahtBot mentioned this pull request Jul 29, 2018

tx pool: Avoid passing redundant hash into addUnchecked (scripted-diff) #13792

Merged

maflcko merged commit d78a8dc into bitcoin:master Jul 29, 2018

maflcko mentioned this pull request Aug 10, 2018

refactoring: Cleanup StartRest() #13938

Merged

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

ryanofsky mentioned this pull request Nov 12, 2018

Remove CWalletTx merging logic from AddToWallet #9381

Merged

kiminuo added a commit to kiminuo/bitcoin that referenced this pull request Apr 2, 2021

Remove superfluous "return" from "addUnchecked" in txmempool.cpp

4e41cee

Follow-up for bitcoin#13774.

kiminuo mentioned this pull request Apr 2, 2021

refactor: Remove superfluous "return" from "addUnchecked" in txmempool.cpp #21577

Closed

trongham reviewed Apr 3, 2021

View reviewed changes

practicalswift deleted the SetMinVersion-return-value branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

		@@ -4091,9 +4079,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,

		// generate a new master key
		CPubKey masterPubKey = walletInstance->GenerateNewSeed();

Conversation

practicalswift commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 26, 2018

Uh oh!

practicalswift commented Jul 27, 2018

Uh oh!

DrahtBot commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 27, 2018

Uh oh!

practicalswift commented Jul 27, 2018

Uh oh!

maflcko commented Jul 27, 2018

Uh oh!

promag commented Jul 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 27, 2018

Uh oh!

practicalswift commented Jul 27, 2018

Uh oh!

Empact commented Jul 28, 2018

Uh oh!

trongham left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Jul 26, 2018 •

edited

Loading

DrahtBot commented Jul 27, 2018 •

edited

Loading