This has never worked with any of the mingw-w64 compilers we use, and
the -O0 is causing issues for builders applying spectre mitigations.
Recent discussion on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90458
also indicates that this should just not be used on Windows.
e017a913d0 bitcoind: Add -daemonwait option to wait for initialization (Wladimir J. van der Laan)
c3e6fdee6d shutdown: Use RAII TokenPipe in shutdown (Wladimir J. van der Laan)
612f746a8f util: Add RAII TokenPipe (Wladimir J. van der Laan)
Pull request description:
This adds a `-daemonwait` flag that does the same as `-daemon` except that it, from a user perspective, backgrounds the process only after initialization is complete. This is similar to the behaviour of some other software such as c-lightning.
This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.
The use of the libc function `daemon()` is replaced by a custom implementation which is inspired by the [glibc implementation](https://github.com/lattera/glibc/blob/master/misc/daemon.c#L44), but which also creates a pipe from the child to the parent process for communication.
An additional advantage of having our own `daemon()` implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.
TODO:
- [x] Factor out `token_read` and `token_write` to an utility, and use them in `shutdown.cpp` as well—this is exactly the same kind of communication mechanism.
- [x] RAII-ify pipe endpoints.
- [x] Improve granularity of the `configure.ac` checks. This currently still checks for the function `daemon()` which makes no sense as it's not used. It should check for individual functions such as
`fork()` and `setsid()` etc—the former being required, the second optional.
- [-] ~~Signal propagation during initialization: if say, pressing Ctrl-C during `-daemonwait` it would be good to pass this SIGINT on to the child process instead of detaching the parent process and letting the child run free.~~ This is not necessary, see https://github.com/bitcoin/bitcoin/pull/21007#issuecomment-769007341.
Future:
- Consider if it makes sense to use this in the RPC tests (there would be no more need for "is RPC ready" polling loops). I think this is out of scope for this PR.
ACKs for top commit:
jonatack:
Tested ACK e017a913d0 checked change since previous review is move-only
Tree-SHA512: 53369b8ca2247e4cf3af8cb2cfd5b3399e8e0e3296423d64be987004758162a7ddc1287b01a92d7692328edcb2da4cf05d279b1b4ef61a665b71440ab6a6dbe2
This adds a `-daemonwait` flag that does the same as `-daemon` except
it, from a user perspective, backgrounds the process only after
initialization is complete.
This can be useful when the process launching bitcoind wants to
guarantee that either the RPC server is running, or that initialization
failed, before continuing. The exit code indicates the initialization
result.
This replaces the use of the libc function `daemon()` by a custom
implementation which is inspired by the glibc implementation, but also
creates a pipe from the child to the parent process for communication.
An additional advantage of having our own `daemon()` implementation is
that no MACOS-specific pragmas are needed anymore to silence a
deprecation warning.
faa06ecc9c build: Bump minimum Qt version to 5.9.5 (Hennadii Stepanov)
Pull request description:
Close#20104.
ACKs for top commit:
laanwj:
Code review ACK faa06ecc9c
jarolrod:
ACK faa06ecc9c
fanquake:
ACK faa06ecc9c - this should be ok to do now.
Tree-SHA512: 7295472b5fd37ffb30f044e88c39d375a5a5187d3f2d44d4e73d0eb0c7fd923cf9949c2ddab6cddd8c5da7e375fff38112b6ea9779da4fecce6f024d05ba9c08
fae216a73d scripted-diff: Rename MakeFuzzingContext to MakeNoLogFileContext (MarcoFalke)
fa4fbec03e scripted-diff: Rename PROVIDE_MAIN_FUNCTION -> PROVIDE_FUZZ_MAIN_FUNCTION (MarcoFalke)
Pull request description:
Split out two renames from #21003:
* `PROVIDE_FUZZ_MAIN_FUNCTION`. *Reason*: This in only used by fuzzing, so the name should indicate that.
* `MakeNoLogFileContext`. *Reason*: Better reflects what the helper does. Also, prepares it to be used in non-fuzz tests in the future.
ACKs for top commit:
practicalswift:
cr ACK fae216a73d71441432cf07212313d18771ce7deb: scripted-diff looks correct
Tree-SHA512: e5d347746f5da72b0c86fd4f07ac2e4b3016e88e8c97a830c73bd79d0af6d0245fe7712487fc20344d6cc25958941716c1678124a123930407e3a437265b71df
9bac71350d build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) (Sebastian Falbesoner)
584fd91d2d init: only use pipe2 if availabile, check in configure (Sebastian Falbesoner)
Pull request description:
The result of the O_CLOEXEC availability check is currently only set in the Makefile and passed to LevelDB (see `LEVELDB_CPPFLAGS_INT` in `src/Makefile.leveldb.include`), but not defined to be used in our codebase. This means that code within the preprocessor conditional `#if HAVE_O_CLOEXEC` was actually never compiled. On the master branch this is currently used for pipe creation in `src/shutdown.cpp`, PR #21007 moves this part to a new module (I found the issue while testing that PR).
The fix is similar to the one in #19803, which solved the same problem for HAVE_FDATASYNC.
In the course of working on the PR it turned out that pipe2 is not available an all platforms, hence a configure check and a corresponding define HAVE_PIPE2 is introduced and used.
The PR can be tested by anyone with a system that has pipe2 and O_CLOEXEC available by putting gibberish into the HAVE_O_CLOEXEC block: on master, everything should compile fine, on PR, the compiler should abort with an error. At least that's my naive way of testing preprocessor logic, happy to hear more sophisticated ways :-)
ACKs for top commit:
laanwj:
Code review ACK 9bac71350d
Tree-SHA512: aec89faf6ba52b6f014c610ebef7b725d9e967207d58b42a4a71afc9f1268fcb673ecc85b33a2a3debba8105a304dd7edaba4208c5373fcef2ab83e48a170051
This option replaces --with-boost-process
This prepares external signer support to be disabled by default.
It adds a configure option to enable this feature and to check
if Boost::Process is present.
This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini
c5da2749e2 build: actually stop configure if Boost isn't available (fanquake)
cad8b527ea build: explicitly install libboost-dev package (fanquake)
Pull request description:
If Boost is not found via AX_BOOST_BASE, we don't actually stop
configuring, only a warning is emitted:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version MINIMUM_REQUIRED_BOOST or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
```
Instead we usually fail when one of the other AX_BOOST_* macros fails to find a library. These macros are slowly being
removed, and in any case, it makes more sense to fail earlier if Boost is missing.
If Boost is unavailable, the failure now looks like:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version 1.58.0 or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
configure: error: Boost is not available!
```
Note that we now just pass the version into AX_BOOST_BASE, which fixes it's display in the output (rather than showing `MINIMUM_REQUIRED_BOOST`).
This PR also has a commit that adds `libboost-dev` to our install instructions and CI. This package is currently installed as a side-effect of installing our other libboost-*-dev packages. However as those continue to disappear, it makes sense to install boost-dev explicitly.
ACKs for top commit:
laanwj:
Code review ACK c5da2749e2
MarcoFalke:
Concept ACK c5da2749e2
Tree-SHA512: f866062f9d7d3a2316b6c887f17c664b9cfff41fdc0cb99ca79d641240fb01a5ae0d34140e515bc465219e1b43d5ca84f7c55f48b9c5b45a80ff2795dafd072b
If Boost is not found via AX_BOOST_BASE, we don't actually stop
configuring, only a warning is emitted:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version MINIMUM_REQUIRED_BOOST or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
```
Instead we would usually fail when one of the other
AX_BOOST_* macros fails to find a library. These macros are slowly being
removed, and in any case, it makes more sense to fail earlier if Boost
is missing.
If Boost is unavailable, the failure now looks like:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version 1.58.0 or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
configure: error: Boost is not available!
```
Note that we now just pass the version into AX_BOOST_BASE, which fixes
it's display in the output (rather than MINIMUM_REQUIRED_BOOST).
Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate
(#15382 (929cda5470))?
The macro also has the side-effect of producing confusing error
messages. i.e in #20744, the CI is currently failing with:
```bash
checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::Process library is available... yes
configure: error: Could not find a version of the Boost::Process library!
```
This isn't useful, given there is no such thing as a `Boost::Process`
library.
This PR just removes the macro entirely, but maintains a `--with-boost-process`
(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of `--enable-disable-external-signer` if/when #16546
is merged.
060a2a64d4 ci: remove boost thread installation (fanquake)
06e1d7d81d build: don't build or use Boost Thread (fanquake)
7097add83c refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981ef8 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)
Pull request description:
This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).
Even though [some concerns were raised](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.
Two other points:
[Cory mentioned in #21022](https://github.com/bitcoin/bitcoin/pull/21022#issuecomment-769274179):
> It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.
Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
* The version of Boost.
* The platform you're building for.
* Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
* Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](https://github.com/boostorg/thread/issues/230#issuecomment-475937761) version of `shared_mutex`.
A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.
With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.
Previous similar PRs were #19183 & #20922. The authors are included in the commits here.
Also related to #21022 - pthread sanity checking.
ACKs for top commit:
laanwj:
Code review ACK 060a2a64d4
vasild:
ACK 060a2a64d4
Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
We are already testing for this, and our test works correctly with a Darwin
target, where the macro does not. Darwin targets do not support "protected"
visibility.
32cbb06676 build: build fuzz tests by default. (Dan Benjamin)
Pull request description:
This fixes issue #19388. The changes are as follows:
- Add a new flag to configure, --enable-fuzz-binary, which allows building test/fuzz/fuzz regardless of whether we are building to do actual fuzzing
- Set -DPROVIDE_MAIN_FUNCTION whenever --enable-fuzz is no
- Add the following libraries to FUZZ_SUITE_LD_COMMON:
- LIBBITCOIN_WALLET
- SQLLITE_LIBS
- BDB_LIBS
- if necessary, some or all of:
- NATPMP_LIBS
- MINIUPNPC_LIBS
- LIBBITCOIN_ZMQ / ZMQ_LIBS
Fixes #19388
ACKs for top commit:
MarcoFalke:
review ACK 32cbb06676📭
Tree-SHA512: c91d713ffe54a3d055daaec02c4317d7e13eed6688821ddc10d894224950b18e276fbdd4acc758c7103b50f34a132b1882b68bc8b60409f97438e0759ced77e1
e9189a750b build: more robustly check for fcf-protection support (fanquake)
Pull request description:
When using Clang 7, we may end up trying to use the flag when it won't
work properly, which can lead to confusing errors. i.e:
```bash
/usr/bin/ld: error: ... <corrupt x86 feature size: 0x8>
```
Use `AX_CHECK_LINK_FLAG` & `--fatal-warnings` to ensure we wont use the flag in this case.
We do this as even when the error is emitted, compilation succeeds, and the binaries produced will run. This means we can't just check if the compiler accepts the flag, or if compilation succeeds (without or without `-Werror`, and/or passing `-Wl,--fatal-warnings`, which may not be passed through to the linker).
This was reported by someone configuring for fuzzing, on Debian 10, where Clang 7 is the default.
See here for a minimal example of the problematic behaviour:
https://gist.github.com/fanquake/9b33555fcfebef8eb8c0795a71732bc6
ACKs for top commit:
pstratem:
tested ACK e9189a750b
MarcoFalke:
not an ACK e9189a750b , I only tested configure on my system (gcc-10, clang-11):
hebasto:
ACK e9189a750b, tested with clang-7, clang-10 and gcc: the `-fcf-protection=full` is not applied for clang-7, but applied for others compilers.
Tree-SHA512: ec24b0cc5523b90139c96cbb33bb98d1e6a24d858c466aa7dfb3c474caf8c50aca53e570fdbc0ff88378406b0ac5d687542452637b1b5fa062e829291b886fc1
This fixes issue #19388. The changes are as follows:
- Add a new flag to configure, --enable-fuzz-binary, which allows building test/fuzz/fuzz regardless of whether we are building to do actual fuzzing
- Set -DPROVIDE_MAIN_FUNCTION whenever --enable-fuzz is no
- Add the following libraries to FUZZ_SUITE_LD_COMMON:
- LIBBITCOIN_WALLET
- SQLLITE_LIBS
- BDB_LIBS
- if necessary, some or all of:
- NATPMP_LIBS
- MINIUPNPC_LIBS
- LIBBITCOIN_ZMQ / ZMQ_LIBS
22eb7930a6 tracing: add tracing framework (William Casarin)
933ab8a720 build: detect sys/sdt.h for eBPF tracing (William Casarin)
Pull request description:
Instead of writing ad-hoc logging everywhere (eg: #19509), we can take advantage of linux user static defined traces, aka. USDTs ( not the stablecoin 😅 )
The linux kernel can hook into these tracepoints at runtime, but otherwise they have little to no performance impact. Traces can pass data which can be printed externally via tools such as bpftrace. For example, here's one that prints incoming and outgoing network messages:
# Examples
## Network Messages
```
#!/usr/bin/env bpftrace
BEGIN
{
printf("bitcoin net msgs\n");
@start = nsecs;
}
usdt:./src/bitcoind:net:push_message
{
$ip = str(arg0);
$peer_id = (int64)arg1;
$command = str(arg2);
$data_len = arg3;
$data = buf(arg3,arg4);
$t = (nsecs - @start) / 100000;
printf("%zu outbound %s %s %zu %d %r\n", $t, $command, $ip, $peer_id, $data_len, $data);
@outbound[$command]++;
}
usdt:./src/bitcoind:net:process_message
{
$ip = str(arg0);
$peer_id = (int64)arg1;
$command = str(arg2);
$data_len = arg3;
$data = buf(arg3,arg4);
$t = (nsecs - @start) / 100000;
printf("%zu inbound %s %s %zu %d %r\n", $t, $command, $ip, $peer_id, $data_len, $data);
@inbound[$ip, $command]++;
}
```
$ sudo bpftrace netmsg.bt
output: https://jb55.com/s/b11312484b601fb3.txt
if you look at the bottom of the output you can see a histogram of all the messages grouped by message type and IP. nice!
## IBD Benchmarking
```
#!/usr/bin/env bpftrace
BEGIN
{
printf("IBD to 500,000 bench\n");
}
usdt:./src/bitcoind:CChainState:ConnectBlock
{
$height = (uint32)arg0;
if ($height == 1) {
printf("block 1 found, starting benchmark\n");
@start = nsecs;
}
if ($height >= 500000) {
@end = nsecs;
@duration = @end - @start;
exit();
}
}
END {
printf("duration %d ms\n", @duration / 1000000)
}
```
This one hooks into ConnectBlock and prints the IBD time to height 500,000 starting from the first call to ConnectBlock
Userspace static tracepoints give lots of flexibility without invasive logging code. It's also more flexible than ad-hoc logging code, allowing you to instrument many different aspects of the system without having to enable per-subsystem logging.
Other ideas: tracepoints for lock contention, threads, what else?
Let me know what ya'll think and if this is worth adding to bitcoin.
## TODO
- [ ] docs?
- [x] Integrate systemtap-std-dev/libsystemtap into build (provides the <sys/sdt.h> header)
- [x] ~dtrace macos support? (is this still a thing?)~ going to focus on linux for now
ACKs for top commit:
laanwj:
Tested ACK 22eb7930a6
0xB10C:
Tested ACK 22eb7930a6
Tree-SHA512: 69242242112b679c8a12a22b3bc50252c305894fb3055ae6e13d5f56221d858e58af1d698af55e23b69bdb7abedb5565ac6b45fa5144087b77a17acd04646a75
595a34dbea contrib/signet: Document miner script in README.md (Anthony Towns)
ff7dbdc08a contrib/signet: Add script for generating a signet chain (Anthony Towns)
13762bcc96 Add bitcoin-util command line utility (Anthony Towns)
95d5d5e625 rpc: allow getblocktemplate for test chains when unconnected or in IBD (Anthony Towns)
81c54dec20 rpc: update getblocktemplate with signet rule, include signet_challenge (Anthony Towns)
Pull request description:
Adds `contrib/signet/miner` for mining signet blocks.
Adds `bitcoin-util` cli utility, with the idea being it can provide bitcoin related functionality that does not rely on the ability to access a running node. Only subcommand currently is "grind" which takes a hex-encoded header and grinds its nonce until its nBits is satisfied.
Updates `getblocktemplate` to include `signet_challenge` field, and makes `getblocktemplate` require the signet rule when invoked on the signet change. Removes connectivity and IBD checks from `getblocktemplate` when applied to a test chain (regtest, testnet, signet).
ACKs for top commit:
laanwj:
code review ACK 595a34dbea
Tree-SHA512: 8d43297710fdc1edc58acd9b53e1bd1671e5724f7097b40ab73653715dc8becc70534c4496cbba9290f4dd6538a7a3d5830eb85f83391ea31a3bb5b9d3378cc3
a191e23b8e doc: Add release notes (Hennadii Stepanov)
ae749d12dd doc: Add libnatpmp stuff (Hennadii Stepanov)
e28f9be87a ci: Add libnatpmp-dev package to some builds (Hennadii Stepanov)
5a0185b6c9 gui: Add NAT-PMP network option (Hennadii Stepanov)
a39f7336a3 net: Add -natpmp command line option (Hennadii Stepanov)
28acffd9d5 net: Add NAT-PMP to port mapping loop (Hennadii Stepanov)
a8d9f275d0 net: Add libnatpmp support (Hennadii Stepanov)
58e8364dcd gui: Apply port mapping changes on dialog exit (Hennadii Stepanov)
cf151cc68c scripted-diff: Rename UPnP stuff (Hennadii Stepanov)
4e91b1e24d net: Add flags for port mapping protocols (Hennadii Stepanov)
8b50d1b5bb net: Keep trying to use UPnP when -upnp=1 (Hennadii Stepanov)
28e2961fd6 refactor: Replace magic number with named constant (Hennadii Stepanov)
02ccf69dd6 refactor: Move port mapping code to its own module (Hennadii Stepanov)
Pull request description:
Close#11902
This PR is an alternative to:
- #12288
- #15717
To compile with NAT-PMP support on Ubuntu [`libnatpmp-dev`](https://packages.ubuntu.com/source/bionic/libnatpmp) should be available.
Log excerpt:
```
2020-02-05T20:12:28Z [mapport] NAT-PMP: public address = 95.164.65.194
2020-02-05T20:12:28Z [mapport] AddLocal(95.164.65.194:18333,3)
2020-02-05T20:12:28Z [mapport] NAT-PMP: port mapping successful.
```
See: [`libnatpmp`](https://miniupnp.tuxfamily.org/libnatpmp.html)
---
Some follow-ups are out of this PR's scope:
- mention NAT-PMP library in the version message
- ~integrate NAT-PMP into the GUI~ (already [added](https://github.com/bitcoin/bitcoin/pull/18077#issuecomment-589405068))
ACKs for top commit:
laanwj:
Tested and code review ACK a191e23b8e
Tree-SHA512: 10e19267c21bf30f20ff1abfc882d526049f0e790b95e12f109dc2bed7c0aef45de03eaf967f4e667e7509be04f1873a5c508087393d947205f3aab2ad6d7cf1
9815332d51 test: Change MuHash Python implementation to match cpp version again (Fabian Jahr)
01297fb3ca fuzz: Add MuHash consistency fuzz test (Fabian Jahr)
b111410914 test: Add MuHash3072 fuzz test (Fabian Jahr)
c122527385 bench: Add Muhash benchmarks (Fabian Jahr)
7b1242229d test: Add MuHash3072 unit tests (Fabian Jahr)
adc708c98d crypto: Add MuHash3072 implementation (Fabian Jahr)
0b4d290bf5 crypto: Add Num3072 implementation (Fabian Jahr)
589f958662 build: Check for 128 bit integer support (Fabian Jahr)
Pull request description:
This is the first split of #18000 which implements the Muhash algorithm and uses it to calculate the UTXO set hash in `gettxoutsetinfo`.
ACKs for top commit:
laanwj:
Code review ACK 9815332d51
Tree-SHA512: 4bc090738f0e3d80b74bdd8122e24a8ce80121120fd37c7e4335a73e7ba4fcd7643f2a2d559e2eebf54b8e3a3bd5f12cfb27ba61ded135fda210a07a233eae45
819d03b932 refactor: took out unused member functions (Zero)
ed69213c2b build: enable unused member function diagnostic (Zero)
Pull request description:
This PR enables the `-Wunused-member-function` compiler diagnostic, as discussed in #19702.
> **Notice**: The `unused-member-function` diagnostic is only available on clang. Therefore, clang should be used to test this PR.
- [x] Include the `-Wunused-member-function`diagnostic in `./configure.ac`. (ed69213c2b)
- [x] Resolve the reported warnings. (819d03b932)
Currently, enabling this flag no longer reports the following warnings:
> **Note**: output from `make 2>&1 | grep "warning: unused member function" | sort | uniq -c`
```
1 index/blockfilterindex.cpp:54:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function]
2 script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function]
1 test/util_tests.cpp:1975:14: warning: unused member function 'operator=' [-Wunused-member-function]
```
All tests have passed locally (from `make check` & `src/test/test_bitcoin`).
This PR closes#19702.
ACKs for top commit:
practicalswift:
ACK 819d03b932 - patch still looks correct :)
MarcoFalke:
ACK 819d03b932
pox:
Tested ACK 819d03b932 with clang after `make clean`. No unused member function warnings.
theStack:
tested ACK 819d03b932
Tree-SHA512: 5fdfbbb02b3dc618a90a874a5caa5e01e596fc1d14a209e75a6981f01b253f9bca0cfac8fdd758dd7151986609fb76571c3745124a29cfd4f8cbb8d82a07272e
When using Clang 7, we may end up trying to use the flag when it won't
work properly, which can lead to confusing errors. i.e:
```bash
/usr/bin/ld: error: ... <corrupt x86 feature size: 0x8>
```
Use CHECK_LINK_FLAG & --fatal-warnings to ensure we wont use the flag in this case.
a0a771843f contrib: Changes to checks for PowerPC64 (Luke Dashjr)
634f6ec4eb contrib: Parse ELF directly for symbol and security checks (Wladimir J. van der Laan)
Pull request description:
Instead of the ever-messier text parsing of the output of the readelf tool (which is clearly meant for human consumption not to be machine parseable), parse the ELF binaries directly.
Add a small dependency-less ELF parser specific to the checks.
This is slightly more secure, too, because it removes potential ambiguity due to misparsing and changes in the output format of `elfread`. It also allows for stricter and more specific ELF format checks in the future.
This removes the build-time dependency for `readelf`.
It passes the test-security-check for me locally, ~~though I haven't checked on all platforms~~. I've checked that this works on the cross-compile output for all ELF platforms supported by Bitcoin Core at the moment, as well as PPC64 LE and BE.
Top commit has no ACKs.
Tree-SHA512: 7f9241fec83ee512642fecf5afd90546964561efd8c8c0f99826dcf6660604a4db2b7255e1afb1e9bb0211fd06f5dbad18a6175dfc03e39761a40025118e7bfc
7587d11ec9 build: remove cdrkit package from depends (fanquake)
0df9819126 build: Replace genisoimage with xorriso (fanquake)
22437fc72e build: Run libdmg-hfsplus's DMG tool in make deploy (Carl Dong)
Pull request description:
This is a redo of fanquake's https://github.com/bitcoin/bitcoin/pull/18151, which, aside from switching us from the deprecated `genisoimage` to the maintained `xorriso`, is also necessary for Guix to achieve determinism without using faketime.
> xorriso and its mkisofs/genisoimage emulation alter-ego xorrisofs are
> more maintained, and has the right toggles for us to achieve output
> determinism without using blunt tools like faketime.
>
> In this commit, we use xorrisofs from the build environment rather than
> building it ourselves using depends. This is not necessary and can be
> changed in the future.
>
> From wiki.debian.org/genisoimage?action=recall&rev=11 :
>
> > The classical command line interface for production of ISO 9660
> > filesystem images is the option set established by program mkisofs.
> > For reasons of licensing and other problems with its author, Debian
> > ships a fork of mkisofs, called genisoimage, which was split off in
> > 2006 and then developed independently.
> >
> > Meanwhile, genisoimage gets no new features and not even bug fixes. It
> > is first choice only if its options -udf or -hfs are needed.
> >
> > Replacement in most uses cases, especially for bootable ISO 9660
> > filesystems, archiving, and backup, is xorrisofs which starts the -as
> > mkisofs emulation mode of program xorriso.
ACKs for top commit:
laanwj:
ACK 7587d11ec9
Tree-SHA512: 62f3aad08fa8bf21192e951d7dd33b24975586d76834cfa3498f4b8cdb586cefec8cab2c073d1951a0884b5e182fd71ef2cf3accad98f84455016776ad3c5422
xorriso and its mkisofs/genisoimage emulation alter-ego xorrisofs are
more maintained, and has the right toggles for us to achieve output
determinism without using blunt tools like faketime.
In this commit, we use xorrisofs from the build environment rather than
building it ourselves using depends. This is not necessary and can be
changed in the future.
From https://wiki.debian.org/genisoimage?action=recall&rev=11 :
> The classical command line interface for production of ISO 9660
> filesystem images is the option set established by program mkisofs.
> For reasons of licensing and other problems with its author, Debian
> ships a fork of mkisofs, called genisoimage, which was split off in
> 2006 and then developed independently.
>
> Meanwhile, genisoimage gets no new features and not even bug fixes. It
> is first choice only if its options -udf or -hfs are needed.
>
> Replacement in most uses cases, especially for bootable ISO 9660
> filesystems, archiving, and backup, is xorrisofs which starts the -as
> mkisofs emulation mode of program xorriso.
fa13e1b0c5 build: Add option --enable-danger-fuzz-link-all (MarcoFalke)
44444ba759 fuzz: Link all targets once (MarcoFalke)
Pull request description:
Currently the linker is invoked more than 150 times when compiling with `--enable-fuzz`. This is problematic for several reasons:
* It wastes disk space north of 20 GB, as all libraries and sanitizers are linked more than 150 times
* It wastes CPU time, as the link step can practically not be cached (similar to ccache for object files)
* It makes it a blocker to compile the fuzz tests by default for non-fuzz builds #19388, for the aforementioned reasons
* The build file is several thousand lines of code, without doing anything meaningful except listing each fuzz target in a highly verbose manner
* It makes writing new fuzz tests unnecessarily hard, as build system knowledge is required; Compare that to boost unit tests, which can be added by simply editing an existing cpp file
* It encourages fuzz tests that re-use the `buffer` or assume the `buffer` to be concatenations of seeds, which increases complexity of seeds and complexity for the fuzz engine to explore; Thus reducing the effectiveness of the affected fuzz targets
Fixes#20088
ACKs for top commit:
practicalswift:
Tested ACK fa13e1b0c5
sipa:
ACK fa13e1b0c5. Reviewed the code changes, and tested the 3 different test_runner.py modes (run once, merge, generate). I also tested building with the new --enable-danger-fuzz-link-all
Tree-SHA512: 962ab33269ebd51810924c51266ecc62edd6ddf2fcd9a8c359ed906766f58c3f73c223f8d3cc49f2c60f0053f65e8bdd86ce9c19e673f8c2b3cd676e913f2642
836a3dc02c Avoid weak-linked getauxval support on non-linux platforms (like macOS) (Jonas Schnelli)
41a413b317 Define correct symbols for getauxval (Jonas Schnelli)
Pull request description:
PR #20358 made use of the two preprocessor symbols `HAVE_STRONG_GETAUXVAL` as well as `HAVE_WEAK_GETAUXVAL`.
These symbols have not been defined in configure.ac. They where only passed selective as CRC32 CPPFLAGS in https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include#L16.
PR #20358 would have broken the macOS build since `getauxval` is not supported on macOS (but weak-linking does pass).
This PR defines the two symbols correctly and reduces calls to `getauxval` to linux.
ACKs for top commit:
laanwj:
Code review ACK 836a3dc02c
jonatack:
utACK 836a3dc02c
Tree-SHA512: 6527f4a617b937f4c368a3cb1c162f1ac38a6f5e6341295554961eaf322906e9b27398a6f7b00819854ceebb5c828d3e6ce0a779edd769adc4053ce8beda3739
Previously, the compression of the .iso file to a .dmg file was done
outside of `make deploy' in order to use the faketime-wrapped version of
libdmg-hfsplus's DMG tool.
Specifying the faketime-wrapped version of the DMG tool to ./configure
fixes this and simplifies build scripts.
c932e0d67e doc: Update wallet database installation guide for macOS (Hennadii Stepanov)
ee7b84e63c build: Use Homebrew's sqlite package if it is available (Hennadii Stepanov)
c96d1f65a5 build, refactor: Check that Homebrew's qt5 package is actually installed (Hennadii Stepanov)
Pull request description:
On master (7ae86b3c68) installed Homebrew `sqlite` package is ignored during build on macOS.
This PR fixes this issue and update macOS build docs.
Closes#20498.
ACKs for top commit:
willcl-ark:
> > That said, another tACK of [c932e0d](c932e0d67e)
hebasto:
> That said, another tACK of [c932e0d](c932e0d67e)
laanwj:
Code review ACK c932e0d67e
jonasschnelli:
code review re-ACK c932e0d67e
Tree-SHA512: 2563f25534d065556b17ee8c0fca957aea61b5ae288a2aa72743e77607843a45c39f209321e0f05b34283a74d2edcf961cf1dc54a35ed0cc21182304bb961505
206f74e88c Support make src/bitcoin-node and src/bitcoin-gui (João Barbosa)
Pull request description:
This change adds the following configure output variables
```
dnl Multi Process
BITCOIN_MP_NODE_NAME=bitcoin-node
BITCOIN_MP_GUI_NAME=bitcoin-gui
```
and adds support for
```sh
make src/bitcoin-node src/bitcoin-gui
```
ACKs for top commit:
laanwj:
Code review ACK 206f74e88c
Tree-SHA512: 4d1a694b9010ecc267ee955f4475127a58e6da72f30179ec740285ee6fe03cd91dcb6847317a47460dbd548edb88b7da6c7a98eac10f0dabe3ce4e83e0aa8093
fanquake