## Issue being fixed or feature implemented
Client version string is inconsistent. Building `v20.0.0-beta.8` tag
locally produces binaries that report `v20.0.0-beta.8` version but
binaries built in guix would report
`v20.0.0rc1-g3e732a952226a20505f907e4fd9b3fdbb14ea5ee` instead. Building
any commit after `v20.0.0-beta.8` locally would result in versions like
`v20.0.0rc1-8c94153d2497` which is close but it's still yet another
format. And both versions with `rc1` in their names are confusing cause
you'd expect them to mention `beta.8` instead maybe (or is it just me?
:D ).
## What was done?
Change it so that the version string would look like this:
on tag: ~`v20.0.0-beta.8-dev` or `v20.0.0-beta.8-gitarc`~
`v20.0.0-beta.8`
post-tag: ~`v20.0.0-beta.8-1-gb837e08164-gitarc`~
`v20.0.0-beta.8-1-gb837e08164`
post-tag format is
`recent tag`-`commits since that tag`-`g+12 chars of commit hash`-`dirty
(optional)` ~-`dev or gitarc`~
~`dev`/`gitarc` suffixes should help avoiding confusion with the release
versions and they also indicate the way non-release binaries were
built.~
Note that release binaries do not use any of this, they still use
`PACKAGE_VERSION` from `configure` like before.
Also, `CLIENT_VERSION_RC` is no longer used in this setup so it was
removed.
Few things aren't clear to me yet:
1. Version bump in `configure.ac` no longer affects the reported version
(unless it's an actual release). Are there any downsides I might be
missing?
2. Which tag should we use on `develop` once we bump version in
configure? `v21.0.0-init`? `v21.0.0-alpha1`?
3. How is it going to behave once `merge master back into develop` kind
of PR is merged? E.g. say `develop` branch is on `v21.0.0-alpha1` tag
and we merge v20.1.0 from `master` back into it. Will this bring
`v20.1.0` release tag into `develop`? Will it become the one that will
be used from that moment? If so we will probably need another tag on
`develop` every time such PR is merged e.g. `v21.0.0-alpha2` (or
whatever the next number is).
Don't think these are blockers but would like to hear thoughts from
others.
## How Has This Been Tested?
Built binaries locally, built them using guix at a specific tag and at
some commit on top of it.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
1ccb9f30c040daf688f89f0d63e9f5e7b131d193 Move Win32 defines to configure.ac to ensure they are globally defined (Luke Dashjr)
Pull request description:
#9245 no longer needs this, since the main `_WIN32_WINNT` got bumped by something else.
So rather than just lose it, might as well get it merged in independently.
I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere.
ACKs for top commit:
fanquake:
ACK 1ccb9f30c040daf688f89f0d63e9f5e7b131d193 - checked that the binaries produced are the same.
Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
excludes:
- a661449a2eeaf88efda36b6a84084dcbfe5b24eb
inapplicable:
- 6fd2118e777d11cbc81a45313d1a7d6400e34f3f
- 06cfc9cadf7c5dd43147e6525a348d5f2d299422
above two commits are inapplicable as tighter UniValue integration
got rid of that logic altogether, backported as dash#4823
0fef60c63d6d2f4df8e698936221e2330ef3a244 build: improved output of configure for build OS (sachinkm77)
Pull request description:
The purpose of this fix is to improve output of the configure script by providing the build OS. This is done by leveraging the build_os set by the script config.sub / config.guess. #18966
ACKs for top commit:
fanquake:
ACK 0fef60c63d6d2f4df8e698936221e2330ef3a244 - thanks for following up.
Tree-SHA512: b9f49df901a9d37eb16c67c063bb3611602a84391aa54d097a52b740f474c2785c24bf405522d15d724fde25070d354bf20b885add2ee4405a71cbe9ebab5ff3
a30b0a24e97eae7f6d1428c5bf339a579872f28e build: enable -Werror=gnu (Vasil Dimov)
Pull request description:
Stop the build if a warning is emitted due to `-Wgnu` and
`--enable-werror` has been used. As usual - this would help notice such
a warning that is about to be introduced in new code.
This is a followup to
https://github.com/bitcoin/bitcoin/pull/18088 build: ensure we aren't using GNU extensions
ACKs for top commit:
practicalswift:
ACK a30b0a24e97eae7f6d1428c5bf339a579872f28e
Empact:
ACK a30b0a24e9
Tree-SHA512: f81b71cf3ee4db88b6f664c571075e0d30800a604f067f44273f256695a1dea533779db2ac859dd0a4cd8b66289c3e45f4aff1cfadfa160a1c354237167b05e2
68537275bd91d1dc14a69609ae443f955bfdbd64 build: Enable -Werror=sign-compare (Ben Woosley)
eac6a3080d38cfd4eb7204ecd327df213958e51a refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377e30678ac9b8338ea920e50b7296da6bd5 test: Fix outstanding -Wsign-compare errors (Ben Woosley)
Pull request description:
Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.
In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.
This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee3c7abfe9d43c9eb68f102498cc2b9a vs 75fb37ce68289eb7e00e2ccdd2ef7f9271332545
ACKs for top commit:
fjahr:
re-ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
practicalswift:
ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
5a157eb3703e1e65ed285f13a824562ab12aa3ff Bugfix: configure: Only avoid -isystem for exact /usr/include path (Luke Dashjr)
556ee6f2fa0e2da2bb12fe05a885431f61db2e47 Bugfix: configure: Quote SUPPRESS_WARNINGS sufficiently to preserve brackets (Luke Dashjr)
Pull request description:
The regex includes `[/ ]` which is supposed to match either a forward slash or a space, but m4 treats the brackets as special characters and effectively strips them out, leading to -isystem /usr/include paths except for in the typical scenario where it is the final parameter in the flag string.
ACKs for top commit:
hebasto:
ACK 5a157eb3703e1e65ed285f13a824562ab12aa3ff, tested on Ubuntu 22.04 with clang 14.0.
vasild:
ACK 5a157eb3703e1e65ed285f13a824562ab12aa3ff
Tree-SHA512: 5c8c282b647b7853b8fad1b5b473703c4a0635073d2685a8ac984151046e2c6a859e6972465419d27356dd29a47f21a2a3a6ad402ec434fe1f9882e5a35f0749
061accfddd2d27ad584c826413c68857d2be9ced build: require libtool 2.4.2 (fanquake)
Pull request description:
Every system we support has 2.4.6 available, except for OpenBSD, which
[currently ships with 2.4.2](https://github.com/openbsd/ports/blob/master/devel/libtool/Makefile) (released 2011). For now, set our minimum
required version to that.
After a 7 year hitus, 2.4.7 has also very recently been released:
https://savannah.gnu.org/forum/forum.php?forum_id=10139.
Partially motivated by comments in https://github.com/bitcoin/bitcoin/pull/24615.
See also: https://repology.org/project/libtool/versions
ACKs for top commit:
achow101:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
hebasto:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
prusnak:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
Tree-SHA512: bc032022b8609b73253ff1c4fd480f4d09be761b8fec295f39319f9499ee2df116f55295da476be551c43ed88fbb0bfed7bb5a188b9979b34147fe39737ec76f
3d70c058684727cd036314014ec0a9208d3e9168 build: remove faketime unsetting and comments from configure.ac (fanquake)
Pull request description:
We no-longer use [`faketime`](https://github.com/wolfcw/libfaketime) (it used to be required in gitian), so as far as I'm aware, there is no need for us to unset `FAKETIME` or mention it in our build docs.
ACKs for top commit:
laanwj:
Code review ACK 3d70c058684727cd036314014ec0a9208d3e9168
prusnak:
Approach ACK 3d70c05
hebasto:
ACK 3d70c058684727cd036314014ec0a9208d3e9168, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 9cf89d63b81119f3d2f02975a66ec0b93e861993fdb0e4f70538e3be6e0047dc09ce87ef2de40cbf877647a21706b39ddf07240c77765278d383d7a7878cc7eb
e46287853f3a41c3f0772d3448d8df4ea01a156f build: remove --enable-determinism configure option (fanquake)
Pull request description:
This was added by me a while back, with the intention of expanding what this did. That hasn't happened, and this hasn't gained much use. There's also been some discussion of some configure option fatigue, so just remove it for now. Note that `-Wl,--no-insert-timestamp` is also already used in the Guix build.
ACKs for top commit:
MarcoFalke:
review ACK e46287853f3a41c3f0772d3448d8df4ea01a156f
jarolrod:
Code Review ACK e46287853f3a41c3f0772d3448d8df4ea01a156f
Tree-SHA512: ac976f88203eca2a49e296a98693dbe53330e0cb0e273c5ff1fcded30daeb6070cc5beeae35cf9acfdc2279cd64c274d5aeb588aef077aa9bfde39bb23570491
8f7704d0321a71c1691837a6bd3b4e05f84d3031 build: improve detection of eBPF support (fanquake)
Pull request description:
Just checking for the `sys/sdt.h` header isn't enough, as systems like macOS have the header, but it doesn't actually have the `DTRACE_PROBE*` probes, which leads to [compile failures](https://github.com/bitcoin/bitcoin/pull/22006#issuecomment-859559004). The contents of `sys/sdt.h` in the macOS SDK is:
```bash
#ifndef _SYS_SDT_H
#define _SYS_SDT_H
/*
* This is a wrapper header that wraps the mach visible sdt.h header so that
* the header file ends up visible where software expects it to be. We also
* do the C/C++ symbol wrapping here, since Mach headers are technically C
* interfaces.
*
* Note: The process of adding USDT probes to code is slightly different
* than documented in the "Solaris Dynamic Tracing Guide".
* The DTRACE_PROBE*() macros are not supported on Mac OS X -- instead see
* "BUILDING CODE CONTAINING USDT PROBES" in the dtrace(1) manpage
*
*/
#include <sys/cdefs.h>
__BEGIN_DECLS
#include <mach/sdt.h>
__END_DECLS
#endif /* _SYS_SDT_H */
```
The `BUILDING CODE CONTAINING USDT PROBES` section from the dtrace manpage is available [here](https://gist.github.com/fanquake/e56c9866d53b326646d04ab43a8df9e2), and outlines the more involved process of using USDT probes on macOS.
ACKs for top commit:
jb55:
utACK 8f7704d0321a71c1691837a6bd3b4e05f84d3031
practicalswift:
cr ACK 8f7704d0321a71c1691837a6bd3b4e05f84d3031
hebasto:
ACK 8f7704d0321a71c1691837a6bd3b4e05f84d3031, tested on macOS Big Sur 11.4 (20F71) and on Linux Mint 20.1 (x86_64) with depends.
Tree-SHA512: 5f1351d0ac2e655fccb22a5454f415906404fdaa336fd89b54ef49ca50a442c44ab92d063cba3f161cb8ea0679c92ae3cd6cfbbcb19728cac21116247a017df5
1111457d7433c2cca1d7e98946f6954e3a0280ef build: Disable deprecated-copy warning only when external warnings are enabled (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/18967
Alternative to https://github.com/bitcoin/bitcoin/pull/22240
ACKs for top commit:
fanquake:
tACK 1111457d7433c2cca1d7e98946f6954e3a0280ef
Tree-SHA512: 0fc826f26ebbeab662fa7eed2a5cc1630c6c4e612deb91734885fc8bae0352be657ec48ae94ff55a984ac36d27b95cea8d947cc5cf408231d56addecf79db83f
9bac71350d98580cc7441957fc7c3fa2f4158553 build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) (Sebastian Falbesoner)
584fd91d2d294883e6896dbd64a2176528e94581 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 9bac71350d98580cc7441957fc7c3fa2f4158553
Tree-SHA512: aec89faf6ba52b6f014c610ebef7b725d9e967207d58b42a4a71afc9f1268fcb673ecc85b33a2a3debba8105a304dd7edaba4208c5373fcef2ab83e48a170051
d51f0fa4b7b19281efe65aacf414845c661d0a13 doc: add release notes for 26896 (fanquake)
2b248798d96f794db08b7725730b5fb4e00b9b10 build: remove --enable-upnp-default from configure (fanquake)
02f5a5e7b5fd7ba35e407d4409202a0e0fed003c build: remove --enable-natpmp-default from configure (fanquake)
25a0e8ba0b31d8bd265df0589fe49241a60d0fc2 Remove configure-time setting of DEFAULT_UPNP (fanquake)
06562e5fa771dab275a9cab4914cd64d961a52bc Remove configure-time setting of DEFAULT_NATPMP (fanquake)
Pull request description:
This PR removes the `--enable-upnp-default` and `--enable-natpmp-default` options from configure.
It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default.
I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a `.conf` file, can't or don't want to pass command line options at runtime, and also don't want to modify the source code?
ACKs for top commit:
hebasto:
ACK d51f0fa4b7b19281efe65aacf414845c661d0a13, rebased and comments have been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/26896#pullrequestreview-1273910740).
TheCharlatan:
ACK d51f0fa4b7b19281efe65aacf414845c661d0a13
Tree-SHA512: 481decd8bddd8b03b7319591e3acf189f7b6b96c9a9a8c5bc1a3f8ec00d0b8f9b52d2f5c28a298a2ec947cfe9611cfd184e393ccb2e4e21bfce86ca7d4de60d3
## Issue being fixed or feature implemented
Bump develop version to v20.1.0; v21 features should not yet be merged
into develop; when the time is come to begin merging v21 features into
develop, we can then bump version again.
## What was done?
Bump version
## How Has This Been Tested?
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
4446ef0a549d567a88d82b606aa8c47f115673f9 build: remove support for weak linking getauxval() (fanquake)
e56100c5b4daf2285dde9807bf654599aa19bd6b build: remove arm includes from getauxval() check (fanquake)
Pull request description:
It was [pointed out in #23030](https://github.com/bitcoin/bitcoin/pull/23030#issuecomment-922893367) that we might be able to get rid of our weak linking of [`getauxval()`](https://man7.org/linux/man-pages/man3/getauxval.3.html) (`HAVE_WEAK_GETAUXVAL`) entirely, with only Android being a potential holdout:
> I wonder if it's time to get rid of HAVE_WEAK_GETAUXVAL. I think it's confusing. Either we build against a C library that has this functionality, or not. We don't do this weak linking thing for any other symbols and recently got rid of the other glibc backwards compatibility stuff.
> Unless there is still a current platform that really needs it (Android?), I'd prefer to remove it from the build system, it has caused enough issues.
After looking at Android further, it would seem that given we are moving to using `std::filesystem`, which [requires NDK version 22 and later](https://github.com/android/ndk/wiki/Changelog-r22), and `getauxval` has been available in the since [API version 18](https://developer.android.com/ndk/guides/cpu-features#features_using_libcs_getauxval3), that shouldn't really be an issue. Support for API levels < 19 will be dropped with the NDK 24 release, and according to [one website](https://apilevels.com/), supporting API level 18+ will cover ~99% of devices. Note that in the CI we currently build with NDK version 22 and API level 28.
The other change in this PR is removing the include of headers for ARM intrinsics, from the check for strong `getauxval()` support in configure, as they shouldn't be needed. Including these headers also meant that the check would basically only succeed when building for ARM. This would be an issue if we remove weak linking, as we wouldn't detect `getauxval()` as supported on other platforms. Note that we also use `getauxval()` in our RNG when it's available.
I've checked that with these changes we detect support for strong `getauxval()` on Alpine (muslibc). On Linux, previously we'd be detecting support for weak getauxval(), now we detect strong support. Note that we already require glibc 2.17, and `getauxval()` was introduced in `2.16`.
This is an alternative / supersedes #23030.
ACKs for top commit:
laanwj:
Code review and tested ACK 4446ef0a549d567a88d82b606aa8c47f115673f9
Tree-SHA512: 5f2a9e9cc2d63bddab73f0dcb169d4d6beda74622af82bc0439722f1189f81d052e2fc1eaf27056a7a606320d5ddc4c11075f0d051dd93d77c5e1c15337f354a
## Issue being fixed or feature implemented
Bump version
## What was done?
## How Has This Been Tested?
Na
## Breaking Changes
Na
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Bump version to rc.1
## What was done?
bump version
## How Has This Been Tested?
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
01a3392b1b778fa4fcf568013326d6ea1de4fb3b Drop bitcoin-wallet dependency on libevent (Russell Yanofsky)
0660119ac372c2863d14060ac1bc9bc243771f94 Drop unintended bitcoin-tx dependency on libevent (Russell Yanofsky)
Pull request description:
This fixes compile errors trying to build bitcoin-tx and bitcoin-wallet without libevent, which were reported by Luke Dashjr in https://github.com/bitcoin/bitcoin/issues/18465
The fix avoiding `bitcoin-tx` dependency on libevent just adds a conditional build rule. This is implemented in the first commit (more details in commit description).
The fix avoiding `bitcoin-wallet` dependency on libevent requires minor code changes, because `bitcoin-wallet` (unlike `bitcoin-tx`) links against code that calls `urlDecode` / `evhttp_uridecode`. This fix is implemented in the second commit (again details in the commit description).
ACKs for top commit:
jonasschnelli:
utACK 01a3392b1b778fa4fcf568013326d6ea1de4fb3b.
Tree-SHA512: d2245e912ab494cccceeb427a1eca8e55b01a0006ff93eebcfb5461ae7cecd1083ac2de443d9db036b18bdc6f0fb615546caaa20c585046f66d234937f74870a
ef712298c3f8bc2afdad783f05080443b72b3f77 util: Check for file being NULL in DirectoryCommit (Luke Dashjr)
457490403853321d308c6ca6aaa90d6f8f29b4cf Fix possible data race when committing block files (Evan Klitzke)
220bb16cbee5b91d0bc0fcc6c71560d631295fa5 util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke)
ce5cbaea63ad4ea78e533bdb14f47f414061ae7f util.h: Document FileCommit function (Evan Klitzke)
844d650eea3bd809884cc5dd996a388bdc58314e util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke)
f6cec0bcaf560fa310853ad3fe17022602b63d5f util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif (Evan Klitzke)
Pull request description:
Reviving #12696
ACKs for top commit:
laanwj:
Code review ACK ef712298c3f8bc2afdad783f05080443b72b3f77
Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce
## Issue being fixed or feature implemented
The order of members in a class/struct definition and the order of their
initialization should match. This ensures that the code is more
error-proof in cases where the order of member initializations is
important, as they may depend on each other.
Instead manual checking of member initialization better let CI handle
it.
Last PR where it's noticed:
https://github.com/dashpay/dash/pull/5531#discussion_r1299404387
## What was done?
New flag "-Werror=reorder" for `configure.ac` and fixes existing code.
## How Has This Been Tested?
Build code with `--enable-werror`
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
904d875cf5aecc337daa6a2243a803033cf4eee3 configure: output notice that test binary is disabled by fuzzing (Andrew Poelstra)
Pull request description:
I wasted a bit of time today running a stale `test_bitcoin` and not understanding why, until I remembered that I'd ./configured my working directory with --enable-fuzz.
Top commit has no ACKs.
Tree-SHA512: 6cbe30547332114ad3fe61c67e224f5a28aac4b1b58e0acecb29cb04f5a34f792c927797aa8000449aae076435bd45acf209b7323b0b48fa971705d6ed3e6529
faf7d4fa86b700ec272806cd2bd8666a92405619 build: Add cov_fuzz target (MarcoFalke)
fac71e364e4bbaeffc35e45aff8c8c2c6f2b5c67 build: link fuzz/test_runner.py for out-of-tree builds (MarcoFalke)
faf2c5aca01643eb560287e08f9c0a7ca0ac9c88 build: Remove unused USE_COVERAGE (MarcoFalke)
Pull request description:
Only libFuzzer is supported right now, so clang is required. Thus, this needs a workaround such as https://github.com/bitcoin/bitcoin/issues/12602#issuecomment-562788247
Can be tested with:
```
mkdir build && cd build
../configure --enable-fuzz --with-sanitizers=fuzzer --enable-lcov --enable-lcov-branch-coverage CC=clang CXX=clang++
make $MAKEJOBS
make cov_fuzz
ACKs for top commit:
practicalswift:
ACK faf7d4fa86b700ec272806cd2bd8666a92405619
Tree-SHA512: 6828f8f81d95f6781713d0b09d7eba2ffdb50217e09ca839db61791a4ed70024859c7a0cb01d9eede79166d574dd57ece01f9d9fe2610d4a72a4ca4a4ce0b838
cd04286825c6512b46bf59ab7b3dfffb0e36d65b build: Fix typo in EVENT_CFLAGS variable (Hennadii Stepanov)
f709ad0c907d87d03002455967cc30ae7d704d80 build: Fix libevent linking for bench_bitcoin binary (Hennadii Stepanov)
Pull request description:
This change fixes `libevent` linking error for the `bench_bitcoin` binary.
This PR is an alternative to #18377.
Fix#18373.
Also fixed a typo in `EVENT_CFLAGS` variable noted by **brakmic**.
ACKs for top commit:
fanquake:
ACK cd04286825c6512b46bf59ab7b3dfffb0e36d65b
Tree-SHA512: a62f7457e86b11d3a55d603ea5d83f3a413792e2f28a0c72300e54d12591bd6f0acc1d76a4bd4b591e0223bc6d530e7a4b9a8b939fe2fdbf2dddfda5b1b537be
## Issue being fixed or feature implemented
gmp can't be detected on macos when installed via `brew` atm
## What was done?
detect package prefix and adjust CPPFLAGS and LDFLAGS accordingly
## How Has This Been Tested?
`./configure`
before: `configure: error: libgmp headers missing`
after: passes
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb build: Add -Werror=mismatched-tags (Hennadii Stepanov)
1485124291368c4a2ca8ea09c18e813f1dbabf5c Fix -Wmismatched-tags warnings (Hennadii Stepanov)
Pull request description:
Warnings were introduced in #20749:
```
./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
class CCheckpointData;
^
./chainparams.h:24:8: note: previous use is here
struct CCheckpointData {
^
./validation.h:43:1: note: did you mean struct here?
class CCheckpointData;
^~~~~
struct
1 warning generated.
```
This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435
ACKs for top commit:
glozow:
utACK b6aadcd5b4🚗
practicalswift:
cr ACK b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb: patch looks correct
Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29
6cef3652d143a1dddad1254cab0953561d24c1fa build: fix -Wformat-security check when compiling with GCC (fanquake)
Pull request description:
GCC expects `-Wformat` to be passed with [`-Wformat-security`](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html), which means
when we test for it in configure it currently fails:
```bash
checking whether C++ compiler accepts -Wformat-security... no
...
configure:15907: checking whether C++ compiler accepts -Wformat-security
configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security conftest.cpp >&5
cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
cc1plus: all warnings being treated as errors
```
and never gets added to our CXX flags. Note that Clang does not have this requirement and the check is working correctly there.
The change in this PR is the simple fix, however we might want to consider using something like `-Wformat=2` in future, which in GCC is equivalent to `-Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.` and similar [in Clang](https://clang.llvm.org/docs/DiagnosticsReference.html#wformat-2).
ACKs for top commit:
practicalswift:
ACK 6cef3652d143a1dddad1254cab0953561d24c1fa
laanwj:
ACK 6cef3652d143a1dddad1254cab0953561d24c1fa
Tree-SHA512: f9230d42af39f85ea9d2f55dbbebd2bae4740fe59b0da2e092af3ac9ef7e6799d3a4cf83eb64574c63982e5f6b14e226d44c84fa0335255d65c9947d86a1ea38
182dbdf0f4b6e6484b0d4588aaefacc75862a99c util: Detect posix_fallocate() instead of assuming (Vasil Dimov)
Pull request description:
Don't assume that `posix_fallocate()` is available on Linux and not
available on other operating systems. At least FreeBSD has it and we
are not using it.
Properly check whether `posix_fallocate()` is present and use it if it
is.
ACKs for top commit:
laanwj:
ACK 182dbdf0f4b6e6484b0d4588aaefacc75862a99c
Tree-SHA512: f9ed4bd661f33ff6b2b1150591e860b3c1f44e12b87c35e870d06a7013c4e841ed2bf17b41ad6b18fe471b0b23a4b5e42cf1400637180888e0bc56c254fe0766
0ae8f18dfe143051fec6ae10ea7df10142e3ff2f build: add -Wgnu to compile flags (fanquake)
3a0fd7726b8b916de6cce33bb67f48990575f923 Remove use of non-standard zero variadic macros (Ben Woosley)
49f6178c3e5e3ad54a419da9d8523207da17fc64 Drop unused LOG_TIME_MICROS helper (Ben Woosley)
5d4999951ee32e333b511245862628e80f83b703 prevector: Avoid unnamed struct, which is a GNU extension (DesWurstes)
Pull request description:
Since we [started using](https://github.com/bitcoin/bitcoin/pull/7165) the `ax_cxx_compile_stdcxx.m4` macro we've been passing `[noext]` to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn _off_ extension support so they would fail to compile".
However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former.
#### anonymous structs
```bash
./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct]
struct {
```
This is fixed in b849212c1e.
#### variadic macros
```bash
./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
::Unserialize(s, VARINT(nVersionDummy));
```
This is taken care of in #18087.
The `LOG_TIME_*` macros introduced in #16805 make use of a [GNU extension](https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html).
```bash
In file included from validation.cpp:22:
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
6 warnings generated.
```
This is fixed in 081a0ab64eb442bc85c4d4a4d3bc2c8e97ac2a6d and 612e8e138b97fc5ad2f38847300132a8fc423c3f.
#### prevention
To ensure that usage doesn't creep back in we can add [`-Wgnu`](https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu) to our compile time flags, which will make Clang warn whenever it encounters GNU extensions.
This would close#14130.
Also related to #17230, where it's suggested we use a GNU extension, the `gnu::pure` attribute.
ACKs for top commit:
practicalswift:
ACK 0ae8f18dfe143051fec6ae10ea7df10142e3ff2f -- diff looks correct
MarcoFalke:
ACK 0ae8f18dfe143051fec6ae10ea7df10142e3ff2f
vasild:
utACK 0ae8f18df
dongcarl:
ACK 0ae8f18dfe143051fec6ae10ea7df10142e3ff2f
Tree-SHA512: c517404681ef8edf04c785731d26105bac9f3c9c958605aa24cbe399c649e7c5ee0c4aa8e714fd2b2d335e2fbea4d571e09b0dec36678ef871f0a6683ba6bb7f
* Squashed 'src/dashbls/' content from commit 66ee820fbc
git-subtree-dir: src/dashbls
git-subtree-split: 66ee820fbc9e3b97370db8c164904af48327a124
* build: stop tracking build-system generated relic_conf.h.in
* build: add support for building bls-signatures from local subtree
* build: add exclusions to linting scripts and filters
* build: drop bls-signatures (bls-dash) from depends
880d4aaf81f3d5d7fbb915905c2e61b816a6a747 build: use BOOST_NO_CXX98_FUNCTION_BASE to suppress warnings (fanquake)
1bdbbbdc46c4e50bf07bc362e7e391ea1a53ea2f build: suppress array-bounds errors in libxkbcommon (fanquake)
Pull request description:
2 changes to better support building with GCC 12, which out of the box, is currently broken if you want to build using depends.
Prevent `-Warray-bounds` errors when building libxkbcommon. i.e:
```bash
src/xkbcomp/ast-build.c:82:27: error: array subscript 'ExprDef[0]' is partly outside array bounds of 'unsigned char[32]' [-Werror=array-bounds]
82 | expr->expr.value_type = type;
| ~~~~~~~~~~~~~~~~~~~~~~^~~~~~
src/xkbcomp/ast-build.c:75:21: note: object of size 32 allocated by 'malloc'
75 | ExprDef *expr = malloc(size);
| ^~~~~~~~~~~~
```
It might be the case that these would be fixed by updating the
package, but that would also require installing new build tools (meson),
as well as potentially more dependencies (wayland), and it'd need
testing with Qt. For now, just turn the errors into wanrings.
Define `BOOST_NO_CXX98_FUNCTION_BASE` to prevent GCC warning about the use of `std::unary_function`. i.e:
```bash
/bitcoin/depends/aarch64-unknown-linux-gnu/include/boost/container_hash/hash.hpp:131:33:
warning: 'template<class _Arg, class _Result> struct std::unary_function' is deprecated [-Wdeprecated-declarations]
131 | struct hash_base : std::unary_function<T, std::size_t> {};
| ^~~~~~~~~~~~~~
In file included from /usr/include/c++/12/bits/unique_ptr.h:37,
from /usr/include/c++/12/memory:76,
from ./init.h:10,
from init.cpp:10:
/usr/include/c++/12/bits/stl_function.h:117:12: note: declared here
117 | struct unary_function
```
Boost `container_hash` (included via functional -> multi_index) uses
[`std::unary_function`, which was deprecated in C++11](https://en.cppreference.com/w/cpp/utility/functional/unary_function), and "removed" in
C++17. It's use causes warnings with newer compilers, i.e GCC 12.1.
Use the MACRO outlined in https://github.com/boostorg/container_hash/issues/22, and added to Boost Config for GCC 12 in https://github.com/boostorg/config/pull/430, to prevent it's use.
[BOOST_NO_CXX98_FUNCTION_BASE](https://www.boost.org/doc/libs/master/libs/config/doc/html/boost_config/boost_macro_reference.html):
> The standard library no longer supports std::unary_function and std::binary_function.
> They were deprecated in C++11 and is removed from C++14.
Guix Build (x86_64):
```bash
```
Guix Build (arm64):
```bash
```
ACKs for top commit:
laanwj:
Code review ACK 880d4aaf81f3d5d7fbb915905c2e61b816a6a747
Tree-SHA512: 10c4679c3eb788e9279acc4960731c55ae1568bd3df525d3c46f97d8b0319e7d8450b1638b6777d98111b5991dba5c787e95d80b1ac932e0b4779d4b8e74875e
2525c096b002a89d4c561e1474800496ad8ebd7e build: remove configure checks for win libraries we don't link against (fanquake)
Pull request description:
While cross compiling, `HOST=x86_64-w64-mingw32`, none of these libs actually seem to be passed to the linker. i.e tailing a build with `make -j5 V=1 | rg -i 'mingwthrd|winspool|rpcrt4|crypt32'`.
I'm not 100% sure about `crypt32`, even though the majority of our Windows cryptography usage, i.e [`CryptAcquireContextW`](https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptacquirecontextw) or [`CryptGenRandom`](https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom) is provided by `advapi32`.
Note that `rpcrt4` and `mingwthrd` are already missing from the MSVC build, so we can sync the remainder once it's clear what's actually needed. Hopefully sipsorcery can add some MSVC insight.
ACKs for top commit:
practicalswift:
ACK 2525c096b002a89d4c561e1474800496ad8ebd7e -- diff looks correct
sipsorcery:
ACK 2525c096b002a89d4c561e1474800496ad8ebd7e.
Tree-SHA512: c756618f85ce2ab1e14e5514dbdc490d94c1c6dfd7a3e3d3b16344ae302fb789585dd10b5c2d784f961f3115bec1d914615051b3184bea00dfbcc3c23884ab4a
4783115fd4cccb46a7f8c592b34fa7c094c29410 net: add ifaddrs.h include (fanquake)
879215e665a9f348c8d3fa92701c34065bc86a69 build: check if -lsocket is required with *ifaddrs (fanquake)
87deac66aa747481e6f34fc80599e1e490de3ea0 rand: only try and use freeifaddrs if available (fanquake)
Pull request description:
Fixes#21485 by linking against `-lsocket` when it's required for using `*ifaddrs` functions.
ACKs for top commit:
laanwj:
Code review ACK 4783115fd4cccb46a7f8c592b34fa7c094c29410
hebasto:
ACK 4783115fd4cccb46a7f8c592b34fa7c094c29410, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4542e036e9b029de970eff8a9230fe45d9204bb22313d075f474295d49bdaf1f1cbb36c0c6e2fa8dbbcdba518d8d3a68a6116ce304b82414315f333baf9af0e4
It's a shared library, so we should keep its name and API
distinguishable from Bitcoin's and avoid pkgconfig confusion
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
e959b46aa933856e7636557f4ec6fce0efbc76aa build: explicitly disable libsecp256k1 openssl based tests (fanquake)
Pull request description:
Backport of #23314
These tests are failing when run against OpenSSL 3, and have been
removed upstream, bitcoin-core/secp256k1#983, so
disabled them for now to avoid `make check` failures.
Note that this will also remove warning output from our build, due to
the use of deprecated OpenSSL API functions. See #23048.
Top commit has no ACKs.
Tree-SHA512: ab3213dc82e7a64a005ce237710009bb447dee2702c4c02245e70df62063a00add73c4e80e9c619ce57345d4a2808fd4dc08e2e02a319b0f3d9285b8b0056599
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
e9f948c72790136656df6056fd9e3698f360e077 build: Convert warnings into errors when testing for -fstack-clash-protection (Hennadii Stepanov)
Pull request description:
Apple clang version 12.0.5 (clang-1205.0.22.9) that is a part of Xcode 12.5, and is based on LLVM clang 11.1.0, fires spammy warnings:
```
clang: warning: argument unused during compilation: '-fstack-clash-protection' [-Wunused-command-line-argument]
```
From the https://github.com/apple/llvm-project:
```
$ git log --oneline | grep 'stack-clash-protection'
00065d5cbd02 Revert "-fstack-clash-protection: Return an actual error when used on unsupported OS"
4d59c8fdb955 -fstack-clash-protection: Return an actual error when used on unsupported OS
df3bfaa39071 [Driver] Change -fnostack-clash-protection to -fno-stack-clash-protection
68e07da3e5d5 [clang][PowerPC] Enable -fstack-clash-protection option for ppc64
515bfc66eace [SystemZ] Implement -fstack-clash-protection
e67cbac81211 Support -fstack-clash-protection for x86
454621160066 Revert "Support -fstack-clash-protection for x86"
0fd51a4554f5 Support -fstack-clash-protection for x86
658495e6ecd4 Revert "Support -fstack-clash-protection for x86"
e229017732bc Support -fstack-clash-protection for x86
b03c3d8c6209 Revert "Support -fstack-clash-protection for x86"
4a1a0690ad68 Support -fstack-clash-protection for x86
f6d98429fcdb Revert "Support -fstack-clash-protection for x86"
39f50da2a357 Support -fstack-clash-protection for x86
```
I suppose, that Apple clang-1205.0.22.9 ends with on of the "Revert..." commits.
This PR prevents using of the `-fstack-clash-protection` flag if it causes warnings.
---
System: macOS Big Sur 11.3 (20E232).
ACKs for top commit:
jarolrod:
re-ACK e9f948c72790136656df6056fd9e3698f360e077
Sjors:
tACK e9f948c72790136656df6056fd9e3698f360e077 on macOS 11.3.1
Tree-SHA512: 30186da67f9b0f34418014860c766c2e7f622405520f1cbbc1095d4aa4038b0a86014d76076f318a4b1b09170a96d8167c21d7f53a760e26017f486e1a7d39d4
7b3434f8002d1a8cf0dbd0a0caef28e783b1efd8 build: don't try and use -fstack-clash-protection on Windows (fanquake)
Pull request description:
This has never worked with any of the mingw-w64 compilers we use, and the `-O0` is causing issues for builders applying spectre mitigations (see [IRC logs](http://www.erisian.com.au/bitcoin-core-dev/log-2021-03-12.html#l-15)).
Recent discussion on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90458 would also indicate that this should just not be used on Windows.
ACKs for top commit:
laanwj:
Concept and code review ACK (but untested) 7b3434f8002d1a8cf0dbd0a0caef28e783b1efd8
hebasto:
ACK 7b3434f8002d1a8cf0dbd0a0caef28e783b1efd8, I've verified that this change does not affect builds for `HOST=x86_64-w64-mingw32` by comparing sizes of the output `*.exe` files.
Tree-SHA512: 72b582321ddff8db3201460fa42a53304e70f141ae078d76a4d4eeb1ca27c8dd567ccb468cc8616179c8df784bd8ca038dcb9a277f9e29f9d98c3cc842916b18
e9189a750b237eba1befc6b16c12c2cee3e0176c 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 e9189a750b237eba1befc6b16c12c2cee3e0176c
MarcoFalke:
not an ACK e9189a750b237eba1befc6b16c12c2cee3e0176c , I only tested configure on my system (gcc-10, clang-11):
hebasto:
ACK e9189a750b237eba1befc6b16c12c2cee3e0176c, 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
d3ef947524a07f8d7fbad5b95781ab6cacb1cb49 build: Check that Homebrew's berkeley-db4 package is actually installed (Hennadii Stepanov)
Pull request description:
On master (a0489f3472f3799dc1ece32a59556fd239c4c14b) the `configure` script is not able to determine that Homebrew's `berkeley-db4` package is uninstalled. This causes a compile error on macOS.
With this PR, and with the [uninstalled](https://stackoverflow.com/questions/20802320/detect-if-homebrew-package-is-installed) `berkeley-db4` package:
```
% ./configure -q
configure: error: libdb_cxx headers missing, Bitcoin Core requires this library for BDB wallet support (--without-bdb to disable BDB wallet support)
```
Related #20478.
ACKs for top commit:
promag:
Tested ACK d3ef947524a07f8d7fbad5b95781ab6cacb1cb49.
willcl-ark:
tACK d3ef947524a07f8d7fbad5b95781ab6cacb1cb49
jonasschnelli:
utACK d3ef947524a07f8d7fbad5b95781ab6cacb1cb49
Tree-SHA512: 8dc532e08249ec63bd357594aa458d314b6e8537fc63f5b1d509c84d0d71d5b1f70172caa1a7efe2fc8af31c829e7982a0695cf3fbe5cbc477019550269915e1
982e548a9a78b1b0abad59b54c780b6b06570452 Don't set BDB flags when configuring without (Jonas Schnelli)
Pull request description:
Configuring `--without-bdb` on MacOS leads to a compile error (when BerkeleyDB is not installed). `brew --prefix berkeley-db4` always reports the target directory (even if not installed).
This PR prevents BDB_CFLAGS (et al) from being populated when configuring `--without-bdb`
```
ld: warning: directory not found for option '-L/Users/user/Documents/homebrew/Cellar/berkeley-db@4/4.8.30/lib'
ld: warning: directory not found for option '-L/Users/user/Documents/homebrew/Cellar/berkeley-db@4/4.8.30/lib'
ld: library not found for -ldb_cxx-4.8
ld: library not found for -ldb_cxx-4.8
```
ACKs for top commit:
promag:
Tested ACK 982e548a9a78b1b0abad59b54c780b6b06570452.
hebasto:
ACK 982e548a9a78b1b0abad59b54c780b6b06570452, tested on macOS 11 Big Sur.
Tree-SHA512: f8ca0adca0e18e2de4c0f99d5332cba70d957a9d31a357483b43dcf61c2ed4749d223eabadd45fdbf3ef0781c6b37217770e9aa935b5207eaf7f87c5bdfe9e95
b536813cefc13f5c54a28a7c2fce8c69e89d6624 build: add -fstack-clash-protection to hardening flags (fanquake)
076183b36b76a11438463883ff916f17aef9e001 build: add -fcf-protection=full to hardening options (fanquake)
Pull request description:
Beginning with Ubuntu `19.10`, it's packaged GCC now has some additional hardening options enabled by default (in addition to existing defaults like `-fstack-protector-strong` and reducing the minimum ssp buffer size). The new additions are`-fcf-protection=full` and `-fstack-clash-protection`.
> -fcf-protection=[full|branch|return|none]
> Enable code instrumentation of control-flow transfers to increase program security by checking that target addresses of control-flow transfer instructions (such as indirect function call, function return, indirect jump) are valid. This prevents diverting the flow of control to an unexpected target. This is intended to protect against such threats as Return-oriented Programming (ROP), and similarly call/jmp-oriented programming (COP/JOP).
> -fstack-clash-protection
> Generate code to prevent stack clash style attacks. When this option is enabled, the compiler will only allocate one page of stack space at a time and each page is accessed immediately after allocation. Thus, it prevents allocations from jumping over any stack guard page provided by the operating system.
If your interested you can grab `gcc-9_9.3.0-10ubuntu2.debian.tar.xz` from https://packages.ubuntu.com/focal/g++-9. The relevant changes are part of the `gcc-distro-specs` patches, along with the relevant additions to the gcc manages:
> NOTE: In Ubuntu 19.10 and later versions, -fcf-protection is enabled by default for C, C++, ObjC, ObjC++, if none of -fno-cf-protection nor -fcf-protection=* are found.
> NOTE: In Ubuntu 19.10 and later versions, -fstack-clash-protection is enabled by default for C, C++, ObjC, ObjC++, unless -fno-stack-clash-protection is found.
So, if you're C++ using GCC on Ubuntu 19.10 or later, these options will be active unless you explicitly opt out. This can be observed with a small test:
```c++
int main() { return 0; }
```
```bash
g++ --version
g++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0
g++ test.cpp
objdump -dC a.out
..
0000000000001129 <main>:
1129: f3 0f 1e fa endbr64
112d: 55 push %rbp
112e: 48 89 e5 mov %rsp,%rbp
1131: b8 00 00 00 00 mov $0x0,%eax
1136: 5d pop %rbp
1137: c3 retq
1138: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
113f: 00
# recompile opting out of control flow protection
g++ test.cpp -fcf-protection=none
objdump -dC a.out
...
0000000000001129 <main>:
1129: 55 push %rbp
112a: 48 89 e5 mov %rsp,%rbp
112d: b8 00 00 00 00 mov $0x0,%eax
1132: 5d pop %rbp
1133: c3 retq
1134: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
113b: 00 00 00
113e: 66 90 xchg %ax,%ax
```
Note the insertion of an `endbr64` instruction when compiling and _not_ opting out. This instruction is part of the Intel Control-flow Enforcement Technology [spec](https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf), which the GCC control flow implementation is based on.
If we're still doing gitian builds for the `0.21.0` and `0.22.0` releases, we'd likely update the gitian image to Ubuntu Focal, which would mean that the GCC used for gitian builds would also be using these options by default. So we should decide whether we want to explicitly turn these options on as part of our hardening options (although not just for this reason), or, we should be opting-out.
GCC has supported both options since 8.0.0. Clang has supported `-fcf-protection` from 7.0.0 and will support `-fstack-clash-protection` in it's upcoming [11.0.0 release](https://clang.llvm.org/docs/ReleaseNotes.html#id6).
ACKs for top commit:
jamesob:
ACK b536813cefc13f5c54a28a7c2fce8c69e89d6624 ([`jamesob/ackr/18921.1.fanquake.build_add_stack_clash_an`](https://github.com/jamesob/bitcoin/tree/ackr/18921.1.fanquake.build_add_stack_clash_an))
laanwj:
Code review ACK b536813cefc13f5c54a28a7c2fce8c69e89d6624
Tree-SHA512: abc9adf23cdf1be384f5fb9aa5bfffdda86b9ecd671064298d4cda0440828b509f070f9b19c88c7ce50ead9ff32afff9f14c5e78d75f01241568fbfa077be0b7
8578c6fccd11404412d2c60f9bede311b79ca0d0 build: Fix search for brew-installed BDB 4 on OS X (Glenn Willen)
Pull request description:
~~NOTE: This PR contains one important fix that I need (to make Bitcoin Core build cleanly on my system without shenanigans), plus some related general cleanup that is not really necessary, and could be annoying. (I am prepared to defend my argument that BDB_CFLAGS is wrong here, and BDB_CPPFLAGS is right, but this could bite anybody who has gotten in the habit of -- or scripted -- setting the former.)~~
Ok, I have been convinced that I was too clever with the refactor and I have removed it. Now it's just the tiny change to fix the build on my local machine.
---
On OS X, when searching Homebrew keg-only packages for BDB 4.8, if we find it,
use BDB_CPPFLAGS and BDB_LIBS instead of CFLAGS and LIBS for the result. This
is (1) more correct, and (2) necessary in order to give this location
priority over other directories in the include search path, which may include
system include directories with other versions of BDB.
ACKs for top commit:
theuni:
ACK 8578c6fccd11404412d2c60f9bede311b79ca0d0.
Tree-SHA512: a28f48fc81a25736f7e77c663f21cd9a6ae1cd115682031c5aa695c94cb5afa11920330a60cd6a54832822a2aec1eb23123ac2e2dcd4f0b3835aef9c9339ac97
b155fcda5186c59fc4fb2a9eaaf791d132e0ab30 doc: fix typo in configure.ac (fanquake)
20a30922fbf6ba14e250ca649239af115dbbe7b0 doc: note why we can't use thread_local with glibc back compat (fanquake)
Pull request description:
Given that we went through a [gitian build](https://github.com/bitcoin/bitcoin/pull/18681) to remember why this is the case, we might as well make a note of it in configure.ac.
[From #18681](https://github.com/bitcoin/bitcoin/pull/18681#issuecomment-615526634):
Looking at the Linux build log, this has failed with:
```bash
Checking glibc back compat...
bitcoind: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
bitcoind: failed IMPORTED_SYMBOLS
bitcoin-cli: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
bitcoin-cli: failed IMPORTED_SYMBOLS
bitcoin-tx: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
bitcoin-tx: failed IMPORTED_SYMBOLS
bitcoin-wallet: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
bitcoin-wallet: failed IMPORTED_SYMBOLS
test/test_bitcoin: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
test/test_bitcoin: failed IMPORTED_SYMBOLS
bench/bench_bitcoin: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
bench/bench_bitcoin: failed IMPORTED_SYMBOLS
qt/bitcoin-qt: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18
```
`__cxa_thread_atexit_impl` is used for [thread_local variable destruction](https://sourceware.org/glibc/wiki/Destructor%20support%20for%20thread_local%20variables):
> To implement this support, glibc defines __cxa_thread_atexit_impl exclusively for use by libstdc++ (which has the __cxa_thread_atexit to wrap around it), that registers destructors for thread_local variables in a list. Upon thread or process exit, the destructors are called in reverse order in which they were added.
As suggested, this only became available in glibc 2.18. From the [2.18 release notes](https://sourceware.org/legacy-ml/libc-alpha/2013-08/msg00160.html):
> * Add support for calling C++11 thread_local object destructors on thread
and program exit. This needs compiler support for offloading C++11
destructor calls to glibc.
ACKs for top commit:
hebasto:
ACK b155fcda5186c59fc4fb2a9eaaf791d132e0ab30
Tree-SHA512: 5b9567e4a70598a4b0b91956f44ae0d93091db17c84cbf9817dac6cfa992c97d3438a8b1bb66644c74891f2149e44984daed445d22de93ca8858c5b0eabefb40
d76894987d0277e8011932ab7dfd77c537f8ea6e logging: enable thread_local usage on macOS (fanquake)
Pull request description:
Now that we're building against a newer SDK (`10.14`), we should be able to enable `thread_local` usage on macOS. Have tested building and running locally, as well as cross-compiling and running the binaries on a macOS 10.14 system.
#### master 8a56f79d491271120abc3843c46e9dda44edd308
```bash
src/bitcoind -logthreadnames=1
2020-02-06T04:38:33Z [] Bitcoin Core version v0.19.99.0-8a56f79d4 (release build)
2020-02-06T04:38:33Z [] Assuming ancestors of block 00000000000000000005f8920febd3925f8272a6a71237563d78c2edfdd09ddf have valid signatures.
2020-02-06T04:38:33Z [] Setting nMinimumChainWork=000000000000000000000000000000000000000008ea3cf107ae0dec57f03fe8
2020-02-06T04:38:33Z [] Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2020-02-06T04:38:33Z [] Using RdSeed as additional entropy source
```
#### this PR d76894987d0277e8011932ab7dfd77c537f8ea6e
```bash
checking for thread_local support... yes
...
src/bitcoind -logthreadnames=1
2020-02-06T04:17:49Z [net] net thread start
2020-02-06T04:17:49Z [opencon] opencon thread start
2020-02-06T04:17:49Z [dnsseed] dnsseed thread start
2020-02-06T04:17:49Z [init] init message: Done loading
2020-02-06T04:17:49Z [msghand] msghand thread start
2020-02-06T04:17:49Z [addcon] addcon thread start
...
2020-02-06T04:17:54Z [init] tor: Thread interrupt
2020-02-06T04:17:54Z [init] Shutdown: In progress...
```
From the [Xcode 8 release notes](https://developer.apple.com/library/archive/releasenotes/DeveloperTools/RN-Xcode/Chapters/Introduction.html#//apple_ref/doc/uid/TP40001051-CH1-SW78)
> C++ now supports the thread_local keyword, which declares thread-local storage (TLS) and supports C++ classes with non-trivial constructors and destructors. (9001553)
ACKs for top commit:
jonasschnelli:
Tested ACK d76894987d0277e8011932ab7dfd77c537f8ea6e
nijynot:
ACK d768949
hebasto:
ACK d76894987d0277e8011932ab7dfd77c537f8ea6e
Tree-SHA512: 48f3e4104b80bd7b6aedcef10bb1957b073530130f33af7c5cb59e876ac3f5480e53d7af1c0b226d809fe9eef1add3d6c3fb6de4af174966202c6030060ea823
3ab18246254019896132d1cdb8af2dcdb213ec3b build: Use dnl for all comments in configure.ac, rather than # (fanquake)
8ddcbb4e41fa91e7f80efe6d9c4d5e9bb1355036 build: Remove backticks from configure.ac (fanquake)
Pull request description:
Use `dnl` for all comments, rather than `#`.
Remove backticks - Their usage for the `bdb_prefix` and `qt5_prefix` commands may have improved backwards compatibility in some cases, however we now require recent versions of macOS. I'm not sure why they were being used in the `HAVE_STD__SYSTEM` and `HAVE_WSYSTEM` defines.
ACKs for top commit:
dongcarl:
ACK 3ab18246254019896132d1cdb8af2dcdb213ec3b
hebasto:
ACK 3ab18246254019896132d1cdb8af2dcdb213ec3b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2bcffb52c365acff87a0e6b9527ae31f36fdabb7ea095a8fd261f9a39b2c2848f5dfc148bc38d21e21e7bd761b1a2960e9a96f508c66be84d9569b8a401e812a
