fa1b3abc834a90fb1cfbd5ac63deb28f3990c1fb ci: Log qa-assets repo last commit (MarcoFalke)
fa22966f3307e22b77ea386e1abb60bf8c606170 fuzz: Print error message when FUZZ is missing (MarcoFalke)
Pull request description:
Some trivial UX improvements.
* Change the exit code for `PRINT_ALL_FUZZ_TARGETS_AND_ABORT` and `WRITE_ALL_FUZZ_TARGETS_AND_ABORT` to `EXIT_SUCCESS` instead of `Aborted (core dumped)`.
* Print readable error message when `FUZZ` is missing instead of `Aborted (core dumped)`.
* Clarify that a fuzz target needs to be compiled into the executable.
ACKs for top commit:
dergoegge:
ACK fa1b3abc834a90fb1cfbd5ac63deb28f3990c1fb
Tree-SHA512: 065ef8920449c64b3516f89a61cb397b505eccf531318c4f3830895d5ff6cd7ae2525cb857320481e3d0ed0b2f8a522cd8f7835e69f021241b6ec297a6102fc8
5228223e1ff2af29e6e77668ce3288005c2adbbc ci: remove MSAN getrandom syscall workaround (fanquake)
d5e06919db5e221bfef445c5a40c88de72dc5869 random: switch to using getrandom() directly (fanquake)
c2ba3f5b0c7d0eece7d16d1ffc125d8a6a9297af random: add [[maybe_unused]] to GetDevURandom (fanquake)
c13c97dbf846cf0e6a5581ac414ef96a215b0dc6 random: getentropy on macOS does not need unistd.h (fanquake)
Pull request description:
This requires a linux kernel of `3.17`+, which seems entirely
reasonable. `3.17` went EOL in 2015, and the last supported `3.x` kernel
(`3.16`) went EOL > 4 years ago, in 2020. For reference, the current
oldest maintained kernel is `4.14` (released 2017, going EOL Jan 2024).
Support for `getrandom()` (and `getentropy()`) was added to
glibc `2.25` https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html:
> * The getentropy and getrandom functions, and the <sys/random.h> header
file have been added.
and we already require `2.27` or later.
All that being said, I don't think you would encounter a current day (+~6 months from now)
system, running with kernel headers older than 3.17 (released 2014) but also having a
glibc of 2.27+ (released 2018)?
Removing this (our only) use of `syscall()` also means we can drop a workaround in our MSAN jobs.
If this is merged, I'll drop the [same workaround in oss-fuzz](25946a5448/projects/bitcoin-core/build.sh (L49-L56)).
ACKs for top commit:
josibake:
ACK 5228223e1f
hebasto:
ACK 5228223e1ff2af29e6e77668ce3288005c2adbbc, I've tested build system changes on Ubuntu 22.04 and macOS Monterey 12.6.6 (x86_64).
Tree-SHA512: cc978e08510c461b875ca8c08ae176b4519fa1108f0efd74dcb7474518945357e0184e54423282c9a496de195e4ddc3e221ee78623bd63e24c50cc86acdf32e2
c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 test: Explicitly specify directory where to search tests for (Hennadii Stepanov)
Pull request description:
For out-of-source builds, the `test/functional/test_runner.py` is supposed to be run from the build directory which allows it to pick the `test/config.ini` file generated by the build system. Currently, it works accidently for the following reasons:
- on POSIX systems, when running a created by Autoconf symlink to the `test/functional/test_runner.py` in the source directory, it actually has the source directory location in the `sys.path`.
- on Windows (the `build_msvc` directory) VS project puts and copies every build artifact into the source tree (which is wrong and ugly).
This PR makes `test/functional/test_runner.py` work from a build directory in any form (a symbolic link, a hard link, a copy) on _all_ supported platforms, which is highly desirable in the upcoming [CMake-based build system](https://github.com/bitcoin/bitcoin/pull/25797).
For the current master branch, this PR has no behaviour change.
Required for https://github.com/hebasto/bitcoin/pull/15.
---
**Steps to reproduce the issue**
While the issue is mostly specific to Windows and CMake builds, it is still possible to reproduce it on the current master branch.
1. Make an out-of-source build:
```
$ ./autogen.sh
$ mkdir ../build && cd ../build
$ ../bitcoin/configure
$ make
```
2. Note that Autoconf created a symbolic link `test/functional/test_runner.py` in the `../build` directory:
```
$ ls -l test/functional/test_runner.py
lrwxrwxrwx 1 hebasto hebasto 47 May 5 17:40 test/functional/test_runner.py -> ../../../bitcoin/test/functional/test_runner.py
```
which works flawlessly.
3. However, replacing this symbolic link with a hard link or a copy of `test/functional/test_runner.py` from the source tree will cause the following error:
```
$ cp ../bitcoin/test/functional/test_runner.py test/functional/test_runner.py
$ ls -l test/functional/test_runner.py
$ ./test/functional/test_runner.py
Temporary test directory at /tmp/test_runner_₿_🏃_20230505_175104
Running Unit Tests for Test Framework Modules
E
======================================================================
ERROR: test_framework (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_framework
Traceback (most recent call last):
File "/usr/lib/python3.10/unittest/loader.py", line 154, in loadTestsFromName
module = __import__(module_name)
ModuleNotFoundError: No module named 'test_framework'
----------------------------------------------------------------------
Ran 1 test in 0.000s
FAILED (errors=1)
Early exiting after failure in TestFramework unit tests
```
ACKs for top commit:
stickies-v:
re-ACK c44f3f2319
MarcoFalke:
lgtm ACK c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 💸
Tree-SHA512: 622ff629080a55f76dd4c1dab6699de0e9f06b75da3315cd3b31b972ef4bde746458bf3e8a95e879b3c6a63be2368af70005a83f6a3c85c4f1ba5be51e91a61d
fa5831bd6f940c4afb43ff625ba4fa6c641e999a build: Do not define `ENABLE_ZMQ` when ZMQ is not available (Hennadii Stepanov)
Pull request description:
A new behavior is consistent with the other optional dependencies.
The source code contains `#if ENABLE_ZMQ` lines only:
```
$ git grep ENABLE_ZMQ -- src/*.cpp
src/init.cpp:#if ENABLE_ZMQ
src/init.cpp:#if ENABLE_ZMQ
src/init.cpp:#if ENABLE_ZMQ
src/init.cpp:#if ENABLE_ZMQ
src/init.cpp:#if ENABLE_ZMQ
```
Change in description line -- "Define to 1..." --> "Define this symbol.." -- is motivated by the fact that the actual value of the defined `ENABLE_ZMQ` macro does not matter at all.
Related to:
- https://github.com/bitcoin/bitcoin/issues/16419
- https://github.com/bitcoin/bitcoin/pull/25302
ACKs for top commit:
TheCharlatan:
ACK fa5831bd6f940c4afb43ff625ba4fa6c641e999a
jarolrod:
ACK fa5831bd6f940c4afb43ff625ba4fa6c641e999a
Tree-SHA512: 5e72ff0d34c4b33205338daea0aae8d7aa0e48fd633e21af01af32b7ddb0532ef68dd3dd74deb2c1d2599691929617e8c09676bcbaaf7d669b88816f866f1db2
6a936580d1c42576f627d5fac5423ec7af88e547 ci: remove RUN_SECURITY_TESTS (fanquake)
Pull request description:
We no-longer run any security/symbol checks in the CI, and doubt we will in future (if we do, it'll be via Guix, where this var would be redundant in any case). The CI environment doesn't (exactly) match the release build environment (and is semi-regularly changing), and the binaries produced in the CI don't match how we build release binaries, so there is no point trying to run these checks, especially as we add more involved tests, i.e #26953.
ACKs for top commit:
josibake:
code review ACK 6a936580d1
TheCharlatan:
ACK 6a936580d1c42576f627d5fac5423ec7af88e547
Tree-SHA512: c0eec61a4b873bac487ba9321b50116a215b4796bd7d416d98ffcd09969dbf635c2cb5aeb225c89d1e6462838fa2a48565048ebe730f48d76d3db46b64855a91
98ea79841177cf7492ec1de1ba1979f75390c63b ci, iwyu: Double maximum line length for includes (Hennadii Stepanov)
Pull request description:
This PR makes the IWYU output in the CI 'tidy' task more useful by avoiding most cases where a comment ends with an ellipsis like that:
```
#include "primitives/transaction.h" // for CTxIn, CMutableTransaction, CTra...
```
ACKs for top commit:
TheCharlatan:
ACK 98ea79841177cf7492ec1de1ba1979f75390c63b
Tree-SHA512: 25195ccb2095884b23586416b86999ebc42577c6d777abdbd176a704fa75c64deb91fa61cd91d570a5408dd459e930e53bc70d963b76c73fca7a800e74b1bdbf
This requires a linux kernel of 3.17.0+, which seems entirely
reasonable. 3.17 went EOL in 2015, and the last supported 3.x kernel
(3.16) went EOL > 4 years ago, in 2020. For reference, the current
oldest maintained kernel is 4.14 (released 2017, EOL Jan 2024).
Support for `getrandom()` (and `getentropy()`) was added to
glibc 2.25, https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html,
and we already require 2.27+.
All that being said, I don't think you would encounter a current day
system, running with kernel headers older than 3.17 (released 2014) but
also having a glibc of 2.27+ (released 2018).
Remove it. Make this change, so in a future commit, we can
combine #ifdefs, and avoid duplicate <sys/random.h> includes once we
switch to using getrandom directly.
Also remove the comment about macOS 10.12. We already require macOS >
10.15, so it is redundant.
4bfcbbfd4a9749f7954e37a7447b5f047465bdf8 doc: remove Security section from build-unix.md (fanquake)
Pull request description:
Our compile documentation isn't the right place for generic binary hardening notes, which are neither particularly Bitcoin-Core specific, or as relevant as they might have once been, i.e non-executable stacks are now just the norm.
Just remove the notes for now, if someone has something more interesting/Bitcoin Core specific, it could be added in separate documentation in the future (maybe into the devwiki or similar).
Split from https://github.com/bitcoin/bitcoin/pull/27685#discussion_r1196517868.
ACKs for top commit:
jarolrod:
ACK 4bfcbbfd4a9749f7954e37a7447b5f047465bdf8
Tree-SHA512: 01b523ba40353d6cafb5dbd6540b514d83a2dc4e462a068fb390ca7de45b78e4a329367f70527f1824006ebe43f8caeea4ad05ec60556d5a5bd0143e27bf6581
Since the kernel library no longer depends on the system file, move it
to the common library instead in accordance to the diagram in
doc/design/libraries.md.
With the previous move of AlertNotify out of the validation file, and
thus out of the kernel library, ScheduleBatchPriority is the last
remaining function used by the kernel library from util/system. Move it
to its own file, such that util/system can be moved out of the util
library in the following few commits.
Moving util/system out of the kernel library removes further networking
as well as shell related code from it.
This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.
The DoWarning and AlertNotify functions are moved out of the
validation.cpp file, which removes its dependency on interface_ui as
well as util/system.
This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the
following few commits. By removing interface_ui from the kernel library,
its dependency on boost is reduced to just boost::multi_index.
This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.
This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.
Define a new kernel notification class with virtual methods for
notifying about internal kernel events. Create a new file in the node
library for defining a function creating the default set of notification
methods such that these do not need to be re-defined all over the
codebase. As a first step, add a `blockTip` method, wrapping
`uiInterface.NotifyBlockTip`.
6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965 [fuzz] Add MiniMiner target + diff fuzz against BlockAssembler (glozow)
3f3f2d59ea2946a7b7cc8cb0222fb602d62645d0 [unit test] GatherClusters and MiniMiner unit tests (glozow)
59afcc83548ea67a863dac7b75d000bc8f6a7023 Implement Mini version of BlockAssembler to calculate mining scores (glozow)
56484f0fdc44261e723563f59df886d5acdd851f [mempool] find connected mempool entries with GatherClusters(…) (glozow)
Pull request description:
Implement Mini version of BlockAssembler to calculate mining scores
Run the mining algorithm on a subset of the mempool, only disturbing the
mempool to copy out fee information for relevant entries. Intended to be
used by wallet to calculate amounts needed for fee-bumping unconfirmed
transactions.
From comments of sipa and glozow below:
> > In what way does the code added here differ from the real block assembly code?
>
> * Only operates on the relevant transactions rather than full mempool
> * Has the ability to remove transactions that will be replaced so they don't impact their ancestors
> * Does not hold mempool lock outside of the constructor, makes copies of the entries it needs instead (though I'm not sure if this has an effect in practice)
> * Doesn't do the sanity checks like keeping weight within max block weight and `IsFinalTx()`
> * After the block template is built, additionally calculates fees to bump remaining ancestor packages to target feerate
ACKs for top commit:
achow101:
ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
Xekyo:
> ACK [6b605b9](6b605b91c1) modulo `miniminer_overlap` test.
furszy:
ACK 6b605b91 modulo `miniminer_overlap` test.
theStack:
Code-review ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
Tree-SHA512: f86a8b4ae0506858a7b15d90f417ebceea5038b395c05c825e3796123ad3b6cb8a98ebb948521316802a4c6d60ebd7041093356b1e2c2922a06b3b96b3b8acb6
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return by default
immature coinbase.
b8ed95127b23873db973b2ef4f68d9f04e9c23ae msvc: Provide `ObjectFileName` explicitly (Hennadii Stepanov)
Pull request description:
This PR is a follow-up to https://github.com/bitcoin/bitcoin/pull/26715.
Fixes intermittent MSVC link [errors](https://cirrus-ci.com/task/6646912535756800).
ACKs for top commit:
sipsorcery:
ACK b8ed95127b23873db973b2ef4f68d9f04e9c23ae.
Tree-SHA512: 4319ecf61b578ce66d240998d089b9bb0a42f89dcfcb1a73f648cd3915f566c773721dcff1feba27d393a743d121334ccb890b1a519173e35a156d6135821ef4
fa4c16b186a34f4172deda617166813c8cb92c59 test: Add test to check tx in the last block can be downloaded (MarcoFalke)
fadc8490ab4fdd91aaf446e08108657b304a3ded test: Split up test_notfound_on_unannounced_tx test case (MarcoFalke)
Pull request description:
If a peer received an `inv` about a transaction, which was included in a block before receiving the corresponding `getdata`, it can be beneficial to send this transaction to the peer to aid compact block relay.
Add a test for this to avoid breaking it in the future.
ACKs for top commit:
sdaftuar:
ACK fa4c16b186a34f4172deda617166813c8cb92c59
instagibbs:
ACK fa4c16b186a34f4172deda617166813c8cb92c59
Tree-SHA512: 1ec16dcc216dd29c849928e753158d45c409612e9ac528db16e5af465bb5b69cd42241ac6f67e5a4583b8e558eeba49fa0a0889a4247b3fb0053b2758eec9490
b53cab0083d99e9610d74517d1d41fc615953770 build: Detect USDT the same way how it is used in the code (Hennadii Stepanov)
Pull request description:
In the code we do not use string literals.
Also a check for `DTRACE_PROBE7` macro has been added as not all systems define`DTRACE_PROBE{6,7,8,9,10,11,12}` macros (e.g., FreeBSD).
ACKs for top commit:
0xB10C:
ACK b53cab0083d99e9610d74517d1d41fc615953770
Tree-SHA512: 74f49424d57bf1929f2b09edba1449cef5a1a2448161952da35302343f3003d5bedeab1417e166b656c5f629303e2de888550b1219e886a1b991b12b9c880794
fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae build: Bump minimum supported GCC to g++-9 (MarcoFalke)
fa69955e741dd60dc6160e81cf223bbecd286806 ci: Bump centos:stream8 to centos:stream9 (MarcoFalke)
fa6a755d9fb22cad3d7063b21a1c8a137ae981b2 ci: Document the false positive error for g++-9 (MarcoFalke)
Pull request description:
It is a bit frustrating to write valid C++ code only to realize that g++-8 fails to parse it later on.
The only non-EOL operating system still shipping with g++-8 is CentOS Stream 8. I think it is reasonable for users of affected Linux distributions to:
* Upgrade their operating system, or compiler to a supported version.
* Alternatively, stay with a previous release of Bitcoin Core as long as it is supported.
Fixes https://github.com/bitcoin/bitcoin/issues/27537
ACKs for top commit:
hebasto:
ACK fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae
fanquake:
ACK fa953f15bfcf95df9aa9c91e1c4b56a205f4d1ae
Tree-SHA512: b9cf7e763d3071e1e008c5010de19601d4773afe46d58cf869d3f59285c53240c739a1cd7235a5525ede1bbdf6b6cb6fb091c8fc314864a28d5b27a400bb7632
69d43905b7f1d4849dfaea1b5744ee473ccc8744 test: add coverage for wallet read write db deadlock (furszy)
12daf6fcdcbf5c7f03038338d843df3877714b24 walletdb: scope bdb::EraseRecords under a single db txn (furszy)
043fcb0b053eee6021cc75e3d3f41097f52698c0 wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor (furszy)
Pull request description:
Decoupled from #26644 so it can closed in favor of #26715.
Basically, with bdb, we can't make a write operation while we are traversing the db with the same db handler. These two operations are performed in different txn contexts and cause a deadlock.
Added coverage by using `EraseRecords()` which is the simplest function that executes this process.
To replicate it, need bdb support and drop the first commit. The test will run forever.
ACKs for top commit:
achow101:
ACK 69d43905b7f1d4849dfaea1b5744ee473ccc8744
hebasto:
re-ACK 69d43905b7f1d4849dfaea1b5744ee473ccc8744
Tree-SHA512: b3773be78925f674e962f4a5c54b398a9d0cfe697148c01c3ec0d68281cc5c1444b38165960d219ef3cf1a57c8ce6427f44a876275958d49bbc0808486e19d7d
This is a change in behavior so that if for some reason we request a block from a peer, we don't allow an unsolicited CMPCT_BLOCK announcement for that same block to cause a request for a full block from the uninvited peer (as some type of request is already outstanding from the original peer)
faf4315c88d8c81c2ff84870bc81aef3cf719816 test: Return dict in MiniWallet::send_to (MarcoFalke)
Pull request description:
Returning a tuple has many issues:
* If only one value is needed, it can not be indexed by name
* If another value is added to the return value, all call sites need to be updated
Bite the bullet now and update all call sites to fix the above issues.
ACKs for top commit:
brunoerg:
crACK faf4315c88d8c81c2ff84870bc81aef3cf719816
theStack:
Code-review ACK faf4315c88d8c81c2ff84870bc81aef3cf719816
stickies-v:
Code review ACK faf4315c88d8c81c2ff84870bc81aef3cf719816
Tree-SHA512: 8ce1aca237df21f04b3990d0e5fcb49cc408fe6404399d3769a64eae1b5218941157d9785fce1bd9e45140cf70e06c3aa42646ee8f7b57855beb784fc3ef0261
Our compile documentation isn't the right place for genric binary
hardening notes, which are neither particularly Bitcoin-Core specific,
or as relevant as they might have once been, i.e non-executable stacks
are now just the norm.
Just remove the notes for now, if someone has
something more interesting/Bitcoin Core specific, it could be added in
separate documentation in the future (maybe into the devwiki or
similar).
Split from
https://github.com/bitcoin/bitcoin/pull/27685#discussion_r1196517868.
fa29651c3ff3e13a42d6505b14971265562f088a doc: Rework build-unix.md (MarcoFalke)
Pull request description:
The doc has many issues:
* The fist section contains outdated non-existing and confusing configure flags like `--enable-cxx` and `--disable-shared`, as well as edge-case expert options such as `BDB_PREFIX`. Fix that by removing the section and adding notes elsewhere, if applicable.
* There are links to the depends system before instructions on how to simply build from system packages. Fix that by moving that later.
* Also, remove sections that are duplicate with other depends READMEs.
ACKs for top commit:
fanquake:
ACK fa29651c3ff3e13a42d6505b14971265562f088a
TheCharlatan:
ACK fa29651c3ff3e13a42d6505b14971265562f088a
Tree-SHA512: 5348ddf3fa094c630d80b63033ca7b40ec0356427856f9a1075b31244f6bf8ec65cb2a738366e1174ef2fe7e0bf5cc249a62c58f458bbaf50668aceeac954820
a94d75fa81bab8f4695ab1756524e639af0ff69c msvc: Do not define `HAVE_CONSENSUS_LIB` (Hennadii Stepanov)
cf6ff1031bd577c18fe8eb2dd168a6d0d1c16e0d msvc: Clean up `libbitcoin_consensus` source files (Hennadii Stepanov)
30aee016f15b43b90efb023d9bd04aa84cf58ade scripted-diff: Rename `libbitcoinconsensus` to `libbitcoin_consensus` (Hennadii Stepanov)
Pull request description:
The current Autotools-based build system operates with two build artifacts:
- [`LIBBITCOIN_CONSENSUS`](3777c75d14/src/Makefile.am (L31)) which is [defined as](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) "Stable, backwards-compatible consensus functionality used by _libbitcoin_node_ and _libbitcoin_wallet_"
- [`LIBBITCOINCONSENSUS`](3777c75d14/src/Makefile.am (L42)) which is [defined as](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) "Shared library build of static _libbitcoin_consensus_ library"
The way how the `libbitcoinconsensus.vcxproj` project is used in the MSVC build system obviously shows that it is the former use case.
This PR makes the related adjustments to the MSVC build system.
ACKs for top commit:
sipsorcery:
ACK a94d75fa81bab8f4695ab1756524e639af0ff69c.
Tree-SHA512: 1144e13ee2b428ce14be8f76729744830c502a07814eb03e2aa6b8e009d8936fd13743e3f36ef3f31fac0e3979eb9af23e6a1364f151df49b3ae18dbd14cbf99
7014e080154f9c82e4fbe043af292a1e761cdb55 doc: remove mention of glibc 2.10+ (fanquake)
Pull request description:
We already require glibc 2.27+, so mentioning a much older version here is redundant.
ACKs for top commit:
TheCharlatan:
ACK 7014e080154f9c82e4fbe043af292a1e761cdb55
Tree-SHA512: 883a566a80cabe34bfb5d902990f3eca08d0e11438e6c128d311e558f373ec232b0934deb85d12d796baacfeae590af8c73aa1b2faef07f27ffa9011270ffd96
