bitcoin-wallet tool: Drop libbitcoin_server.a dependency #15639

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/link2 changing 18 files +89 −47

ryanofsky commented at 5:32 AM on March 22, 2019: member

Dropping the bitcoin-wallet dependency on libbitcoin_server.a ensures wallet code can't access node global state, avoiding bugs like #15557 (review)
ryanofsky force-pushed on Mar 22, 2019
fanquake added the label Wallet on Mar 22, 2019
ryanofsky force-pushed on Mar 22, 2019
DrahtBot commented at 7:25 AM on March 22, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15750 ([rpc] Remove the addresses field from the getaddressinfo return object by jnewbery)
- #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
- #15557 (Enhance bumpfee to include inputs when targeting a feerate by instagibbs)
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.
ryanofsky force-pushed on Mar 22, 2019
ryanofsky force-pushed on Mar 22, 2019
practicalswift commented at 10:09 AM on March 22, 2019: contributor

Concept ACK

Decoupling is good.

Somewhat related: #15612 ("Reduce the number of globals used")
l2a5b1 commented at 12:41 PM on March 25, 2019: contributor

I just left feedback in #15638 pertaining to the newly introduced circular dependencies. I agree with @practicalswift that decoupling is good, but I hope we can do this without introducing new circular dependencies.
ryanofsky referenced this in commit 9742ff8d79 on Mar 30, 2019
DrahtBot added the label Needs rebase on Apr 1, 2019
ryanofsky force-pushed on Apr 8, 2019
DrahtBot removed the label Needs rebase on Apr 8, 2019
DrahtBot added the label Needs rebase on Apr 9, 2019
ryanofsky force-pushed on Apr 10, 2019
DrahtBot removed the label Needs rebase on Apr 10, 2019

bitcoin-wallet tool: Drop MakeChain calls

Pass null Chain interface pointer to CWallet. This is needed to drop
libbitcoin_server dependency and avoid linking node code.

fbc6bb8e83

Remove access to node globals from wallet-linked code

Remove last few instances of accesses to node global variables from wallet
code. Also remove accesses to node globals from code in policy/policy.cpp that
isn't actually called by wallet code, but does get linked into wallet code.

This is the last change needed to allow bitcoin-wallet tool to be linked
without depending on libbitcoin_server.a, to ensure wallet code doesn't access
node global state and avoid bugs like
https://github.com/bitcoin/bitcoin/pull/15557#discussion_r267735431

b874747b51

bitcoin-wallet tool: Drop libbitcoin_server.a dependency

This ensures wallet code doesn't access node global state, avoiding bugs like
https://github.com/bitcoin/bitcoin/pull/15557#discussion_r267735431

78a2fb55c9

laanwj referenced this in commit 6a135fbe5b on Apr 10, 2019
ryanofsky force-pushed on Apr 10, 2019
ryanofsky commented at 2:33 PM on April 10, 2019: member

I just left feedback in #15638 pertaining to the newly introduced circular dependencies

Thanks, this was addressed some time ago in the other PR.
jgarzik approved
jgarzik commented at 2:50 PM on April 10, 2019: contributor

tested ACK.

Cosmetic question/comment: is the proj moving back towards underscores in variables like old school Unix (dust_relay_fee), or sticking with C++ style lower-cased first letter, and no underscores?
jnewbery commented at 3:13 PM on April 10, 2019: member

utACK 78a2fb55c97fbc26f7b74c5b1fb999a2aff8ce88. Nice work, Russ.
MarcoFalke commented at 3:47 PM on April 10, 2019: member

utACK 78a2fb5
meshcollider commented at 9:28 AM on April 11, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15639/commits/78a2fb55c97fbc26f7b74c5b1fb999a2aff8ce88
meshcollider merged this on Apr 11, 2019
meshcollider closed this on Apr 11, 2019
ryanofsky commented at 9:37 AM on April 11, 2019: member

re: #15639#pullrequestreview-225022833

Cosmetic question/comment: is the proj moving back towards underscores in variables like old school Unix

Pretty much yes. It's documented in the developer notes: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-general
deadalnix referenced this in commit ae40c8eaf2 on May 22, 2020
deadalnix referenced this in commit ed7b9d6689 on May 26, 2020
deadalnix referenced this in commit 103cee9a7f on May 27, 2020
deadalnix referenced this in commit a7a55fb3ac on May 27, 2020
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15557 Enhance `bumpfee` to include inputs when targeting a feerate #15638 Move-only: Pull wallet code out of libbitcoin_server #27322 Move IsDeprecatedRPCEnabled to rpc/util, rm redundant rpcEnableDeprecated

bitcoin-wallet tool: Drop libbitcoin_server.a dependency #15639

Conflicts