babb9f5db64145a4adec07dee1c8328195363777 depends: remove non-native libmultiprocess build (Cory Fields)
5d105fb8c3ffa39c3e716c3147ee74795b129113 depends: Switch libmultiprocess packages to use local git subtree (Ryan Ofsky)
9b35518d2f3ff67cbc87d30bd3b882fe72fd5d87 depends, moveonly: split up int_get_build_id function (Ryan Ofsky)
2d373e27071f54e04f8aaf6753e5481ccbc9ed3f lint: Add exclusions for libmultiprocess subtree (Ryan Ofsky)
e88ab394c163a8ebe59ee240b09430d788ef274e doc: Update documentation to explain libmultiprocess subtree (Ryan Ofsky)
d4bc5639829f0c98bd2faeddba9be4e8c6667875 cmake: Fix clang-tidy "no input files" errors (Ryan Ofsky)
abdf3cb6456f3f79b3130d2001bf780deadccf87 cmake: Fix warnings from boost headers (Ryan Ofsky)
8532fcb1c30d6556c273bd77a2a7daa6b0a2448d cmake: Fix ctest mptest "Unable to find executable" errors (Ryan Ofsky)
d597ab1dee6b3541c827e6be61fc8a9b61d1e23d cmake: Support building with libmultiprocess subtree (Ryan Ofsky)
69f0d4adb72c1dc5c261517899d87b0fe8b88304 scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake (Ryan Ofsky)
a2f28e4be96e92079a219567cf20214996aefc53 Squashed 'src/ipc/libmultiprocess/' content from commit 35944ffd23fa (Ryan Ofsky)
d6244f85c509d7644a57def892298a4dde988c3b depends: Update libmultiprocess library to simplify cmake subtree build (Ryan Ofsky)
Pull request description:
This adds the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library and code generator as a subtree in `src/ipc/libmultiprocess` and allows it to be built with the cmake `-DENABLE_IPC` option, which is disabled by default.
This PR does not entirely remove the depends system [libmultiprocess package](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/native_libmultiprocess.mk) because the package is useful when cross compiling. (A cross-compiling cmake build cannot easily build and run a native code generation tool.) However, it does update the depends package to build from the new git subtree, instead of being downloaded separately from github, so the same sources are used to build both the runtime library and the code generator.
This PR includes the following manual changes (not created automatically with `git subtree add`) which just update the build system and documentation:
- [`d6244f85c509` depends: Update libmultiprocess library to simplify cmake subtree build](d6244f85c5)
- [`69f0d4adb72c` scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake](69f0d4adb7)
- [`d597ab1dee6b` cmake: Support building with libmultiprocess subtree](d597ab1dee)
- [`8532fcb1c30d` cmake: Fix ctest mptest "Unable to find executable" errors](8532fcb1c3)
- [`abdf3cb6456f` cmake: Fix warnings from boost headers](abdf3cb645)
- [`d4bc5639829f` cmake: Fix clang-tidy "no input files" errors](d4bc563982)
- [`e88ab394c163` doc: Update documentation to explain libmultiprocess subtree](e88ab394c1)
- [`2d373e27071f` lint: Add exclusions for libmultiprocess subtree](2d373e2707)
- [`9b35518d2f3f` depends, moveonly: split up int_get_build_id function](9b35518d2f)
- [`5d105fb8c3ff` depends: Switch libmultiprocess packages to use local git subtree](5d105fb8c3)
- [`babb9f5db641` depends: remove non-native libmultiprocess build](babb9f5db6)
---
Previous minisketch subtree PR #23114 may be useful for comparison
Instructions for subtree verification can be found:
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees
- https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh
TL&DR:
```sh
git remote add --fetch libmultiprocess https://github.com/chaincodelabs/libmultiprocess.git
test/lint/git-subtree-check.sh -r src/ipc/libmultiprocess
```
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
Sjors:
re-ACK babb9f5db64145a4adec07dee1c8328195363777
TheCharlatan:
tACK babb9f5db64145a4adec07dee1c8328195363777
vasild:
ACK babb9f5db64145a4adec07dee1c8328195363777
Tree-SHA512: 43d4eecca5aab63e55c613de935965666eaced327f9fe859a0e9c9b85f7685dc16c5c8d6e03e09ca998628c5d468633f4f743529930b037049abe8e0101e0143
b96f1a696aa792068b5a4fa16e2d4a342e4f55b8 add clang/llvm based coverage report generation (Prabhat Verma)
Pull request description:
Followed up from the [comment](https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674522975) on the issue [#31927](https://github.com/bitcoin/bitcoin/issues/31927) , issues have been observed building coverage reports with `gcov` in MacOs and NixOs. This PR adds the steps to generate a coverage report based on the default llvm/clang tooling.
ACKs for top commit:
Crypt-iQ:
tACK b96f1a696aa792068b5a4fa16e2d4a342e4f55b8
hodlinator:
re-ACK b96f1a696aa792068b5a4fa16e2d4a342e4f55b8
janb84:
Re ACK [b96f1a6](b96f1a696a)
Tree-SHA512: bc54f170e84bb76b3eba7285bd49f051c0b99b784d583a550d8e51511497bcc4df8964bbe3991777648d2f829809db8eabb0cbf0d25f9da5e49e1cfc62f6d8d0
aa7a898c236eb519aaf546afee2b9c2b6dfdea1f doc: use testnet4 in developer docs (Sjors Provoost)
6c217d22fdc978cac0f970cf2296a9fa1e00ee97 test: use testnet4 in argsman test (Sjors Provoost)
7c200ece80575d399a552f5757c07ac2c8c7ec6c test: use testnet4 in key_io_valid.json (Sjors Provoost)
d424bd59413c8ffc7a263635e5b9882497d39fed test: drop unused testnet3 magic bytes (Sjors Provoost)
8cfc09fafe59adae4904a21589736de93a00ad2d test: cover testnet4 magic in assumeutxo.py (Sjors Provoost)
4281e3603a2eadefc8535b863128a05cf3a5a75f zmq: use testnet4 in zmq_sub.py example (Sjors Provoost)
Pull request description:
In preparation for dropping testnet3 entirely in #31974 this PR migrates a few things to testnet4:
* the ZMQ examples
* developer docs
* various unit tests
* the snapshot magic byte check in `feature_assumeutxo.py`
It drops `testnet3` from `MAGIC_BYTES` in the test framework, since no test uses it.
ACKs for top commit:
fjahr:
re-ACK aa7a898c236eb519aaf546afee2b9c2b6dfdea1f
maflcko:
lgtm ACK aa7a898c236eb519aaf546afee2b9c2b6dfdea1f 🔊
hodlinator:
re-ACK aa7a898c236eb519aaf546afee2b9c2b6dfdea1f
Tree-SHA512: 235f74273234e8fb2aedf0017dea5c16bb9813ec7a1f89a51abe85691f09830a5ead834115d7db0936e12e55a40bc81888856a8002fe507c1474407e77f8b9fb
329a0dcdafe05002f662e8737a76bfdeaba9a3ed doc: clarify the documentation of `Assume` (ismaelsadeeq)
Pull request description:
An Expression inside `Assume` may be optimized away in production builds when the compiler proves they are side-effect-free.
This use case is demonstrated in #31363 and is suggested to be documented in https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2736410023.
ACKs for top commit:
l0rinc:
ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed
hodlinator:
re-ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed
jonatack:
ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed
rkrux:
re-ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed
Tree-SHA512: 4bbb807a1e632694863c1a1fa2e93cc5a756b19f8d78f0642ebe7ffafb01835765fa66c76a680dc6f3c412a5abb0c4a33fb7212c26b4b2d80b6b3b7ee8284b2e
This uses a macro, which can be a bit more brittle than an alias
template. However, class template argument deduction for alias templates
is only implemented in clang-19.
568fcdddaec2cc8decba5a098257f31729cc1caa scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e96919603af829d0b677779a234a0f6e cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](e146c1867e/lldb/cmake/modules/LLDBStandalone.cmake (L128-L130)):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
ryanofsky:
Code review ACK 568fcdddaec2cc8decba5a098257f31729cc1caa. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
theuni:
ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
This pull request fixes a word order error in developer-notes.md.
Before:
"In cases where do you call .c_str(), you might want to additionally check that the string does not contain embedded '\0' characters..."
After:
"In cases where you do call .c_str(), you might want to additionally check that the string does not contain embedded '\0' characters..."
Explanation:
The sentence had incorrect word order, making it grammatically incorrect. Rearranging "do you" to "you do" corrects the sentence, improving the readability and clarity of the documentation.
replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes
replaced --enable-multiprocess with -DWITH_MULTIPROCESS=ON
replaced --disable-zmq with -DWITH_ZMQ=OFF
0b003e1ff7e09b56cd013b15f1998495994b7211 docs: Updated debug build instructions for cmake (ion-)
Pull request description:
In the [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md) the section on [compiling for debug](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-debugging) displays outdated instructions that were applicable before the move to cmake build system.
This PR just gives instructions on how to build for debugging in the context of cmake.
ACKs for top commit:
kevkevinpal:
ACK [0b003e1](0b003e1ff7)
achow101:
ACK 0b003e1ff7e09b56cd013b15f1998495994b7211
tdb3:
ACK 0b003e1ff7e09b56cd013b15f1998495994b7211
Tree-SHA512: fc8b4824d68e47b3e64320b4ce728df4242d00c2e8c75dcf5daa60af3050c62d94fa9741e447c7d81e72194470ed68a4e19bbee8cf6a1412d6566c7b699cf242
6a68343ffbf3291eb243d90c00df50e672ff3944 doc: Prepend 'build/' to binary paths under 'src/' in docs (Lőrinc)
91b3bc2b9c572d9244e1cf542655c1b32e87aaf7 doc: Update documentation generation example in developer-notes.md (Lőrinc)
Pull request description:
In [the other readmes](6ce50fd9d0/src/test/README.md (L19)) we've provided a default build directory instead, unified the `developer-notes.md` to specify it explicitly.
In the next commit I've used this default to go over each reference to our binaries and changed their in-source references to the build directory.
Some of these changes were in example outputs - I haven't validated that the outputs are still the same.
I haven't modified the build folders in the devtools.
ACKs for top commit:
maflcko:
review ACK 6a68343ffbf3291eb243d90c00df50e672ff3944
pablomartin4btc:
ACK 6a68343ffbf3291eb243d90c00df50e672ff3944
fanquake:
ACK 6a68343ffbf3291eb243d90c00df50e672ff3944 - we still need to followup with other scripts/devtools, and likely unify what we are doing in some way, but this is an improvement.
Tree-SHA512: 905d9c68cafe1e405e98d6aa089d7a36a34c9e03403df5c67ac2c9a98cfa54a0305b647cb92247dcb9f49e9b509a8ba88367392b95618c67059684c67b6c36fb
This supports lcov 2.x in the sense that we are no-longer hardcoding
version specific options, and instead will use the `LCOV_OPTS` variable
(which is the more correct/flexible thing to do in any case). It's also
quite likely that devs are already having to pass extra options to lcov
2.x, given it's more stringent in terms of coverage generation and error
checking. See this thread for an example:
https://github.com/linux-test-project/lcov/issues/238.
Added an example to the developer notes.
Tested on one machine (LCOV 2.0, gcc 13.2) with:
```bash
./autogen.sh
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
make
make cov
<snip>
Processing file src/netaddress.cpp
lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
Overall coverage rate:
lines......: 81.8% (79362 of 97002 lines)
functions......: 77.8% (10356 of 13310 functions)
branches......: 49.6% (130628 of 263196 branches)
```
and another machine (LCOV 2.1, Clang 18.1.3) with:
```bash
./autogen.sh
./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
make
make cov
<snip>
Processing file src/util/strencodings.cpp
lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
Overall coverage rate:
source files: 622
lines.......: 79.8% (70311 of 88132 lines)
functions...: 78.1% (13968 of 17881 functions)
branches....: 44.5% (157551 of 354317 branches)
Message summary:
101 warning messages:
count: 1
inconsistent: 100
3528 ignore messages:
inconsistent: 3528
```
e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615677d226c9f6b3f2b66d833315d40da logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
achow101:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
stickies-v:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
jamesob:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
These provide simple and clear ways to write the most common logging
operations:
LogInfo("msg");
LogDebug(BCLog::LogFlags::NET, "msg");
LogError("msg");
LogWarning("msg");
LogTrace(BCLog::LogFlags::NET, "msg");
For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
91d08889218e06631f43a3dab0bae576aa46e43c sync: unpublish LocksHeld() which is used only in sync.cpp (Vasil Dimov)
3df37e0c78c3d5139c963a74eda56c331355ef72 doc: clarify that LOCK() does AssertLockNotHeld() internally (Vasil Dimov)
Pull request description:
Constructs like
```cpp
AssertLockNotHeld(m);
LOCK(m);
```
are equivalent to (almost, modulo some logging differences, see below)
```cpp
LOCK(m);
```
for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. Requests to do the former keep coming during review process. `developer-notes.md` explicitly states "Combine annotations in function declarations with run-time asserts in function definitions", but that seems to be too strong or unclear. `LOCK()` is also a run-time assert in this case.
Also remove `LocksHeld()` from the public interface in `sync.h` since it is only used in `sync.cpp`.
ACKs for top commit:
achow101:
ACK 91d08889218e06631f43a3dab0bae576aa46e43c
hebasto:
ACK 91d08889218e06631f43a3dab0bae576aa46e43c, I have reviewed the code and it looks OK.
Tree-SHA512: c4b7ef2c0bfeb28d1c4f55f497810f629873137e02f5a92137c02cb1ff603ac76473dcd2171e594491494a5cb87b8c0c803e06b86f190d4acb231791e28e802d
The thread does not only load blocks, it loads the mempool and,
in a future commit, will start the indexes as well.
Also, renamed the 'ThreadImport' function to 'ImportBlocks'
And the 'm_load_block' class member to 'm_thread_load'.
-BEGIN VERIFY SCRIPT-
sed -i "s/ThreadImport/ImportBlocks/g" $(git grep -l ThreadImport -- ':!/doc/')
sed -i "s/loadblk/initload/g" $(git grep -l loadblk -- ':!/doc/release-notes/')
sed -i "s/m_load_block/m_thread_load/g" $(git grep -l m_load_block)
-END VERIFY SCRIPT-
