Skip to content

Commit 011a895

Browse files
achow101achow101
committed
Merge bitcoin#29015: kernel: Streamline util library
c7376ba doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky) 4f74c59 util: Move util/string.h functions to util namespace (Ryan Ofsky) 4d05d3f util: add TransactionError includes and namespace declarations (Ryan Ofsky) 680eafd util: move fees.h and error.h to common/messages.h (Ryan Ofsky) 02e62c6 common: Add PSBTError enum (Ryan Ofsky) 0d44c44 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky) 9bcce26 util: move spanparsing.h to script/parsing.h (Ryan Ofsky) 6dd2ad4 util: move spanparsing.h Split functions to string.h (Ryan Ofsky) 23cc8dd util: move HexStr and HexDigit from util to crypto (TheCharlatan) 6861f95 util: move util/message to common/signmessage (Ryan Ofsky) cc5f29f build: move memory_cleanse from util to crypto (Ryan Ofsky) 5b93094 build: move chainparamsbase from util to common (Ryan Ofsky) ffa27af test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky) Pull request description: Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically: - Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity. - Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing. - Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library. Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of bitcoin#28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time. ACKs for top commit: achow101: ACK c7376ba TheCharlatan: Re-ACK c7376ba hebasto: re-ACK c7376ba. Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
2 parents a7bc9b7 + c7376ba commit 011a895

File tree

121 files changed

+1023
-526
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

121 files changed

+1023
-526
lines changed

‎contrib/devtools/check-deps.sh‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
#!/usr/bin/env bash
2+
3+
export LC_ALL=C
4+
set -Eeuo pipefail
5+
6+
# Declare paths to libraries
7+
declare -A LIBS
8+
LIBS[cli]="libbitcoin_cli.a"
9+
LIBS[common]="libbitcoin_common.a"
10+
LIBS[consensus]="libbitcoin_consensus.a"
11+
LIBS[crypto]="crypto/.libs/libbitcoin_crypto_base.a crypto/.libs/libbitcoin_crypto_x86_shani.a crypto/.libs/libbitcoin_crypto_sse41.a crypto/.libs/libbitcoin_crypto_avx2.a"
12+
LIBS[node]="libbitcoin_node.a"
13+
LIBS[util]="libbitcoin_util.a"
14+
LIBS[wallet]="libbitcoin_wallet.a"
15+
LIBS[wallet_tool]="libbitcoin_wallet_tool.a"
16+
17+
# Declare allowed dependencies "X Y" where X is allowed to depend on Y. This
18+
# list is taken from doc/design/libraries.md.
19+
ALLOWED_DEPENDENCIES=(
20+
"cli common"
21+
"cli util"
22+
"common consensus"
23+
"common crypto"
24+
"common util"
25+
"consensus crypto"
26+
"node common"
27+
"node consensus"
28+
"node crypto"
29+
"node kernel"
30+
"node util"
31+
"util crypto"
32+
"wallet common"
33+
"wallet crypto"
34+
"wallet util"
35+
"wallet_tool util"
36+
"wallet_tool wallet"
37+
)
38+
39+
# Add minor dependencies omitted from doc/design/libraries.md to keep the
40+
# dependency diagram simple.
41+
ALLOWED_DEPENDENCIES+=(
42+
"wallet consensus"
43+
"wallet_tool common"
44+
"wallet_tool crypto"
45+
)
46+
47+
# Declare list of known errors that should be suppressed.
48+
declare -A SUPPRESS
49+
# init.cpp file currently calls Berkeley DB sanity check function on startup, so
50+
# there is an undocumented dependency of the node library on the wallet library.
51+
SUPPRESS["libbitcoin_node_a-init.o libbitcoin_wallet_a-bdb.o _ZN6wallet27BerkeleyDatabaseSanityCheckEv"]=1
52+
# init/common.cpp file calls InitError and InitWarning from interface_ui which
53+
# is currently part of the node library. interface_ui should just be part of the
54+
# common library instead, and is moved in
55+
# https://github.com/bitcoin/bitcoin/issues/10102
56+
SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z11InitWarningRK13bilingual_str"]=1
57+
SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z9InitErrorRK13bilingual_str"]=1
58+
# rpc/external_signer.cpp adds defines node RPC methods but is built as part of the
59+
# common library. It should be moved to the node library instead.
60+
SUPPRESS["libbitcoin_common_a-external_signer.o libbitcoin_node_a-server.o _ZN9CRPCTable13appendCommandERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPK11CRPCCommand"]=1
61+
62+
usage() {
63+
echo "Usage: $(basename "${BASH_SOURCE[0]}") [BUILD_DIR]"
64+
}
65+
66+
# Output makefile targets, converting library .a paths to libtool .la targets
67+
lib_targets() {
68+
for lib in "${!LIBS[@]}"; do
69+
for lib_path in ${LIBS[$lib]}; do
70+
# shellcheck disable=SC2001
71+
sed 's:/.libs/\(.*\)\.a$:/\1.la:g' <<<"$lib_path"
72+
done
73+
done
74+
}
75+
76+
# Extract symbol names and object names and write to text files
77+
extract_symbols() {
78+
local temp_dir="$1"
79+
for lib in "${!LIBS[@]}"; do
80+
for lib_path in ${LIBS[$lib]}; do
81+
nm -o "$lib_path" | grep ' T ' | awk '{print $3, $1}' >> "${temp_dir}/${lib}_exports.txt"
82+
nm -o "$lib_path" | grep ' U ' | awk '{print $3, $1}' >> "${temp_dir}/${lib}_imports.txt"
83+
awk '{print $1}' "${temp_dir}/${lib}_exports.txt" | sort -u > "${temp_dir}/${lib}_exported_symbols.txt"
84+
awk '{print $1}' "${temp_dir}/${lib}_imports.txt" | sort -u > "${temp_dir}/${lib}_imported_symbols.txt"
85+
done
86+
done
87+
}
88+
89+
# Lookup object name(s) corresponding to symbol name in text file
90+
obj_names() {
91+
local symbol="$1"
92+
local txt_file="$2"
93+
sed -n "s/^$symbol [^:]\\+:\\([^:]\\+\\):[^:]*\$/\\1/p" "$txt_file" | sort -u
94+
}
95+
96+
# Iterate through libraries and find disallowed dependencies
97+
check_libraries() {
98+
local temp_dir="$1"
99+
local result=0
100+
for src in "${!LIBS[@]}"; do
101+
for dst in "${!LIBS[@]}"; do
102+
if [ "$src" != "$dst" ] && ! is_allowed "$src" "$dst"; then
103+
if ! check_disallowed "$src" "$dst" "$temp_dir"; then
104+
result=1
105+
fi
106+
fi
107+
done
108+
done
109+
check_not_suppressed
110+
return $result
111+
}
112+
113+
# Return whether src library is allowed to depend on dst.
114+
is_allowed() {
115+
local src="$1"
116+
local dst="$2"
117+
for allowed in "${ALLOWED_DEPENDENCIES[@]}"; do
118+
if [ "$src $dst" = "$allowed" ]; then
119+
return 0
120+
fi
121+
done
122+
return 1
123+
}
124+
125+
# Return whether src library imports any symbols from dst, assuming src is not
126+
# allowed to depend on dst.
127+
check_disallowed() {
128+
local src="$1"
129+
local dst="$2"
130+
local temp_dir="$3"
131+
local result=0
132+
133+
# Loop over symbol names exported by dst and imported by src
134+
while read symbol; do
135+
local dst_obj
136+
dst_obj=$(obj_names "$symbol" "${temp_dir}/${dst}_exports.txt")
137+
while read src_obj; do
138+
if ! check_suppress "$src_obj" "$dst_obj" "$symbol"; then
139+
echo "Error: $src_obj depends on $dst_obj symbol '$(c++filt "$symbol")', can suppess with:"
140+
echo " SUPPRESS[\"$src_obj $dst_obj $symbol\"]=1"
141+
result=1
142+
fi
143+
done < <(obj_names "$symbol" "${temp_dir}/${src}_imports.txt")
144+
done < <(comm -12 "${temp_dir}/${dst}_exported_symbols.txt" "${temp_dir}/${src}_imported_symbols.txt")
145+
return $result
146+
}
147+
148+
# Declare array to track errors which were suppressed.
149+
declare -A SUPPRESSED
150+
151+
# Return whether error should be suppressed and record suppresssion in
152+
# SUPPRESSED array.
153+
check_suppress() {
154+
local src_obj="$1"
155+
local dst_obj="$2"
156+
local symbol="$3"
157+
for suppress in "${!SUPPRESS[@]}"; do
158+
read suppress_src suppress_dst suppress_pattern <<<"$suppress"
159+
if [[ "$src_obj" == "$suppress_src" && "$dst_obj" == "$suppress_dst" && "$symbol" =~ $suppress_pattern ]]; then
160+
SUPPRESSED["$suppress"]=1
161+
return 0
162+
fi
163+
done
164+
return 1
165+
}
166+
167+
# Warn about error which were supposed to be suppress, but were not encountered.
168+
check_not_suppressed() {
169+
for suppress in "${!SUPPRESS[@]}"; do
170+
if [[ ! -v SUPPRESSED[$suppress] ]]; then
171+
echo >&2 "Warning: suppression '$suppress' was ignored, consider deleting."
172+
fi
173+
done
174+
}
175+
176+
# Check arguments.
177+
if [ "$#" = 0 ]; then
178+
BUILD_DIR="$(dirname "${BASH_SOURCE[0]}")/../../src"
179+
elif [ "$#" = 1 ]; then
180+
BUILD_DIR="$1"
181+
else
182+
echo >&2 "Error: wrong number of arguments."
183+
usage >&2
184+
exit 1
185+
fi
186+
if [ ! -f "$BUILD_DIR/Makefile" ]; then
187+
echo >&2 "Error: directory '$BUILD_DIR' does not contain a makefile, please specify path to build directory for library targets."
188+
usage >&2
189+
exit 1
190+
fi
191+
192+
# Build libraries and run checks.
193+
cd "$BUILD_DIR"
194+
# shellcheck disable=SC2046
195+
make -j"$(nproc)" $(lib_targets)
196+
TEMP_DIR="$(mktemp -d)"
197+
extract_symbols "$TEMP_DIR"
198+
if check_libraries "$TEMP_DIR"; then
199+
echo "Success! No unexpected dependencies were detected."
200+
else
201+
echo >&2 "Error: Unexpected dependencies were detected. Check previous output."
202+
fi
203+
rm -r "$TEMP_DIR"

‎doc/design/libraries.md‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
| *libbitcoin_cli* | RPC client functionality used by *bitcoin-cli* executable |
66
| *libbitcoin_common* | Home for common functionality shared by different executables and libraries. Similar to *libbitcoin_util*, but higher-level (see [Dependencies](#dependencies)). |
77
| *libbitcoin_consensus* | Stable, backwards-compatible consensus functionality used by *libbitcoin_node* and *libbitcoin_wallet*. |
8+
| *libbitcoin_crypto* | Hardware-optimized functions for data encryption, hashing, message authentication, and key derivation. |
89
| *libbitcoin_kernel* | Consensus engine and support library used for validation by *libbitcoin_node*. |
910
| *libbitcoinqt* | GUI functionality used by *bitcoin-qt* and *bitcoin-gui* executables. |
1011
| *libbitcoin_ipc* | IPC functionality used by *bitcoin-node*, *bitcoin-wallet*, *bitcoin-gui* executables to communicate when [`--enable-multiprocess`](multiprocess.md) is used. |
@@ -53,21 +54,29 @@ bitcoin-wallet[bitcoin-wallet]-->libbitcoin_wallet_tool;
5354
libbitcoin_cli-->libbitcoin_util;
5455
libbitcoin_cli-->libbitcoin_common;
5556
57+
libbitcoin_consensus-->libbitcoin_crypto;
58+
5659
libbitcoin_common-->libbitcoin_consensus;
60+
libbitcoin_common-->libbitcoin_crypto;
5761
libbitcoin_common-->libbitcoin_util;
5862
5963
libbitcoin_kernel-->libbitcoin_consensus;
64+
libbitcoin_kernel-->libbitcoin_crypto;
6065
libbitcoin_kernel-->libbitcoin_util;
6166
6267
libbitcoin_node-->libbitcoin_consensus;
68+
libbitcoin_node-->libbitcoin_crypto;
6369
libbitcoin_node-->libbitcoin_kernel;
6470
libbitcoin_node-->libbitcoin_common;
6571
libbitcoin_node-->libbitcoin_util;
6672
6773
libbitcoinqt-->libbitcoin_common;
6874
libbitcoinqt-->libbitcoin_util;
6975
76+
libbitcoin_util-->libbitcoin_crypto;
77+
7078
libbitcoin_wallet-->libbitcoin_common;
79+
libbitcoin_wallet-->libbitcoin_crypto;
7180
libbitcoin_wallet-->libbitcoin_util;
7281
7382
libbitcoin_wallet_tool-->libbitcoin_wallet;
@@ -78,22 +87,23 @@ class bitcoin-qt,bitcoind,bitcoin-cli,bitcoin-wallet bold
7887
```
7988
</td></tr><tr><td>
8089

81-
**Dependency graph**. Arrows show linker symbol dependencies. *Consensus* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus and util.
90+
**Dependency graph**. Arrows show linker symbol dependencies. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto, and util.
8291

8392
</td></tr></table>
8493

8594
- The graph shows what _linker symbols_ (functions and variables) from each library other libraries can call and reference directly, but it is not a call graph. For example, there is no arrow connecting *libbitcoin_wallet* and *libbitcoin_node* libraries, because these libraries are intended to be modular and not depend on each other's internal implementation details. But wallet code is still able to call node code indirectly through the `interfaces::Chain` abstract class in [`interfaces/chain.h`](../../src/interfaces/chain.h) and node code calls wallet code through the `interfaces::ChainClient` and `interfaces::Chain::Notifications` abstract classes in the same file. In general, defining abstract classes in [`src/interfaces/`](../../src/interfaces/) can be a convenient way of avoiding unwanted direct dependencies or circular dependencies between libraries.
8695

87-
- *libbitcoin_consensus* should be a standalone dependency that any library can depend on, and it should not depend on any other libraries itself.
96+
- *libbitcoin_crypto* should be a standalone dependency that any library can depend on, and it should not depend on any other libraries itself.
8897

89-
- *libbitcoin_util* should also be a standalone dependency that any library can depend on, and it should not depend on other internal libraries.
98+
- *libbitcoin_consensus* should only depend on *libbitcoin_crypto*, and all other libraries besides *libbitcoin_crypto* should be allowed to depend on it.
9099

91-
- *libbitcoin_common* should serve a similar function as *libbitcoin_util* and be a place for miscellaneous code used by various daemon, GUI, and CLI applications and libraries to live. It should not depend on anything other than *libbitcoin_util* and *libbitcoin_consensus*. The boundary between _util_ and _common_ is a little fuzzy but historically _util_ has been used for more generic, lower-level things like parsing hex, and _common_ has been used for bitcoin-specific, higher-level things like parsing base58. The difference between util and common is mostly important because *libbitcoin_kernel* is not supposed to depend on *libbitcoin_common*, only *libbitcoin_util*. In general, if it is ever unclear whether it is better to add code to *util* or *common*, it is probably better to add it to *common* unless it is very generically useful or useful particularly to include in the kernel.
100+
- *libbitcoin_util* should be a standalone dependency that any library can depend on, and it should not depend on other libraries except *libbitcoin_crypto*. It provides basic utilities that fill in gaps in the C++ standard library and provide lightweight abstractions over platform-specific features. Since the util library is distributed with the kernel and is usable by kernel applications, it shouldn't contain functions that external code shouldn't call, like higher level code targetted at the node or wallet. (*libbitcoin_common* is a better place for higher level code, or code that is meant to be used by internal applications only.)
92101

102+
- *libbitcoin_common* is a home for miscellaneous shared code used by different Bitcoin Core applications. It should not depend on anything other than *libbitcoin_util*, *libbitcoin_consensus*, and *libbitcoin_crypto*.
93103

94-
- *libbitcoin_kernel* should only depend on *libbitcoin_util* and *libbitcoin_consensus*.
104+
- *libbitcoin_kernel* should only depend on *libbitcoin_util*, *libbitcoin_consensus*, and *libbitcoin_crypto*.
95105

96-
- The only thing that should depend on *libbitcoin_kernel* internally should be *libbitcoin_node*. GUI and wallet libraries *libbitcoinqt* and *libbitcoin_wallet* in particular should not depend on *libbitcoin_kernel* and the unneeded functionality it would pull in, like block validation. To the extent that GUI and wallet code need scripting and signing functionality, they should be get able it from *libbitcoin_consensus*, *libbitcoin_common*, and *libbitcoin_util*, instead of *libbitcoin_kernel*.
106+
- The only thing that should depend on *libbitcoin_kernel* internally should be *libbitcoin_node*. GUI and wallet libraries *libbitcoinqt* and *libbitcoin_wallet* in particular should not depend on *libbitcoin_kernel* and the unneeded functionality it would pull in, like block validation. To the extent that GUI and wallet code need scripting and signing functionality, they should be get able it from *libbitcoin_consensus*, *libbitcoin_common*, *libbitcoin_crypto*, and *libbitcoin_util*, instead of *libbitcoin_kernel*.
97107

98108
- GUI, node, and wallet code internal implementations should all be independent of each other, and the *libbitcoinqt*, *libbitcoin_node*, *libbitcoin_wallet* libraries should never reference each other's symbols. They should only call each other through [`src/interfaces/`](../../src/interfaces/) abstract interfaces.
99109

0 commit comments

Comments
 (0)