Remove unused C++ code not covered by unit tests #10075

practicalswift · 2017-03-25T10:21:51Z

No description provided.

laanwj · 2017-03-25T10:24:34Z

src/rpc/protocol.cpp

@@ -114,13 +114,3 @@ bool GetAuthCookie(std::string *cookie_out)
        *cookie_out = cookie;
    return true;
 }
-
-void DeleteAuthCookie()


Huh, it's a mistake that this is unused; the auth cookie is supposed to be removed on shutdown.

Apparently I mistakenly removed the call from StopRPC in #5677/40b556d3742a1f65d67e2d4c760d0b13fe8be5b7. Good catch :)

laanwj · 2017-03-25T10:26:14Z

src/wallet/wallet.cpp

@@ -3495,22 +3495,6 @@ bool CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, c
    return true;
 }

-bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const


This is part of the *DestData API, please don't remove it just because it's not used at the moment. Same reasoning as here: #9987 (comment)

@laanwj Thanks for reviewing. I see - so basically a test is needed here :-)

laanwj · 2017-03-25T10:27:22Z

src/net.cpp

@@ -2616,7 +2598,6 @@ int CConnman::GetBestHeight() const
 }

 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
-unsigned int CConnman::GetSendBufferSize() const{ return nSendBufferMaxSize; }


CConnman is recent code and part of an ongoing effort by @theuni, please discuss with him before removing things.

@laanwj I see. @theuni, would you mind adding a test for this one? :-)

@practicalswift Yes, thanks, that's a good idea.

practicalswift · 2017-03-25T10:29:35Z

Trivia: This PR combined with #9987 removes 0.134 % (173 of 128 675 lines of code) of the Bitcoin C++ codebase :-)

$ git grep "" | egrep '\.(cpp|h):' | wc -l
128675

laanwj · 2017-03-25T10:29:48Z

src/chainparamsbase.cpp

@@ -103,8 +103,3 @@ std::string ChainNameFromCommandLine()
        return CBaseChainParams::TESTNET;
    return CBaseChainParams::MAIN;
 }
-
-bool AreBaseParamsConfigured()


ACK on this one, apparently the last use of this went away in #6149/27d760580456d206c5a02ef29797f296f510099c.

laanwj · 2017-03-25T10:33:30Z

src/key.cpp

@@ -131,14 +131,6 @@ void CKey::MakeNewKey(bool fCompressedIn) {
    fCompressed = fCompressedIn;
 }

-bool CKey::SetPrivKey(const CPrivKey &privkey, bool fCompressedIn) {


ACK on this one. This seems to have been unused since #5227/36fa4a78acac0ae6bb0e95c6ef78630120a28bdd.

laanwj · 2017-03-25T10:35:44Z

src/netaddress.cpp

@@ -179,12 +179,6 @@ bool CNetAddr::IsLocal() const
   return false;
 }

-bool CNetAddr::IsMulticast() const


ACK on this one. Cannot find when this was ever used, and I don't think we need this now or on the forseeable future.

Looks like it was introduced in #735 (5 years ago) and yes possibly never used.

laanwj · 2017-03-25T10:36:24Z

src/netaddress.cpp

@@ -570,11 +564,6 @@ std::string CService::ToString() const
    return ToStringIPPort();
 }

-void CService::SetPort(unsigned short portIn)


Setting ports seems to be reasonably useful part of the API, unless all of CService is made immutable I'd suggest keeping this.

laanwj · 2017-03-25T10:43:41Z

src/wallet/db.cpp

@@ -463,15 +463,6 @@ void CDBEnv::CloseDb(const std::string& strFile)
    }
 }

-bool CDBEnv::RemoveDb(const std::string& strFile)


Not 100% sure about this one. We don't use it, and haven't used it for a long time. From what I see it was introduced in eed1785 (pull# unknown) and never used. Would deleting wallets be something we'd ever support? (e.g. as part of multiwallet). It is an extremely dangerous thing to do, on the other hand for watch-only wallets it'd be useful.

@laanwj I'd vote for removing it until someone finds a need for it. Reasoning: such a dangerous operation should be tested, and also YAGNI :-)

practicalswift · 2017-03-28T07:46:01Z

@laanwj I've now addressed your review items. Please let me know if any further adjustments are needed :-)

fanquake · 2017-04-02T10:51:45Z

src/rpc/server.cpp

@@ -322,6 +322,7 @@ void StopRPC()
 {
    LogPrint("rpc", "Stopping RPC\n");
    deadlineTimers.clear();
+    DeleteAuthCookie();


I think this could/should be broken out into it's own PR. It's wasn't obvious right away why this was being added.

At least it's a separate commit in this PR. But splitting it to a separate PR would make sense, yes.

practicalswift · 2017-04-02T13:55:43Z

@fanquake @laanwj Good points! Re-introduction of auth cookie removal on shutdown has been moved to #10139 :-)

practicalswift · 2017-04-11T21:32:02Z

Let me know if there are any suggestions on further tweaks needed for this PR :-)

fanquake · 2017-04-26T10:43:06Z

This needs a rebase.

practicalswift · 2017-04-26T19:23:04Z

@fanquake Thanks for the notification. Now rebased!

Let me know if anything needs to be changed in this PR.

paveljanik · 2017-04-26T19:46:17Z

ACK b51aaf1

b51aaf1 Remove unused C++ code not covered by unit tests (practicalswift) Tree-SHA512: 267bbd87df01a296bf23e82a8b6ee968e13e23a6aaecc535d803890a3e3e9f6208c7fc4c1f97afd98ed3e498b12fe1ada7e3cb2977ad12359a813f57336c74e5

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

223bda1 key.cpp: Use PubKey constant size. (furszy) 0c6f95d remove unused code (furszy) Pull request description: Straightforward unused code cleanup coming from bitcoin#9987 and bitcoin#10075. ACKs for top commit: random-zebra: utACK 223bda1 Fuzzbawls: ACK 223bda1 Tree-SHA512: 9937edbcf4f59ba7835f889ea4638d9bd32e28e3cd865ed043a1f0856ab8ec2b08a3cd4f851536db1703b7f21d4910f22fb905231f73dd0ae7adecd6f5a07055

laanwj reviewed Mar 25, 2017

View reviewed changes

fanquake added the Refactoring label Mar 25, 2017

practicalswift force-pushed the unused branch 4 times, most recently from 9f4d9b4 to 1f8e7dc Compare March 28, 2017 06:40

fanquake reviewed Apr 2, 2017

View reviewed changes

practicalswift force-pushed the unused branch from 1f8e7dc to 32d30ca Compare April 2, 2017 13:54

Remove unused C++ code not covered by unit tests

b51aaf1

practicalswift force-pushed the unused branch from 32d30ca to b51aaf1 Compare April 26, 2017 19:20

laanwj merged commit b51aaf1 into bitcoin:master Apr 27, 2017

jtimon mentioned this pull request May 3, 2017

Use a proper factory for creating chainparams #8855

Merged

6 tasks

practicalswift deleted the unused branch April 10, 2021 19:30

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

furszy mentioned this pull request Sep 16, 2021

Cleaning unused code PIVX-Project/PIVX#2558

Merged

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

Remove unused C++ code not covered by unit tests #10075

Remove unused C++ code not covered by unit tests #10075

Uh oh!

Conversation

practicalswift commented Mar 25, 2017

Uh oh!

laanwj Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

practicalswift commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

practicalswift Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Apr 2, 2017

Uh oh!

practicalswift commented Apr 11, 2017

Uh oh!

fanquake commented Apr 26, 2017

Uh oh!

practicalswift commented Apr 26, 2017

Uh oh!

paveljanik commented Apr 26, 2017

Uh oh!

Uh oh!

laanwj Mar 25, 2017 •

edited

Loading

practicalswift commented Mar 25, 2017 •

edited

Loading

practicalswift Mar 25, 2017 •

edited

Loading