fa7b07571f test: Fix feature_init intermittent issues (MarcoFalke)
fa4595deb3 test: Remove random line number feature from feature_init.py (MarcoFalke)
Pull request description:
The test doesn't work currently because the log might be finalized before `wait_for_debug_log` is started, in which case it will assume the log is empty and fail to detect any line.
Fix this by calling `wait_for_debug_log` first. Fixes#24060.
Also, remove the "random line number" part of the test, because it doesn't really test anything novel. `wait_for_debug_log` is inherently racy, so will randomly terminate at the exact point or later. So the randomization is already sufficiently covered.
ACKs for top commit:
jamesob:
ACK fa7b07571f
mzumsande:
Code Review ACK fa7b07571f
Tree-SHA512: f107a04926270a1d9ac8f53b6e5254566d1827ba7d27fd74f9ce909319f8794f8bd46da9c42256bf41b6df26c0bad8b9a0d54e4e148695189af09f31e3948330
1111d33532 refactor: Make MessageBoxFlags enum underlying type unsigned (MarcoFalke)
Pull request description:
All values in the enum are unsigned. Also, flags shouldn't be treated as signed types. So clarify the underlying type and remove a sanitizer suppression.
ACKs for top commit:
hebasto:
ACK 1111d33532, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 48b16c4a0ace1a1e4d351d6eadadbb1bc42aef7fd82e24e3ea50c62f2c04a552ed21027158d09aa97e630c8c7d732cb150c38065333d7c2accbae46593b7ed9f
020acea99b refactor: replace RecursiveMutex m_chainstate_mutex with Mutex (w0xlt)
ddeefeef20 refactor: add negative TS annotations for `m_chainstate_mutex` (w0xlt)
1dfd31bc26 scripted-diff: rename m_cs_chainstate -> m_chainstate_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex m_cs_chainstate`.
`m_cs_chainstate` is only held in `ActivateBestChain()` and `InvalidateBlock()`.
So apparently there is no recursion involved, so the `m_cs_chainstate` can be a non-recursive mutex.
ACKs for top commit:
hebasto:
ACK 020acea99b, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK 020acea99b🌴
shaavan:
reACK 020acea99b
Tree-SHA512: c7c16e727e326df3410514915ce753a2a5e1da78857ef965ef683e36251e1b73c9cced4cd5231b04dbe2be0ea14084f6731b4d7a4d9a8e086e982b985e37e4b4
This is needed for the next commit.
Also, it doesn't really test anything novel. wait_for_debug_log is
inherently racy, so will randomly terminate at the exact point or later.
So the randomization is already sufficiently covered by the existing
test.
fa4339e4c1 Extract CTxIn::MAX_SEQUENCE_NONFINAL constant (MarcoFalke)
Pull request description:
Extracting the constant makes it possible to attach documentation to it.
Also, rework the docs for the other "sequence constants".
ACKs for top commit:
w0xlt:
reACK fa4339e for specifying the transaction version.
darosior:
re-ACK fa4339e4c1
luke-jr:
crACK fa4339e4c1
Tree-SHA512: 8d8f3dd5afb33eb5b72aa558e1e03de874c5ed02aa1084888e92ed86f3aaa5c725db45ded02e14cdfa67a92ac6774e97185b697f20a8ab63abbfcaa2fcd1fc6a
fa832103aa Avoid integer sanitizer warnings in chain.o (MarcoFalke)
Pull request description:
The two changes make the code more self-documenting and also allow to remove 5 file-wide suppressions for the module
ACKs for top commit:
PastaPastaPasta:
utACK fa832103aa
jonatack:
ACK fa832103aa
Tree-SHA512: d32a06099c56eed9f69130a3209f989872acc593f849528acd7746ee6caa96688cc32de37e8e59ad5d25dcb8912e341f1a43e50642dadeff6ca7624d0873ad10
fa6842978d fuzz: Speed up script fuzz target (MarcoFalke)
Pull request description:
Currently the script fuzz target takes the longest time (5000 seconds, aka 80 minutes, see https://cirrus-ci.com/task/5651378755338240?logs=ci#L4501).
Fix this by making it twice as fast.
Instead of running all possible combinations for all fuzz inputs, consume a bool and decide at runtime which path to take.
I moved the new calls to the end to not invalidate existing fuzz inputs.
ACKs for top commit:
prusnak:
ACK fa6842978d
Tree-SHA512: 5e408255f96f9e92e472f4e8a8a0f8d8814bad444ac0ff7d5db5ed84a59a861135ffe5e04d81f479b0695cb17e4d7af005734959dd4aa9328bdc5acc98f36665
99a6b699cd Fix race condition for SetBannedSetDirty() calls (Hennadii Stepanov)
83c7646715 Avoid calling BanMan::SweepBanned() twice in a row (Hennadii Stepanov)
33bda6ab87 Fix data race condition in BanMan::DumpBanlist() (Hennadii Stepanov)
5e20e9ec38 Prevent possible concurrent CBanDB::Write() calls (Hennadii Stepanov)
Pull request description:
This PR split from bitcoin/bitcoin#24097 with some additions. This makes the following switch from `RecursiveMutex` to `Mutex` a pure refactoring.
See details in commit messages.
ACKs for top commit:
w0xlt:
reACK 99a6b69
shaavan:
ACK 99a6b699cd
Tree-SHA512: da4e7268c7bd3424491f446145f18af4ccfc804023d0a7fe70e1462baab550a5e44f9159f8b9f9c7820d2c6cb6447b63883616199e4d9d439ab9ab1b67c7201b
20276ca5d1 Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() (Jon Atack)
Pull request description:
Following up on https://github.com/bitcoin/bitcoin/pull/22932#discussion_r794495535 by Marco Falke (good observation, thank you), we can replace a cs_main lock in `CBlockTreeDB::LoadBlockIndexGuts()` with a Clang thread safety annotation/assertion instead. The unlocked code is reverted to its original state before #22932.
ACKs for top commit:
hebasto:
ACK 20276ca5d1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2d91d1c962af0286d835d92a56396a27ea00e9d061b869eaff9421b448a083b0513828e1d4df7504396896057bf1e344e180a50271a5cfd1e1c7b6527155b2bb
58ccc88541 lint: add creat and ba into ignore-words for lint-spelling (brunoerg)
bad0e7f521 doc: Fix typos pointed out by lint-spelling (brunoerg)
Pull request description:
Occuring -> occurring (random.h)
Covert -> convert (chacha_poly_aead.cpp)
Fix `nWe` false positive in blockchain.cpp (https://github.com/bitcoin/bitcoin/pull/24203#issuecomment-1025116962)
Got it by linter, other ones are false positives.
ACKs for top commit:
prusnak:
ACK 58ccc88541
Tree-SHA512: b350d0e64968b96ead226da0be6aa4ca3f8e482ae401697867684ce8478e96b954124b3dea6dcd697aad4206f209f32f238d7cf0a0589075f24f5cf629c563f3
c73415bc10 build: Fix xargs warnings for Guix builds (Hennadii Stepanov)
Pull request description:
On master (e3ce019667) there are warnings in `./contrib/guix/guix-build` logs:
```
xargs: warning: options --max-args and --replace/-I/-i are mutually exclusive, ignoring previous --max-args value
```
This PR fixes such warnings.
ACKs for top commit:
prusnak:
utACK c73415b
Tree-SHA512: a7b55f59afbb19b78f795cea64acacf29903cfcd5fd7c37a771b073c1f2ff54555a26f3d00c1c73a8ef588396217ddf598e32b2ae961559042cc051b0aad162a
b3ccf26df4 build: force CRCCheck in Windows installer (fanquake)
Pull request description:
Otherwise a user can pass `/NCRC` on the command line and bypass the
CRC check, meaning they could use a corrupted installer. I can't think of
a reason why we'd want to allow the use of corrupted installers.
[NSIS docs](https://nsis.sourceforge.io/Docs/Chapter4.html#acrccheck):
> Specifies whether or not the installer will perform a CRC on itself before allowing an install. Note that if the user uses /NCRC on the command line when executing the installer, and you didn't specify 'force', the CRC will not occur, and the user will be allowed to install a (potentially) corrupted installer.
ACKs for top commit:
hebasto:
ACK b3ccf26df4, I have reviewed the code and it looks OK, I agree it can be merged.
prusnak:
utACK b3ccf26df4
Tree-SHA512: 6b3c151bfd896dbf1a4af26114aec1721e4d4e6dad89eef796cd754c988b1bb03fd9a3f9889be3c754ff5b1cd7bf5bbfc126876c76038205c17807daff029319
9fbd1bb7fa gui: use available space to display "Last Transaction" in peer details (Jon Atack)
6cd132d380 gui: add "Addresses Rate-Limited" (m_addr_rate_limited) to peer details (Jon Atack)
19623d3182 gui: add "Addresses Processed" (m_addr_processed) to peer details (Jon Atack)
a465a66ef2 gui: add "Address Relay" (m_addr_relay_enabled) to peer details (Jon Atack)
Pull request description:
This pull adds the following address fields in rpc getpeerinfo and cli -netinfo to the gui peers details:
- Address Relay (Yes/No)
- Addresses Processed (integer)
- Addresses Rate-Limited (integer)
and uses the additional horizontal space to display "Last Transaction" (instead of "Last Tx").
ACKs for top commit:
hebasto:
ACK 9fbd1bb7fa, tested on Ubuntu 21.10 (Qt 5.15.2).
w0xlt:
reACK 9fbd1bb
Tree-SHA512: 76d414b82f432b7baf2cadcf2f52412a3af8ad78a93755bb82c65df5353dda4d2e2522428a36c8bb95316bf84b17f2485636c33ce5ae11566469671b5384d845
Another thread can `SetBannedSetDirty(true)` while `CBanDB::Write()`
call being executed. The following `SetBannedSetDirty(false)`
effectively makes `m_is_dirty` flag value inconsistent with the actual
`m_banned` state. Such behavior can result in data loss, e.g., during
shutdown.
The m_is_dirty value being read in BannedSetIsDirty() can differ from
the value being set in SweepBanned(), i.e., be inconsistent with a
BanMan instance internal state.
faa75fa193 Avoid unsigned integer overflow in bitcoin-tx (MarcoFalke)
Pull request description:
While `npos` means "largest unsigned value" and adding `1` to it yields `0`, it may be clearer to just assign `0` to it and only increment otherwise.
This also allows to remove a file-wide suppression for `unsigned-integer-overflow`.
ACKs for top commit:
hebasto:
ACK faa75fa193, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK faa75fa193
Tree-SHA512: c24436641e5d801341c948b812c7f711d5dff70efdf04af00fd3221f4b81d93f25608dddaa36230ba81ca7ab0d18bdd957095d4561e22621e4d69017934f0a16
a380922891 Release notes for getdeploymentinfo rpc (Anthony Towns)
240cad09ba rpc: getdeploymentinfo: include signalling info (Anthony Towns)
376c0c6dae rpc: getdeploymentinfo: include block hash/height (Anthony Towns)
a7469bcd35 rpc: getdeploymentinfo: change stats to always refer to current period (Anthony Towns)
7f15c1841b rpc: getdeploymentinfo: allow specifying a blockhash other than tip (Anthony Towns)
fd826130a0 rpc: move softfork info from getblockchaininfo to getdeploymentinfo (Anthony Towns)
Pull request description:
The aim of this PR is to improve the ability to monitor soft fork status. It first moves the softfork section from getblockchaininfo into a new RPC named getdeploymentinfo, which is then also able to query the status of forks at an arbitrary block rather than only at the tip. In addition, bip9 status is changed to indicate the status of the given block, rather than just for the next block, and an additional field is included to indicate whether each block in the signalling period signaled.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK a380922891
Sjors:
tACK a380922891
fjahr:
tACK a380922891
Tree-SHA512: 7417d733b47629f229c5128586569909250481a3e94356c52fe67a03fd42cd81745246e384b98c4115fb61587714c879e4bc3e5f5c74407d9f8f6773472a33cb
fa5d2e678c Remove unused char serialize (MarcoFalke)
fa24493d63 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217 span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c
sipa:
re-utACK fa5d2e678c
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
6ea5682784 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0ba8 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88768 Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b00577 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5f6a Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457cb83 Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
572393448b Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ced28 Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341c10 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack)
Pull request description:
Issues:
- `CBlockIndex` member functions `GetBlockPos()`, `GetUndoPos()`, `IsAssumedValid()`, `RaiseValidity()`, and `IsValid()` and block storage functions `WriteUndoDataForBlock()` and `IsBlockPruned()` are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like #22895. Doing this also enables the next step:
- `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #17161. A solution is to guard it by cs_main, along with fellow data members `nFile`, `nDataPos` and `nUndoPos`.
This pull:
- adds thread safety lock annotations for the functions listed above
- guards `CBlockIndex::nStatus`, `nFile`, `nDataPos` and `nUndoPos` by cs_main
How to review and test:
- debug build with clang and verify there are no `-Wthread-safety-analysis` warnings
- review the code to verify each annotation or lock is necessary and sensible, or if any are missing
- look for whether taking a lock can be replaced by a lock annotation instead
- for more information about Clang thread safety analysis, see
- https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization
Mitigates/potentially closes#17161.
ACKs for top commit:
laanwj:
Code review ACK 6ea5682784
Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b
5e8975e269 fs: consistently use fsbridge for fopen() (fanquake)
486261dfcb fs: add missing <cassert> include (fanquake)
21f781ad79 fs: consistently use fsbridge for {i,o}fstream (fanquake)
Pull request description:
These changes are part of #20744, but are also ok to do now, and reduce the diff in that PR. See commit messages for details. Revived from #23857.
ACKs for top commit:
laanwj:
Code review ACK 5e8975e269
MarcoFalke:
ACK 5e8975e269 🏕
Tree-SHA512: ee2dc857ce2479b39b65615e689f934b962e580299b0e7a0c6361633402b0d61e6e4479f41f6480e2c46101264d93f330b8f7b57e56df95a7f77e046a4e44697
f13e642c83 build: Disable valgrind when building zeromq package in depends (Hennadii Stepanov)
b970f03bea build: Disable libbsd when building zeromq package in depends (Hennadii Stepanov)
77899991b1 build: Update netbsd_kevent_void.patch (Hennadii Stepanov)
Pull request description:
Since v4.3.3 (068385c951) `libzmq` uses `libbsd` by default.
This PR disables `libbsd` explicitly, as it's not a part of our depends. Zeromq will fallback to its internal `strlcpy` implementation.
Otherwise, on systems with installed `libbsd-dev` package the `zeromq` package build system erroneously detects `libbsd` package from the host system:
```diff
--- a/libzmq.pc
+++ b/libzmq.pc
@@ -8,5 +8,5 @@
Version: 4.3.4
Libs: -L${libdir} -lzmq
Libs.private: -lpthread
-Requires.private:
+Requires.private: libbsd
Cflags: -I${includedir}
```
This causes the `configure` fails to detect the `zeromq` package:
```
configure: WARNING: libzmq version 4.x or greater not found, disabling
```
---
Other minor improvements:
- fixed `netbsd_kevent_void.patch` offset
- disabled valgrind as it's used in unit tests which we do not run:
```diff
--- a/zmq-configure-output
+++ b/zmq-configure-output
@@ -119,11 +119,6 @@
checking whether the g++ -m64 linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
checking dynamic linker characteristics... (cached) GNU/Linux ld.so
checking how to hardcode library paths into programs... immediate
-checking for valgrind... valgrind
-checking for Valgrind tool memcheck... memcheck
-checking for Valgrind tool helgrind... helgrind
-checking for Valgrind tool drd... drd
-checking for Valgrind tool exp-sgcheck... exp-sgcheck
checking linker version script flag... --version-script
checking if version scripts can use complex wildcards... yes
checking for working posix_memalign... yes
```
ACKs for top commit:
fanquake:
ACK f13e642c83
Tree-SHA512: d4c86d4a841eb6e7c32157e84972243072f905496c2a4c14ec6f6ab4216df6695cbf29baa2233ce27eaede35d1e250ad2f9975b16f570d01509f0c5da4597cad
6498ba151b transaction decoding infer output descriptors (Gregory Sanders)
Pull request description:
Following discussion in #16725 this is complementary data to expose. All outputs are inferred.
ACKs for top commit:
achow101:
ACK 6498ba151b
meshcollider:
utACK 6498ba151b
Tree-SHA512: 36664117ddbe46d5fdde7ed6541ef2c9d8dfb7a3636b97f363bf1c325096fe00d9d2acea2d1917ea19fdb82f1ea296c12e440c5c703d6a9bfc1a02fba028bcd8
This is needed to prevent compilation failures once boost is removed,
however is still correct to include now, and reduces the diff in #20744.
<string> is extracted from the defines because it is used for Windows
and non-Windows code, i.e get_filesystem_error_message().
fac8caaa62 doc: Fix rpc docs (MarcoFalke)
Pull request description:
Broken in commit 39d9bbe4ac.
The fix removes the "type" `OBJ_EMPTY` added in commit 8d1a3e6498, which isn't really a separate type and instead runs a check on `OBJ` whether it is empty or not.
ACKs for top commit:
Sjors:
tACK fac8caaa62
Tree-SHA512: dd978fe526a45095800249204afd26a239078e83b15124a5756ac078c473a677a3084b8f54e34d6dd5580abef7275c875a14bc9eb20d8feab066dfb0f0932967
449dffc610 test, bugfix: fix intermittent failure in getrawtransaction_tests (Jon Atack)
Pull request description:
Easiest to review with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`.
Resolves#23991
Top commit has no ACKs.
Tree-SHA512: a50c597857e45c52dae1b6fc5988ea077c09f2b32ffe3605f0c64bd780f5dd2635755cb26c4981233bf925c5ea9f7310163349a549239dc85657558f253ffb7b
75656adfd2 test: add functional test for `-maxtipage` parameter (Sebastian Falbesoner)
Pull request description:
This PR adds a missing test for the `-maxtipage` parameter which controls what is the allowed maximum tip age for leaving IBD:
792d0d8d51/src/init.cpp (L540)
Relevant code path in the `CChainState::IsInitialBlockDownload` method:
792d0d8d51/src/validation.cpp (L1479-L1480)
The test is pretty simple and should be self-explanatory.
ACKs for top commit:
MarcoFalke:
review ACK 75656adfd2
Tree-SHA512: 0a10dca13cb18c29e64fc8412f4c8f2bcaff1bab8645bd85266c242ba88ce036a150c03cbbe9810c3bb44649810af0aa9cb3584dbae886a7bdb16b72150d08de
fa3bdbd37b test: remove unused sanitizer suppressions (MarcoFalke)
Pull request description:
Looks like those are not needed (anymore)
ACKs for top commit:
fanquake:
ACK fa3bdbd37b
Tree-SHA512: 4bedb6363aba8ea7763291ee0cd074e6bfd77e691bb32999c3959393864dc396bacba1eced2b10d9d600b66e8b83b91f7bc6692331dbd113bbaa87e72d11e2e8
fac8165443 Remove unused checkFinalTx (MarcoFalke)
fa272eab44 wallet: Avoid dropping confirmed coins (MarcoFalke)
888841ea8d interfaces: Remove unused is_final (MarcoFalke)
dddd05e7a3 qt: Treat unconfirmed txs as unconfirmed (MarcoFalke)
Pull request description:
The wallet has several issues:
## Unconfirmed txs in the GUI
The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn't locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics.
## Confirmed txs in the wallet
The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the `-1` default argument of `CheckFinalTx`.
The issues are fixed in separate commits and there is even a test.
ACKs for top commit:
achow101:
ACK fac8165443
prayank23:
reACK fac8165443
glozow:
code review ACK fac8165443, I understand now how this fixes both issues.
Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
