faf4315c88 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 faf4315c88
theStack:
Code-review ACK faf4315c88
stickies-v:
Code review ACK faf4315c88
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.
fa29651c3f 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 fa29651c3f
TheCharlatan:
ACK fa29651c3f
Tree-SHA512: 5348ddf3fa094c630d80b63033ca7b40ec0356427856f9a1075b31244f6bf8ec65cb2a738366e1174ef2fe7e0bf5cc249a62c58f458bbaf50668aceeac954820
a94d75fa81 msvc: Do not define `HAVE_CONSENSUS_LIB` (Hennadii Stepanov)
cf6ff1031b msvc: Clean up `libbitcoin_consensus` source files (Hennadii Stepanov)
30aee016f1 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 a94d75fa81.
Tree-SHA512: 1144e13ee2b428ce14be8f76729744830c502a07814eb03e2aa6b8e009d8936fd13743e3f36ef3f31fac0e3979eb9af23e6a1364f151df49b3ae18dbd14cbf99
7014e08015 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 7014e08015
Tree-SHA512: 883a566a80cabe34bfb5d902990f3eca08d0e11438e6c128d311e558f373ec232b0934deb85d12d796baacfeae590af8c73aa1b2faef07f27ffa9011270ffd96
This is required for the next commit. Also, drop CI_RETRY_EXE before
"dnf install", because it requires getopt, which will only be installed
later on via util-linux
fa3761d19d ci: Reduce tsan CPU and memory for faster scheduling (MarcoFalke)
aaaa07bc84 ci: Use credits for ARM task (MarcoFalke)
Pull request description:
After https://github.com/bitcoin/bitcoin/pull/27562 the task should finish in less than 10 minutes, so also using credits for it will be cheap and improve dev experience.
ACKs for top commit:
fanquake:
ACK fa3761d19d
Tree-SHA512: 98ba76eaa63bcbab674076bb9877b3a20d1b90e6d51e2fd7b97ae245414e984d734006b15ea60228a1f4f9bc72fbe2a1a8910fd05e7c52dd2a8b223bfaa25b50
fa199ee614 ci: Drop NO_WERROR=1 for clang-10 build (MarcoFalke)
fad2c200f4 build: Bump minimum Clang to clang-10 (MarcoFalke)
fad7cfee8d doc: Remove outdated CentOS comment (MarcoFalke)
Pull request description:
It doesn't make sense to support a minimum clang version that is difficult to install on all supported operating systems, which generally ship a later version:
* Ubuntu Focal 20.04: https://packages.ubuntu.com/focal/clang-10 and https://packages.ubuntu.com/focal/clang-12
* Debian Bullseye: https://packages.debian.org/bullseye/clang-13
* CentOS 8 Stream: All Clang versions from 11.0 to 15.0
Also, it allows to drop build code, which means it won't waste review when rolling over into cmake (`cmake/module/CheckStdFilesystem.cmake`).
ACKs for top commit:
hebasto:
ACK fa199ee614
fanquake:
ACK fa199ee614
Tree-SHA512: c1a0e8f191a6db866b8be3c9d254dc3f576fa021e2eaaeb68f3354554a8b38eaa90bbf9871ff92351b715e62a6b7b98cf94eba6dc53d7c951bddb6ad49ba7716
97844d9268 index: Enable reindex-chainstate with active indexes (Martin Zumsande)
60bec3c82d index: Use first block from locator instead of looking for fork point (Martin Zumsande)
Pull request description:
This makes two improvements to the index init phase:
**1) Prevent index corruption in case a reorg happens when the index was switched off**:
This is done by reading in the top block stored in the locator instead of looking for a fork point already in `BaseIndex::Init()`.
Before, we'd just go back to the fork point by calling `FindForkInGlobalIndex()`, which would have corrupted the coinstatsindex because its saved muhash needs to be reverted step by step by un-applying all blocks in between, which wasn't done before. This is now being done a bit later in `ThreadSync()`, which has existing logic to call the custom `Rewind()` method when going back along the chain to the forking point (thanks ryanofsky for pointing this out to me!).
**2) Allow using the `-reindex-chainstate` option without needing to disabling indexes**:
With `BaseIndex::Init()` not calling `FindForkInGlobalIndex()` anymore, we can allow `reindex-chainstate` with active indexes. `reindex-chainstate` deletes the chain and rebuilds it later in `ThreadImport`, so there is no chain available during `BaseIndex::Init()`, which would lead to problems (see #24789).
But now we'll only need the chain a bit later in `BaseIndex::ThreadSync`, which will wait for the reindex-chainstate in `ThreadImport` to finish and will continue syncing after that.
ACKs for top commit:
ryanofsky:
Code review ACK 97844d9268. Just simple rebase since last review
Tree-SHA512: e24973fc22e0b87a49026f4820aecb0a4e415f4d381bade9969dd31cf97afecfea0449dce7fcc797343b792199cc8287276d1f5ffa4433dcb54fb24a808db6fb
This is achieved by letting the index sync thread wait until
reindex-chainstate is finished.
This also disables the pruning check when reindexing the chainstate (which is
incompatible with prune mode) because there would be no chain at this point
in init.
The index sync code has logic to go back the chain to the forking point, while
also updating index-specific state, which is necessary to prevent
possible corruption of the coinstatsindex.
Also add a test for this (a reorg happens while the index is deactivated)
that would not pass before this change.
a09269a146 guix: document when certain guix patches can be dropped (fanquake)
Pull request description:
Additional notes for when patches can be dropped.
ACKs for top commit:
hebasto:
ACK a09269a146, I have reviewed the changes and they look OK.
jarolrod:
ACK a09269a146
Tree-SHA512: c1876b9a4e3cf73645d25c9077cef19a9b6b7fe2eda5dc9d82fd3ca3f9105453406c1b197e6635035b6ce19c9f255c070bebed5563f68913033d04627202155a
ddddf4957b ci: Run iwyu on all src files (MarcoFalke)
Pull request description:
This makes it easier to look at the CI output of a file without having to manually add it first to the list.
ACKs for top commit:
hebasto:
ACK ddddf4957b
Tree-SHA512: 342b52838ae45ea343731c30058cdd5595d5ea5601a1f396de4466ccdd63f7ab07b3a193df3669e4dca7cb535557dcc98f866b3cf986b98176b20ecead123868
We no-longer run any security/syymbol 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.
This partially reverts commit 71383f2fad.
This should be fine, because if warnings are issues again in the future,
it can be disabled again, along with a list of the false warnings.
fad09b703f ci: Remove unused errtrace trap ERR (MarcoFalke)
Pull request description:
This was added in commit 069752b726, presumably at a time when the functional tests wouldn't capture stderr.
Now that all tests capture and print stderr on failure, it can be removed. Reference:
* Unit tests capture via `2>&1`:
d7700d3a26/src/Makefile.test.include (L421)
* Functional tests capture as well:
d7700d3a26/test/functional/test_framework/test_node.py (L356)
ACKs for top commit:
fanquake:
ACK fad09b703f
hebasto:
ACK fad09b703f, tested on Ubuntu 22.04: I can still see warnings from the sanitizers in both unit and functional tests.
Tree-SHA512: 1e786eee432a7a50eb9f78b06b2b157321cc16f91b613e3b476e9e51572592fe4bcf4dc15df176e5f019f24497ac68cf332d2037b55b57498c93f4e19613163c
This incorrectly assumed num_blocks_total would be greater than 0. This is not guaranteed until the ConnectBlock call right below it.
The total and average metric is not very useful because it does not distinguish between blocks read from disk and those loaded from memory. So rather than fixing the divide by zero issue, we just drop the metric.
These should only be relevant for a glibc that is built as part of a
Guix system, and should not be required for a glibc that is just being
built to compile our binaries against. A x86_64 linux bitcoind produced
with Guix using master vs this change has no difference. i.e:
```diff
@@ -20311,15 +20311,15 @@
This is experimental software.
The source code is available from %s.
Please contribute if you find %s useful. Visit %s for further information about the software.
The %s developers
The Bitcoin Core developers
<https://bitcoincore.org/>
Copyright (C) %i-%i
-v25.99.0-gda0bf1d07639b0490791bbd6aec71bbea8aa2aThe %s developer<https://github.com/bitcoin/bitcDistributed under the MIT software license, see the accompanyingThis is experimeThe source code is available froPlease contribute if you find %s useful. Visit %s for further information about Copyright (C) %ibool BCLog::Logger::StartLogging()
+v25.99.0-gd7700d3a26478d9b1648463c188648c7047b1cThe %s developer<https://github.com/bitcoin/bitcDistributed under the MIT software license, see the accompanyingThis is experimeThe source code is available froPlease contribute if you find %s useful. Visit %s for further information about Copyright (C) %ibool BCLog::Logger::StartLogging()
std::string BCLog::Logger::LogLevelToStr(BCLog::Level) const
std::string LogCategoryToStr(BCLog::LogFlags)
void BCLog::Logger::LogPrintStr(const string&, const string&, const string&, int, BCLog::LogFlags, BCLog::Level)
void BCLog::Logger::ShrinkDebugFile()
Failed to shrink debug log file: fseek(...) failed
logging.cpp
m_buffering
```
```diff
@@ -1505889,15 +1505889,15 @@
call aa3380 <malloc@plt+0xa4edb0>
mov (%rsp),%rdx
movdqa 0x465540(%rip),%xmm0
mov %rax,0x7a0559(%rip)
lea 0x7a0552(%rip),%rsi
lea 0x3957bb(%rip),%rdi
mov %rdx,0x7a0554(%rip)
- mov $0x3038,%edx
+ mov $0x3036,%edx
movups %xmm0,(%rax)
movdqa 0x465524(%rip),%xmm0
mov %dx,0x30(%rax)
mov 0x7a0529(%rip),%rdx
movups %xmm0,0x10(%rax)
movdqa 0x46551d(%rip),%xmm0
movups %xmm0,0x20(%rax)
```
```diff
@@ -37238,17 +37238,17 @@
0x00b73730 65202573 20646576 656c6f70 65727300 e %s developers.
0x00b73740 54686520 42697463 6f696e20 436f7265 The Bitcoin Core
0x00b73750 20646576 656c6f70 65727300 434f5059 developers.COPY
0x00b73760 494e4700 3c687474 70733a2f 2f626974 ING.<https://bit
0x00b73770 636f696e 636f7265 2e6f7267 2f3e0043 coincore.org/>.C
0x00b73780 6f707972 69676874 20284329 2025692d opyright (C) %i-
0x00b73790 25690053 61746f73 68690000 00000000 %i.Satoshi......
- 0x00b737a0 7632352e 39392e30 2d676461 30626631 v25.99.0-gda0bf1
- 0x00b737b0 64303736 33396230 34393037 39316262 d07639b0490791bb
- 0x00b737c0 64366165 63373162 62656138 61613261 d6aec71bbea8aa2a
+ 0x00b737a0 7632352e 39392e30 2d676437 37303064 v25.99.0-gd7700d
+ 0x00b737b0 33613236 34373864 39623136 34383436 3a26478d9b164846
+ 0x00b737c0 33633138 38363438 63373034 37623163 3c188648c7047b1c
0x00b737d0 54686520 25732064 6576656c 6f706572 The %s developer
0x00b737e0 3c687474 70733a2f 2f676974 6875622e <https://github.
0x00b737f0 636f6d2f 62697463 6f696e2f 62697463 com/bitcoin/bitc
0x00b73800 44697374 72696275 74656420 756e6465 Distributed unde
0x00b73810 72207468 65204d49 5420736f 66747761 r the MIT softwa
0x00b73820 7265206c 6963656e 73652c20 73656520 re license, see
0x00b73830 74686520 6163636f 6d70616e 79696e67 the accompanying
```
```diff
@@ -1,5 +1,5 @@
Hex dump of section '.gnu_debuglink':
0x00000000 62697463 6f696e64 2e646267 00000000 bitcoind.dbg....
- 0x00000010 6b6e8eda kn..
+ 0x00000010 345cb865 4\.e
```
1b1ffbd014 Build: Log when test -f fails in Makefile (TheCharlatan)
541012e621 Build: Use AM_V_GEN in Makefiles where appropriate (TheCharlatan)
Pull request description:
This PR triages some behavior around Makefile recipe echoing suppression.
When generating new files as part of the Makefile the recipe is sometimes suppressed with $(AM_V_GEN) and sometimes with `@`. We should prefer $(AM_V_GEN), since this also prints the lines in silent mode. This is arguably more in style with the current recipe echoing.
Before:
`Generated test/data/script_tests.json.h`
Now:
` GEN test/data/script_tests.json.h`
A side effect of this change is that the recipe for generating build.h is now echoed on each make run. Arguably this makes its generation more transparent.
Sometimes the error emitted by `test -f` is currently thrown without any logging. This makes it a bit harder to debug. Instead, print a helpful log message to point the developer in the right direction.
Alternatively this could have been implemented by just removing the recipe echo suppression (@), but the subsequent make output became too noisy.
ACKs for top commit:
fanquake:
ACK 1b1ffbd014
Tree-SHA512: e31869fab25e72802b692ce6735f9561912caea903c1577101b64c9cb115c98de01a59300e8ffe7b05b998345c1b64a79226231d7d1453236ac338c62dc9fbb3
e9dcac1ec7 add `lief` to `spelling.ignore-words` (brunoerg)
258f93000b test: fix spelling in `interface_usdt_utxocache` (brunoerg)
Pull request description:
Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier)
ACKs for top commit:
fanquake:
ACK e9dcac1ec7
Tree-SHA512: f42cdbb74144c5d289d70bb9bac1179650bb32594678e15f4e18e4b2f68009999d60ff69494d4e68650d82fc1838e67515ed2f922ee84db98735ef906911ec40
0282b2126d walletdb: Remove unused CreateMockWalletDatabase (Andrew Chow)
Pull request description:
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.
I thought this was included in #26715, maybe it got lost in a rebase.
ACKs for top commit:
furszy:
utACK 0282b212
brunoerg:
crACK 0282b2126d
Tree-SHA512: 18445c4d8a4e2609ef7471c6d75297f43694b79e768f6c993a5addb1dc0e88bd12bac263c9d8e903d828ddf6bf50434f9e2f72048f4fc528e98fed8ee65123ca
so we erase all the records atomically or abort the entire
procedure.
and, at the same time, we can share the same db txn context
for the db cursor and the erase functionality.
extra note from the Db.cursor doc:
"If transaction protection is enabled, cursors must be
opened and closed within the context of a transaction"
thus why added a `CloseCursor` call before calling to
`TxnAbort/TxnCommit`.
If the batch ptr is not passed, the cursor will not use the db active
txn context which could lead to a deadlock if the code tries to modify
the db while it is traversing it.
E.g. the 'EraseRecords()' function.
