Move-only: Pull wallet code out of libbitcoin_server #15638

ryanofsky · 2019-03-22T05:00:15Z

This is a move-only commit. No code is changing and the moves can be easily verified with:

git log -p -n1 --color-moved=dimmed_zebra

This commit moves functions and variables that wallet code depends on out of libbitcoin_server.a, so the bitcoin-wallet tool can be built without libbitcoin_server.a in #15639, and attempting to access server state from wallet code will result in link errors instead of silently broken code.

List of moves:

CheckTransaction moves from consensus/tx_verify.cpp to consensus/tx_check.cpp
urlDecode moves from httpserver.cpp to util/url.cpp
TransactionErrorString moves from node/transaction.cpp to util/error.cpp
StringForFeeReason and FeeModeFromString move from policy/fees.cpp to util/fees.cpp
incrementalRelayFee dustRelayFee and nBytesPerSigOp move from policy/policy.cpp to policy/settings.cpp
SignalsOptInRBF moves from policy/rbf.cpp to util/rbf.cpp
fIsBareMultisigStd moves from validation.cpp to policy/settings.cpp
ConstructTransaction TxInErrorToJSON and SignTransaction move from rpc/rawtransaction.cpp to rpc/rawtransaction_util.cpp
RPCTypeCheck RPCTypeCheckArgument RPCTypeCheckObj AmountFromValue ParseHashV``ParseHashO ParseHexV ParseHexO HelpExampleCli and HelpExampleRpc move from rpc/server.cpp to rpc/util.cpp
AmountHighWarn and AmountErrMsg move from ui_interface.cpp to util/error.cpp
FormatStateMessage and strMessageMagic move from validation.cpp to util/validation.cpp
VerifyWallets LoadWallets StartWallets FlushWallets StopWallets and UnloadWallets move from wallet/init.cpp to wallet/node.cpp

DrahtBot · 2019-03-22T07:25:59Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
#15141 (Rewrite DoS interface between validation and net_processing by sdaftuar)
#14837 (Stricter & More Performant Invariant Checking in CheckBlock by JeremyRubin)
#14696 (qa: Add explicit references to related CVE's in p2p_invalid_block test. by lucash-dev)
#13949 (Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency by Empact)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
#13357 (Define SIGHASH_MASK in validation and determine the use of SIGHASH_SINGLE in signing by jl2012)
#12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
#10823 (Allow all mempool txs to be replaced after a configurable timeout (default 6h) by greenaddress)
#10443 (Add fee_est tool for debugging fee estimation code 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.

Empact · 2019-03-22T09:08:11Z

I split this out for review here: https://github.com/Empact/bitcoin/commits/pr/link-split (you can diff Empact@9d9ad38 for consistency) and came away with 2 proposed changes:

~~Empact@3d5f9e9 - TransactionErrorString is not used outside rpc/util.cpp, which includes node/transaction.h via rpc/util.h. Seems appropriate to leave it there.~~

Empact@173d3c4 - this moves StringFeeForReason back as it's only used in wallet/wallet.cpp which includes policy/fees.h directly. It also makes util/fees independent of policy/fees, which seems appropriate re. layering.

ryanofsky · 2019-03-22T11:51:41Z

@Empact, I can use your branch with split up commits if other people think that is easier to review. But I'm not sure that they will. Personally I would rather scan a single commit with --color-moved=dimmed_zebra, verify that it is move-only, and be able to see the whole picture than to have to verify fragmentary commits.

The two changes you propose are not good, though. They have some problems conceptually, and I think will break the followup PR #15639, although I didn't test.

The point of this PR is to designate a directory (src/node/) and a library (libbitcoin_server.a) which contain node-specific code, which wallet code cannot call, and qt code cannot call. This is enforced by the linker after #15639, and should be clearly explained in the readme and makefile.

Both wallet code and qt code should be able to translate error codes into strings. So TransactionErrorString belongs in a place like util/error.h and not underneath src/node/. (Beyond #15639, I would like util/error.h to be a common place for defining simple error types and messages that don't pull in outside dependencies, so error values only need to be defined once, and we don't need to write unnecessary code to catch exceptions or translate errors to different types when they are passed between wallet/node/rpc/gui code boundaries.)

Similarly, both wallet code and qt code should be able to call StringForFeeReason, which means it shouldn't be defined in src/policy/fees.cpp, because that file uses mempool types and would require wallet and qt code to be linked with mempool code, which is what this PR is trying to avoid and would be an error after #15639.

jnewbery · 2019-03-22T14:14:50Z

Very strong concept ACK to this and #15639. This is the natural follow-up to #10973. That PR defined a node<->wallet interface and made it so wallet code shouldn't directly access node functions. This PR with #15639 ensures that wallet code can't directly access node functions.

My only question is when these should be merged. This conflicts with a lot of PRs (including high-priority #15141) so perhaps it should wait. On the other hand, rebases will be trivial since this is just a code move.

test/lint/lint-circular-dependencies.sh

jnewbery · 2019-03-25T13:30:24Z

if other people think that is easier to review

I think it would be slightly easier to review if this was split into multiple commits, each moving a function with with justification for the move.

Sjors

tACK b35ba2c

The dimmed zebra helps with checking the moves. I'm fine with either splitting this into smaller commits that explain which things are are moved and why, or just putting a list in the PR description.

I'm able to compile on macOS 10.14.3 with and without wallet.

For some reason the x64 build in Visual Studio 2017 on Windows breaks:

AppVeyor seems happy though. I did a clean before building. Building the parent commit also breaks, although with different errors. Maybe we recently introduced a problem? cc @ken2812221 & @sipsorcery

src/consensus/tx_check.h

sipsorcery · 2019-03-28T18:32:05Z

@Sjors this PR builds correctly for me with VS 2017 x64 Debug and Release. Did you re-run the msvc-autogen.py script after pulling the changes? There have been modifications to the makefiles so the VS project files need to be re-generated.

Sjors · 2019-03-29T07:48:16Z

@sipsorcery I did not. Can't test it at the moment, but that seems like a plausible explanation.

ryanofsky

Updated b35ba2c -> 9742ff8 (pr/link.5 -> pr/link.6, compare) adding suggested comments, removing circular lint warnings.

re: #15638 (review)

The dimmed zebra helps with checking the moves. I'm fine with either splitting this into smaller commits that explain which things are are moved and why, or just putting a list in the PR description

I added a list of moves to the PR description. A few people have requested splitting this PR up, and if someone still thinks this is a good idea, I'd urge them to at least first spend a few minutes looking over the commit with --color-moved=dimmed_zebra.

I don't want to break up this PR into a large number of commits, because we have lots of circular dependencies and lots of different binaries (tests, benchmarks, tools, frontends) that pull object files in with different link orders. We also build on many different platforms, so right now any commit that moves a symbol from one object file to another is basically playing russian roullete with travis and appveyor. I spent hours after I first got this PR working locally to eventually get it working on the various travis platforms and appveyor. I don't want to have to repeat this work again with a bunch of individual commits, or to blindly push a bunch of broken commits. I think splitting the commit up at best would only make it slightly easier to review with --color-moved=dimmed_zebra, and in actuality would just make this more intimidating and more awkward to review and maintain. I think a bitcoin contributor who literally spent 10 minutes looking at the current commit with --color-moved=dimmed_zebra, could fully review the PR, verifying that it is move-only and reading all the new comments.

src/consensus/tx_check.h

test/lint/lint-circular-dependencies.sh

ryanofsky · 2019-04-02T17:14:15Z

John was asking questions about why different functions were moving to various libraries. I'll describe how I was thinking about this below, but I don't consider the question to be extremely important, because the libraries are already pretty scrambled up, and I'm not trying to unscramble them here. All I am trying to do is split up .cpp files that have functions called by the wallet, so wallet code that links these .cpp files doesn't pull in a bunch of node dependencies.

But just to provide reassurance that I am not messing up the libraries in some way here, here is how I am thinking about the organization:

libbitcoin_server.a, libbitcoin_wallet.a, and libbitcoinqt.a should all be terminal dependencies. They should be able to depend on other symbols in other libraries, but no other libraries should depend on symbols in them (and they shouldn't depend on each other).
libbitcoin_consensus.a should be a standalone library that doesn't depend on symbols in other libraries mentioned here
libbitcoin_common.a and libbitcoin_util.a seem very interchangeable right now and mutually depend on each other. I think we should either merge them into one library, or create a new top-level src/common/ directory complementing src/util/, and start to distinguish general purpose utility code (like argument parsing) from bitcoin-specific utility code (like formatting bip32 paths and using ChainParams). Both these libraries can be depended on by libbitcoin_server.a, libbitcoin_wallet.a, and libbitcoinqt.a, and they can depend on libbitcoin_consensus.a. If we want to split util and common up, as opposed to merging them together, then util shouldn't depend on libconsensus, but common should.

Over time, I think it'd be nice if source code organization reflected library organization . I think it'd be nice if all libbitcoin_util source files lived in src/util, all libbitcoin_consensus.a source files lived in src/consensus, and all libbitcoin_server.a code lived in src/node (and maybe the library was called libbitcoin_node.a).

jnewbery · 2019-04-02T21:36:43Z

I split this PR into smaller commits to help my review: https://github.com/jnewbery/bitcoin/tree/pr15638_8 . I pushed each commit as a separate branch on my fork to be built by travis: https://travis-ci.org/jnewbery/bitcoin/builds.

I personally find this easier to review as each move is justified by its commit log.

I'll describe how I was thinking about this below [...] here is how I am thinking about the organization

This is really useful. Would you mind opening an issue with basically the description you've given above. I think it'd be useful to have as a reference for how we think the code organization should be moving, and as a place to solicit feedback from other contributors.

jnewbery · 2019-04-02T21:39:12Z

@laanwj has also expressed a desire to reorganize the source code (#15465 (comment)):

What kind of frustrates me, personally, is that we didn't manage to clearly separate out the consensus parts. Not to a library (which would be useful for rust-bitcoin et al), but also not even to a separate directory.

Empact · 2019-04-03T05:40:46Z

Here are 2 revised proposals, related to the earlier work:

Empact@c72764e - Moves TransactionError to its own file, src/transaction_error.h, revises includes to require the more narrow transaction_error rather than node/transaction where appropriate.

Empact@90293e6 - Now just makes util/fees.h the source for FeeEstimateMode, FeeReason, and related methods, so that their declarations and definitions are in a common location.

Based on the current pr/link

practicalswift · 2019-04-03T07:42:24Z

Concept ACK

ryanofsky · 2019-04-03T17:36:51Z

Updated 9742ff8 -> fee746f (pr/link.6 -> pr/link.7, compare) to John's branch pr15638_8, adding a minor fix to the fourth commit (118f5fd), which didn't compile

* merge 15638: Move CheckTransaction from lib_server to lib_consensus * merge 15638: Move policy settings to new src/policy/settings unit * merge 15638: Move rpc utility methods to rpc/util * merge 15638: Move rpc rawtransaction util functions to rpc/rawtransaction_util.cpp * merge 15638: Move several units into common libraries * merge 15638: Move wallet load functions to wallet/load unit * merge 15638: Document src subdirectories and different libraries * [build] Add several util units (cleanup) * build: resolve missing declarations by re-specifying headers

…ay#4560) * merge 15638: Move CheckTransaction from lib_server to lib_consensus * merge 15638: Move policy settings to new src/policy/settings unit * merge 15638: Move rpc utility methods to rpc/util * merge 15638: Move rpc rawtransaction util functions to rpc/rawtransaction_util.cpp * merge 15638: Move several units into common libraries * merge 15638: Move wallet load functions to wallet/load unit * merge 15638: Document src subdirectories and different libraries * [build] Add several util units (cleanup) * build: resolve missing declarations by re-specifying headers fix build errors fix build errors fix build errors

fanquake added the Refactoring label Mar 22, 2019

ryanofsky force-pushed the pr/link branch from 4207ce9 to 20edb8c Compare March 22, 2019 05:22

ryanofsky mentioned this pull request Mar 22, 2019

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

Merged

ryanofsky force-pushed the pr/link branch 2 times, most recently from c5b3a3f to 89061c8 Compare March 22, 2019 07:11

ryanofsky force-pushed the pr/link branch from 89061c8 to b35ba2c Compare March 22, 2019 07:34

ryanofsky mentioned this pull request Mar 22, 2019

Refactor: separate wallet from node #10973

Merged

lucayepa mentioned this pull request Mar 23, 2019

Code style PRs after v0.18 branch split #15465

Closed

l2a5b1 reviewed Mar 25, 2019

View reviewed changes

test/lint/lint-circular-dependencies.sh Outdated Show resolved Hide resolved

l2a5b1 reviewed Mar 25, 2019

View reviewed changes

test/lint/lint-circular-dependencies.sh Show resolved Hide resolved

maflcko mentioned this pull request Mar 25, 2019

Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency #13949

Closed

Sjors reviewed Mar 26, 2019

View reviewed changes

src/consensus/tx_check.h Show resolved Hide resolved

ryanofsky force-pushed the pr/link branch from b35ba2c to 9742ff8 Compare March 30, 2019 00:02

ryanofsky commented Mar 30, 2019

View reviewed changes

src/consensus/tx_check.h Show resolved Hide resolved

test/lint/lint-circular-dependencies.sh Outdated Show resolved Hide resolved

test/lint/lint-circular-dependencies.sh Show resolved Hide resolved

ryanofsky force-pushed the pr/link branch from 9742ff8 to fee746f Compare April 3, 2019 17:30

maflcko mentioned this pull request Apr 3, 2019

Library source code organization #15732

Closed

DrahtBot added the Needs rebase label Apr 6, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Move-only: Pull wallet code out of libbitcoin_server #15638

Move-only: Pull wallet code out of libbitcoin_server #15638

Uh oh!

Conversation

ryanofsky commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Empact commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 22, 2019

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Mar 25, 2019

Uh oh!

Sjors left a comment • 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!

Uh oh!

sipsorcery commented Mar 28, 2019

Uh oh!

Sjors commented Mar 29, 2019

Uh oh!

ryanofsky left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Apr 2, 2019

Uh oh!

jnewbery commented Apr 2, 2019

Uh oh!

jnewbery commented Apr 2, 2019

Uh oh!

Empact commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Apr 3, 2019

Uh oh!

ryanofsky commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ryanofsky commented Mar 22, 2019 •

edited

Loading

DrahtBot commented Mar 22, 2019 •

edited

Loading

Empact commented Mar 22, 2019 •

edited

Loading

ryanofsky commented Mar 22, 2019 •

edited

Loading

Sjors left a comment •

edited

Loading

ryanofsky left a comment •

edited

Loading

Empact commented Apr 3, 2019 •

edited

Loading

ryanofsky commented Apr 3, 2019 •

edited

Loading