Page MenuHomePhabricator
Differential D7041

[refactor] remove const CChainParams& object from CWallet
ClosedPublic
Actions

Authored by majcosta on Jul 26 2020, 12:47.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCa1e3e13501c0: [refactor] remove const CChainParams& object from CWallet
Summary

this replaces the const chainParams& member in CWallet with a public method that retrieves it from its Chain interface

the exception is the case of the Chain-less bitcoin-wallet/wallet-tool, where it returns the global Params(), which at least is in single place now instead of a billion.

Test Plan
ninja check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

majcosta created this revision.Jul 26 2020, 12:47
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 26 2020, 12:47
majcosta requested review of this revision.Jul 26 2020, 12:47
Harbormaster completed remote builds in B12163: Diff 22520.Jul 26 2020, 13:02
majcosta planned changes to this revision.Jul 26 2020, 13:34

this might not be needed

majcosta requested review of this revision.Jul 30 2020, 20:02
Fabien added a subscriber: Fabien.Jul 31 2020, 08:47

What's the use case ?

deadalnix requested changes to this revision.Jul 31 2020, 20:31
deadalnix added a subscriber: deadalnix.

@majcosta While I don't have anything against this per se, packaging it with some code that actually uses this would be beneficial. I assume to is about removing the params member of CWallet because it is redundant with the chain member. Here is what I suggest you do:

1/ Use the chain member in CWallet instead of param where possible without this change.
2/ Do this change with the removal of the param member in CWallet.

This revision now requires changes to proceed.Jul 31 2020, 20:31
majcosta mentioned this in D7736: [refactor] replace calls to Params() CWallet chainParams member.Oct 2 2020, 10:33
majcosta mentioned this in D7744: [refactor] replace global Params() use with CWallet chainParams member.Oct 2 2020, 14:24
majcosta mentioned this in rABC314bf14ecf3e: [refactor] replace calls to Params() CWallet chainParams member.Oct 2 2020, 19:09
majcosta updated this revision to Diff 24233.Oct 2 2020, 19:24

addressed feedback

majcosta added a comment.Oct 2 2020, 19:27

still figuring out a heap-use-after-free ASAN warning on this thing

Harbormaster completed remote builds in B13172: Diff 24233.Oct 2 2020, 19:43
majcosta retitled this revision from [refactor] allow access to ChainImpl's m_params via Chain interface to [refactor] remove const CChainParams& object from CWallet.Oct 2 2020, 19:55
majcosta edited the summary of this revision. (Show Details)
majcosta edited the test plan for this revision. (Show Details)
teamcity added a comment.Oct 2 2020, 23:55
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Snippet of first build failure:

rpc_getblockstats.py                             | ✓ Passed  | 1 s
rpc_getchaintips.py                              | ✓ Passed  | 3 s
rpc_help.py                                      | ✓ Passed  | 1 s
rpc_invalidateblock.py                           | ✓ Passed  | 6 s
rpc_misc.py                                      | ✓ Passed  | 1 s
rpc_named_arguments.py                           | ✓ Passed  | 1 s
rpc_net.py                                       | ✓ Passed  | 1 s
rpc_preciousblock.py                             | ✓ Passed  | 1 s
rpc_psbt.py                                      | ✓ Passed  | 44 s
rpc_scantxoutset.py                              | ✓ Passed  | 5 s
rpc_setban.py                                    | ✓ Passed  | 2 s
rpc_signmessage.py                               | ✓ Passed  | 1 s
rpc_signrawtransaction.py                        | ✓ Passed  | 1 s
rpc_txoutproof.py                                | ✓ Passed  | 2 s
rpc_uptime.py                                    | ✓ Passed  | 1 s
rpc_users.py                                     | ✓ Passed  | 2 s
rpc_whitelist.py                                 | ✓ Passed  | 1 s
tool_wallet.py                                   | ✓ Passed  | 4 s
wallet_abandonconflict.py                        | ✓ Passed  | 15 s
wallet_address_types.py                          | ✓ Passed  | 14 s
wallet_avoidreuse.py                             | ✓ Passed  | 4 s
wallet_backup.py                                 | ✓ Passed  | 37 s
wallet_balance.py                                | ✓ Passed  | 8 s
wallet_basic.py                                  | ✓ Passed  | 23 s
wallet_coinbase_category.py                      | ✓ Passed  | 1 s
wallet_create_tx.py                              | ✓ Passed  | 10 s
wallet_createwallet.py                           | ✓ Passed  | 2 s
wallet_createwallet.py --usecli                  | ✓ Passed  | 3 s
wallet_disable.py                                | ✓ Passed  | 1 s
wallet_dump.py                                   | ✓ Passed  | 3 s
wallet_encryption.py                             | ✓ Passed  | 5 s
wallet_groups.py                                 | ✓ Passed  | 12 s
wallet_hd.py                                     | ✓ Passed  | 5 s
wallet_import_rescan.py                          | ✓ Passed  | 5 s
wallet_import_with_label.py                      | ✓ Passed  | 1 s
wallet_importmulti.py                            | ✓ Passed  | 3 s
wallet_importprunedfunds.py                      | ✓ Passed  | 2 s
wallet_keypool.py                                | ✓ Passed  | 2 s
wallet_keypool_topup.py                          | ✓ Passed  | 2 s
wallet_labels.py                                 | ✓ Passed  | 3 s
wallet_listreceivedby.py                         | ✓ Passed  | 11 s
wallet_listsinceblock.py                         | ✓ Passed  | 3 s
wallet_listtransactions.py                       | ✓ Passed  | 13 s
wallet_multiwallet.py                            | ✓ Passed  | 16 s
wallet_multiwallet.py --usecli                   | ✓ Passed  | 45 s
wallet_reorgsrestore.py                          | ✓ Passed  | 3 s
wallet_resendwallettransactions.py               | ✓ Passed  | 5 s
wallet_txn_clone.py                              | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock                  | ✓ Passed  | 3 s
wallet_txn_doublespend.py                        | ✓ Passed  | 2 s
wallet_txn_doublespend.py --mineblock            | ✓ Passed  | 4 s
wallet_watchonly.py                              | ✓ Passed  | 1 s
wallet_watchonly.py --usecli                     | ✓ Passed  | 1 s
wallet_zapwallettxes.py                          | ✓ Passed  | 5 s

ALL                                              | ✓ Passed  | 799 s (accumulated) 
Runtime: 161 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
deadalnix requested changes to this revision.Oct 3 2020, 13:02

Looks good, but you need to verify why the test suite is busted.

This revision now requires changes to proceed.Oct 3 2020, 13:02
majcosta updated this revision to Diff 26253.Dec 7 2020, 19:08

rebase

Harbormaster completed remote builds in B14340: Diff 26253.Dec 7 2020, 19:48
deadalnix accepted this revision.Dec 8 2020, 00:43
deadalnix added inline comments.
src/qt/test/addressbooktests.cpp
65 ↗(On Diff #26253)

I think you messed up the rebase.

src/wallet/wallet.cpp
294 ↗(On Diff #26253)

I got to say, this is both ugly and beautiful XD

src/wallet/wallet.h
818 ↗(On Diff #26253)

likestamp

This revision is now accepted and ready to land.Dec 8 2020, 00:43
Closed by commit rABCa1e3e13501c0: [refactor] remove const CChainParams& object from CWallet (authored by majcosta). · Explain WhyDec 8 2020, 15:00
This revision was automatically updated to reflect the committed changes.
majcosta added a commit: rABCa1e3e13501c0: [refactor] remove const CChainParams& object from CWallet.
PiRK mentioned this in D10188: remove CChainParams from wallet methods that don't need it.Sep 23 2021, 12:02
PiRK mentioned this in rABC9a147d403524: remove CChainParams from wallet methods that don't need it.Sep 27 2021, 06:13

Revision Contents
Changeset List

PathSize
src/
bench/
4 lines
3 lines
interfaces/
3 lines
1 line
2 lines
qt/
test/
6 lines
6 lines
test/
util/
2 lines
wallet/
10 lines
35 lines
2 lines
test/
9 lines
20 lines
3 lines
32 lines
2 lines
10 lines
11 lines
8 lines
4 lines

Diff 26267

View Options

src/bench/coin_selection.cpp

Loading...
View Options

src/bench/wallet_balance.cpp

Loading...
View Options

src/interfaces/chain.h

Loading...
View Options

src/interfaces/chain.cpp

Loading...
View Options

src/interfaces/wallet.cpp

Loading...
View Options

src/qt/test/addressbooktests.cpp

Loading...
View Options

src/qt/test/wallettests.cpp

Loading...
View Options

src/test/util/wallet.cpp

Loading...
View Options

src/wallet/rpcdump.cpp

Loading...
View Options

src/wallet/rpcwallet.cpp

Loading...
View Options

src/wallet/salvage.cpp

Loading...
View Options

src/wallet/test/coinselector_tests.cpp

Loading...
View Options

src/wallet/test/ismine_tests.cpp

Loading...
View Options

src/wallet/test/wallet_test_fixture.cpp

Loading...
View Options

src/wallet/test/wallet_tests.cpp

Loading...
View Options

src/wallet/test/walletdb_tests.cpp

Loading...
View Options

src/wallet/wallet.h

Loading...
View Options

src/wallet/wallet.cpp

Loading...
View Options

src/wallet/walletdb.cpp

Loading...
View Options

src/wallet/wallettool.cpp

Loading...