c821ab8be8 Use `GetAllOutputTypes` in `getblock` RPC function (Kiminuo)
d970a85d33 Move `GetAllOutputTypes` function from `rpc/rawtransaction.cpp` to `rpc/util.{h|cpp}` (Kiminuo)
Pull request description:
This PR attempts to replicate 0ccf9b2e55/src/rpc/rawtransaction.cpp (L547) to one other place (at the moment) so that users have better idea what RPC methods can actually return.
I created this PR as a follow-up to the idea mentioned here https://github.com/bitcoin/bitcoin/pull/23320#discussion_r732458112 (resolved).
ACKs for top commit:
kristapsk:
re-ACK c821ab8be8
Tree-SHA512: 5ff66a41ad7c43ec769f4a99933d2d070feea7c617286d94b6f9bfa1a2547a42211915778210a89074ad4b14d99f34852cc6871efed5e6f1e2ffedd40d669386
fa30e62cc6 doc: Rework generate* doc (MarcoFalke)
Pull request description:
Hide the test-only calls and clarify the short description
ACKs for top commit:
0xB10C:
reACK fa30e62cc6. changes since fa3bb584dcc742a767b2141cd7324877e3cf5302 are: dropping the `immediately` + formatting the touched line and a rebase
Tree-SHA512: 07439f39660bbf144c2cc406b6010b64dcdd27150d78654fe04a36a982a519f837a0cf0f030c9f30af69c451ccf7a3b7287a275637aa81904c202029b9efc661
0683f377e1 Add tr() descriptor unit tests (Pieter Wuille)
4b2e31a7ae Bugfix: make ToPrivateString work with x-only keys (Pieter Wuille)
18ad54c3b2 Bugfix: set x-only flag when inferring pk() inside tr() (Pieter Wuille)
Pull request description:
This fixes two bugs in the current logic for `tr()` descriptors:
* ToPrivateString does not always work, because the provided private key may mismatch the parity of the x-only public key.
* The descriptors inferred for `pk()` inside `tr()` have the wrong x-only flag, leading to such descriptors generating the wrong scriptPubKey (roundtripping through ToString does fix it however, so this seems unobservable in the current code).
These were discovered while adding unit tests to descriptor_tests that cover various aspects of `tr()` descriptors, which are now also added here.
ACKs for top commit:
achow101:
ACK 0683f377e1
instagibbs:
ACK 0683f377e1
jonatack:
Code review ACK 0683f377e1
Tree-SHA512: fc0e11b45da53054a108effff2029d67b64e508b160a6e22e00c98b506c39ec12ccc95afd21ea68a6c691eb62930afc7af18908f2fa3a954d102afdc67bc355a
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
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
