6981de4435 doc: fix wording of alertnotify (willcl-ark)
Pull request description:
The documentation of the `alertnotify` startup option no longer matches the implementation.
Currently the alert is only triggered by `DoWarning` (as part of `CChainstate::UpdateTip` when blocks containing unknown versionbits are detected on the network, indicating that there may be an upcoming softfork which you don't know about), but not when we see a "really long fork":
2825c41a61/src/validation.cpp (L2418-L2433)
I think it would be desirable in a follow-up PR to implement the logic to alert on a (really) long fork, but not to alert for "partition detection" (abnormally slow/fast blocks). `PartitionChecker` code was removed in ab8be98fdb
ACKs for top commit:
josibake:
ACK 6981de4435
achow101:
ACK 6981de4435
Tree-SHA512: ea124f53ca1db803ba93d649f4bc983484c47fb5fe7fa61a8eb32fcbc7425f67d8578e66a6ba70202e13868fe8add0103306dede3b1edd1d3261ffb9c1042b87
62cc138ecb Rename wallet-tool to bitcoin-wallet in code comment (Kristaps Kaupe)
0db3ad3ba4 Mention -signet in bitcoin-wallet help output (Kristaps Kaupe)
Pull request description:
* Mention `-signet` in sentence where there is already `-testnet/-signet` in help output.
* Rename `wallet-tool` to `bitcoin-wallet` in single remaining place in code comments (was already done in #17648 at other places).
ACKs for top commit:
RandyMcMillan:
tACK 62cc138ecb
Tree-SHA512: c5df7811b8200f61943908dcf3b2b788fe991bf00bef28f069ab8784924556ffd5d86fc0ba2ad0b3c3f9be2ba73a34bc67059d7c057bba646c1801ffa3cb2070
fa1b89a6bd scripted-diff: Rename nReadPos to m_read_pos in streams.h (MarcoFalke)
fa56c79df9 Make CDataStream work properly on 64-bit systems (MarcoFalke)
fab02f7991 streams: Fix read-past-the-end and integer overflows (MarcoFalke)
Pull request description:
This is a follow-up to commit e26b62093a with the following fixes:
* Fix unsigned integer overflow in `ignore()`, when `nReadPos` wraps.
* Fix unsigned integer overflow in `read()`, when `nReadPos` wraps.
* Fix read-past-the-end in `read()`, when `nReadPos` wraps.
This shouldn't be remote-exploitable, because it requires a stream of more than 1GB of size. However, it might be exploitable if the attacker controls the datadir (I haven't checked).
A unit test for the overflow in `ignore()` looks like following. It is left as an excercise to the reader to replace `foo.ignore(7)` with the appropriate call to `read()` to reproduce the overflow and read-error in `read()`.
```diff
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 922fd8e513..ec6ea93919 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -534,6 +534,20 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
} catch (const std::ios_base::failure&) {
}
+ CDataStream foo{0, 0};
+ auto size{std::numeric_limits<uint32_t>::max()};
+ foo.resize(size);
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ foo.ignore(std::numeric_limits<int32_t>::max());
+ size -= std::numeric_limits<int32_t>::max();
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ foo.ignore(std::numeric_limits<int32_t>::max());
+ size -= std::numeric_limits<int32_t>::max();
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ BOOST_CHECK_EQUAL(foo.size(), 1);
+ foo.ignore(7); // Should overflow, as the size is only 1
+ BOOST_CHECK_EQUAL(foo.size(), uint32_t(1 - 7));
+
// Very large scriptPubKey (3*10^9 bytes) past the end of the stream
CDataStream tmp(SER_DISK, CLIENT_VERSION);
uint64_t x = 3000000000ULL;
```
ACKs for top commit:
klementtan:
Code Review ACK fa1b89a6bd:
Tree-SHA512: 67f0a1baafe88eaf1dc844ac55b638d5cf168a18c945e3bf7a2cb03c9a5976674a8e3af2487d8a2c3eae21e5c0e7a519c8b16ee7f104934442e2769d100660e9
a1515cdd96 ci: use Ubuntu Jammy for Windows CI (fanquake)
Pull request description:
This means we'll compile using [GCC 10.3.x](https://packages.ubuntu.com/jammy/g++-mingw-w64) and [mingw-w64 8.0.0](https://packages.ubuntu.com/jammy/mingw-w64) which better matches our Guix release environment.
ACKs for top commit:
MarcoFalke:
cr ACK a1515cdd96
hebasto:
ACK a1515cdd96, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: a57cce1874324c9dd00e5d8989996d214facbdd561440471c15e6cc1808bca1c6fd758abe7a1b87378b2e7f9c25e7c9d8242df911cd1ef6cfbe49718adc3be5d
fa8dad0e07 rpc: Fix implicit-integer-sign-change in verifychain (MarcoFalke)
Pull request description:
It doesn't really make sense to treat `DEFAULT_CHECKLEVEL` as unsigned as long as `VerifyDB` accepts a signed integer.
Making it signed also avoids a cast round trip from signed->unsigned->signed in the RPC.
ACKs for top commit:
luke-jr:
utACK fa8dad0e07
theStack:
Code-review ACK fa8dad0e07
Tree-SHA512: 75499dbe4ace2962792e5fbec7defb10c25fdbbfde951d5e542a91daa880cc50395da0287173e2c84a28e18267c74af7b44b9f38ce364bcb0216c402f65b7641
820c03aff5 index: check muhash is in sync on coinstatsindex launch (Fabian Jahr)
38ed58b850 index: remove txindex references from base index (Fabian Jahr)
Pull request description:
This change lets the `coinstatsindex` fail loudly in case the internal `muhash` state differs from the last finalized output saved on disk, which would indicate that the `muhash` state somehow got out of sync. This should generally not happen since both are written to disk in a batch but #24076 seems to indicate that the might still be an issue.
Since #24076 so far can not be reproduced reliably, the issue should not be closed yet. Further investigation and testing needs to be done.
ACKs for top commit:
Sjors:
re-ACK 820c03aff5
mzumsande:
re-ACK 820c03aff5
ryanofsky:
Code review ACK 820c03aff5. Good to catch the error earlier
Tree-SHA512: 3c985d7152698d25bad95d4ad512ff87dff13fabef790589c5a6cf93ca4251ad599e12feb7251a084503e2a213b022eaacfbaaa601464114ad372b029f64f204
799968e8b3 tracing: misc follow-ups to 22902 (0xb10c)
36a6584703 tracing: correctly scope utxocache:flush tracepoint (Arnab Sen)
Pull request description:
This PR is a follow-up to the [#22902](https://github.com/bitcoin/bitcoin/pull/22902).
Previously, the tracepoint `utxocache:flush` was called, even when it was not flushing. So, the tracepoint is now scoped to write only when coins cache to disk.
ACKs for top commit:
0xB10C:
ACK 799968e8b3
Tree-SHA512: ebb096cbf991c551c81e4339821f10d9768c14cf3d8cb14d0ad851acff5980962228a1c746914c6aba3bdb27e8be53b33349c41efe8bab5542f639916e437b5f
81738d2881 test: Remove suppression no longer needed with headers-only Boost.Test (Hennadii Stepanov)
Pull request description:
It appears, that moving to [headers-only](https://github.com/bitcoin/bitcoin/pull/24301) Boost.Test makes the removed suppression unneeded even without [bumping](https://github.com/bitcoin/bitcoin/pull/24383) boost version.
ACKs for top commit:
MarcoFalke:
cr ACK 81738d2881
Tree-SHA512: e60443f79a2e38cc78fceeff5c2956d622e8a10730129f9c27c14aef59bc6fa0894b8011e6191530443bf3165f78da978bc08ad04248ddb65e2da373264afa6a
07dcf1a76e build: remove boost dep from libmultiprocess (fanquake)
Pull request description:
Looks like this hasn't been needed since https://github.com/chaincodelabs/libmultiprocess/pull/25 and was just missed in #19160.
ACKs for top commit:
ryanofsky:
Code review ACK 07dcf1a76e. Should probably wait for GUIX build results, but I think this should be fine
hebasto:
ACK 07dcf1a76e
Tree-SHA512: 7988efd4aaf6ad512d60cfd33f350df56090daf88aac3aed2a1d400e80bc723dc27d27f5fa5d75359f9fae60d04b87d4b120d4e79e3079df8631956ab6c3b83c
d4b3483cec Primitives: Correct CTransaction deserialization docstring (TheCharlatan)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/8589 CTxWitness was removed and instead replaced with CScriptWitness inside each CTxIn.
ACKs for top commit:
w0xlt:
ACK d4b3483
Tree-SHA512: 02bb73e8a7d1fc449e4776a162009261baecc573837fade74ad7d76b3cd63200424e02fd0abd000c63706072f2ab3c95d3053139495b81347463f43e56192ca9
- mention 'Lost X events' workaround
- clarify flush tracepoint docs
- fix typo in tracepoint context
- clarify flush for prune
The documentation and examples for the `fFlushForPrune` argument
of the utxocache flush tracepoint weren't clear without looking
at the code.
See these comments: https://github.com/bitcoin/bitcoin/pull/22902#issuecomment-987094612
- doc: note that there can be temporary UTXO caches
Bitcoin Core uses temporary clones of it's _main_ UTXO cache in some
places. The utxocache:add and :spent tracepoints are triggered when
temporary caches are changed too. This is documented.
a4da16fbd4 Improve -netinfo help based on feedback from users and devs (Jon Atack)
Pull request description:
Clarify which networks are displayed by the peer counts table (*reachable* networks; follow-up to #23324) in response to questions received over the past months, and a few other improvements.
ACKs for top commit:
laanwj:
Code review ACK a4da16fbd4
w0xlt:
ACK a4da16f
kristapsk:
utACK a4da16fbd4
Tree-SHA512: e6522c08421aa7f10d50723156d0a8fc5ec82cad2f0bd931bbec603077fcd4921c6505ef743d57386fba81c95dcfc77df75abf3378319886368e4ae33f9a6d73
8e9699cb10 Update doc to match new default wallet type (Bitcoin Hodler)
Pull request description:
#23002 changed the default wallet type to descriptors, so this doc was out of date.
ACKs for top commit:
achow101:
ACK 8e9699cb10
Tree-SHA512: 2f69b23c153163bf2a091dbf728b713d28f795cc81e031bf201160882d2456494e94955ff6385634615fdcfece11542749ad1c982e2994e64ed69011380a2353
Switching the endianness of a hex string `str` can simply be
achieved by `bytes.fromhex(str)[::-1].hex()`, i.e. we can use
that and remove those helper methods.
fae3f17823 fuzz: Split script formatting from script fuzz target (MarcoFalke)
Pull request description:
This is a follow-up to commit 9237bdaac1.
The target was improved a bit, but is still taking enormously long. See for example 4096 seconds in https://cirrus-ci.com/task/5153886888525824?logs=ci#L4451.
Most of the time is spent formatting the script. See the flamegraph: 
Thus, I suggest to split up the formatting into a new target. This will:
* Allow more fuzz cycles in the `script` target when exploring the search space with the fuzz engine
* Hopefully allow to reduce the fuzz inputs in `qa-assets` without losing coverage
ACKs for top commit:
fanquake:
ACK fae3f17823
Tree-SHA512: f86154b23019b7721e5dd10f54d11f4f7603d280471a396cb5256f4c460f48333318a60efe8b77fa8749a4abc67ad2631211b766fde5da70ded9fab8f904747b
b223c3c21e test: Add functional test for symlinked blocks directory (laanwj)
ddb75c2e87 test: Add fs_tests/create_directories unit test (Hennadii Stepanov)
1f46b6e46e util: Work around libstdc++ create_directories issue (laanwj)
Pull request description:
Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than #24266, which worked around one instance of the problem.
The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
ACKs for top commit:
jonatack:
re-ACK b223c3c21e per `git range-diff df08250 67019cd b223c3c`
hebasto:
re-ACK b223c3c21e
w0xlt:
re-ACK b223c3c
vasild:
ACK b223c3c21e
Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc
eb8b22d517 block_connected: re-use previous GetTimeMicros (William Casarin)
80e1c55687 block_connected: don't serialize block hash twice (William Casarin)
Pull request description:
In the validation:block_connected tracepoint, we call block->GetHash(), which
ends up calling CBlockHeader::GetHash(), executing around 8000 serialization
instructions. We don't need to do this extra work, because block->GetHash() is
already called further up in the function. Let's save that value as a local
variable and re-use it in our tracepoint so there is no unnecessary tracepoint
overhead.
Shave off an extra 100 or so instructions from the validation:block_connected
tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down
to 54 instructions. Still high, but much better than the previous ~154 and
8000 instructions which it was originally.
Signed-off-by: William Casarin <jb55@jb55.com>
ACKs for top commit:
0xB10C:
ACK eb8b22d517
laanwj:
Code review ACK eb8b22d517
theStack:
re-ACK eb8b22d517
Tree-SHA512: 92ae585e487554e0f73042a8abaa239f630502c1d198e010bd7c1de252d882bccb627bbf0e4faec09c1253e782b145bcf153f9fee78cdb8456188044a96f8267
Work around libstdc++ issue [PR101510] with create_directories where the
leaf already exists as a symlink. Fixes#24257, introduced by the switch
to `std::filesystem`. It is meant to be more thorough than #24266, which
only worked around one instance of the problem.
The issue was fixed upstream in
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc,
but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
dc01cbc538 test: Add fs_tests/rename unit test (Hennadii Stepanov)
d4999d40b9 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov)
Pull request description:
Unfortunately, bitcoin/bitcoin#24308 introduced a [regression](https://github.com/bitcoin/bitcoin/pull/24308#issuecomment-1037259386) for mingw builds.
The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior
> If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_...
fails with the "File exists" error.
This PR reverts back the `MoveFileExW` call, and adds the [suggested](https://github.com/bitcoin/bitcoin/pull/24308#pullrequestreview-878832906) unit test.
ACKs for top commit:
vasild:
ACK dc01cbc538
Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
f485a07454 Add missing thread safety lock assertions in validation.h (Jon Atack)
37af8a20cf Add missing thread safety lock assertions in validation.cpp (Jon Atack)
Pull request description:
A number of functions in validation.{h,cpp} have a thread safety lock annotation in the declaration but are missing the corresponding run-time lock assertion in the definition.
ACKs for top commit:
hebasto:
re-ACK f485a07454, only suggested change since my [previous](https://github.com/bitcoin/bitcoin/pull/24177#pullrequestreview-877810465) review.
vasild:
ACK f485a07454
Tree-SHA512: c86c0c0e8fe6ec7ae9ed9890f1dd7d042aa482ecf99feb6679a670aa004f6e9a99f7bc047205a34968fab7f1f841898c59b48c3ed6245c166e3b5abbf0867445
