The current code does not have a bug, but is implicitly casting -1 to
65535 and the sanitizer has no way to know whether we intend that or
not.
```
FUZZ=bitset src/test/fuzz/fuzz /tmp/fuz
error: implicit conversion from type 'int' of value -1 (32-bit, signed)
to type 'value_type' (aka 'unsigned short') changed the value to 65535
(16-bit, unsigned)
Base64: Qv7bX/8=
```
1807df3d9fb0135057a33e01173080ea14b0403d test: addrman: tried 3 times and never a success so `isTerrible=true` (brunoerg)
Pull request description:
This PR adds test coverage for the following verification:
```cpp
if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
return true;
}
```
If we've tried an address for 3 or more times and were unsuccessful, this address should be pointed out as "terrible".
-------
You can test this by applying:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 054a9bee32..93a9521b59 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -81,7 +81,7 @@ bool AddrInfo::IsTerrible(NodeSeconds now) const
}
if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
- return true;
+ return false;
}
```
ACKs for top commit:
jonatack:
re-ACK 1807df3d9fb0135057a33e01173080ea14b0403d
naumenkogs:
ACK 1807df3d9fb0135057a33e01173080ea14b0403d
achow101:
ACK 1807df3d9fb0135057a33e01173080ea14b0403d
Tree-SHA512: e3cc43c98bddfe90f585d5b4bd00543be443b77ecaf038615261aa8cc4d14fc2f1006d0b00c04188040eaace455c5c6dbb3bb200a2c0f29c3b4ef5128bf0973a
50cce20013c97e1257cb9f4c9319701a09b0a196 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0914775756dcac2d9fa7fe4e4d4e70ba0c test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c1eef95c0b626573a9a2e217f4a541a880 test: Validate error messages on fail (Lőrinc)
d5f8d607ab1f8fd9fc17aba4105867b450117be2 test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7490a5005d227da75dc8f2d1ab73c6e9d2 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81c3818b4138602c127d6a16018aae75687 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b733699cfc79253ffae1527106baa428dd62f39 coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282022e6ce4eb3ce526800a6ada3b4a2996d coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498eabc910efa3ed7a6d32e687107248bb5be coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)
Pull request description:
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.
`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).
Once it was marked as `dirty`, we couldn’t set the state back to clean with `AddFlags(0)`—tests explicitly checked against that.
This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of `CCoinsCacheEntry`, as many tests were simply validating invalid combinations in this way.
The last few commits contain significant test refactorings to make `coins_tests` easier to change in follow-ups.
ACKs for top commit:
andrewtoth:
Code Review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196
laanwj:
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196
ryanofsky:
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196. Looks good! Thanks for the followups.
Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c
95a0104f2e9869799db84add108ae8c57b56d360 test: Add tests for directories in place of config files (Hodlinator)
e85abe92c7cc5380489c028479f0d42f91827efd args: Catch directories in place of config files (Hodlinator)
e4b6b1822ce004365be11c54c8f5f02f95303fb0 test: Add tests for -noconf (Hodlinator)
483f0dacc413f4b1ba1b74c2429c4367b87e7f11 args: Properly support -noconf (Hodlinator)
312ec64cc0619f58c6e8abc5855fd2fa0e920c3f test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator)
7402658bc2b9d835b240edb9c1dac308859687c3 test: -norpccookiefile (Hodlinator)
39cbd4f37c3d3a32cd993cbc78052d53f700989b args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator)
e82ad88452bce4132e4583727e610d52dcf9ad9e logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator)
6e28c76907ca0012b1a4556d9a982dfffb5abaf6 test: Harden testing of cookie file existence (Hodlinator)
75bacabb55f3d54ad9b2c660cafc82c167e4f644 test: combine_logs.py - Output debug.log paths on error (Hodlinator)
bffd92f00f5bfbb5a622d0bd20bfeed9f8b10928 args: Support -nopid (Hodlinator)
12f8d848fd91b11d5cffe21dfc3ba124102ee236 args: Disallow -nodatadir (Hodlinator)
6ff9662760099c405cf13153dd1de22900045f5e scripted-diff: Avoid printing version information for -noversion (Hodlinator)
e8a2054edc814b2c4661b96a3dce91da9be68fa4 doc args: Document narrow scope of -color (Hodlinator)
Pull request description:
- Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users.
- No longer print version information when getting passed `-noversion`.
- Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
- Support `-norpccookiefile`
- Support `-nopid`
- Properly support `-noconf` (instead of working by accident). Also detect when directories are specified instead of files.
Prompted by investigation in https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013.
ACKs for top commit:
l0rinc:
utACK 95a0104f2e9869799db84add108ae8c57b56d360
achow101:
ACK 95a0104f2e9869799db84add108ae8c57b56d360
ryanofsky:
Code review ACK 95a0104f2e9869799db84add108ae8c57b56d360. Looks good! Thanks for all your work on this breaking the changes down and making them simple.
Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785
fa3e074304780549b1e7972217930e34fa55f59a refactor: Tidy fixups (MarcoFalke)
fa72646f2b197810a324cb0544d9a1fac37d3f9c move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0aac3b5ec26d3c7fc98240f879f9906 refactor: Pick translated string after format (MarcoFalke)
Pull request description:
The changes are required for https://github.com/bitcoin/bitcoin/pull/31061, however they also make sense on their own. For example, they are fixing up an `inline namespace`, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to `src/util/strencodings.h`). Also, a unit test comment is fixed.
ACKs for top commit:
ryanofsky:
Code review ACK fa3e074304780549b1e7972217930e34fa55f59a. Nice changes! These should allow related PRs to be simpler.
l0rinc:
ACK fa3e074304780549b1e7972217930e34fa55f59a
hodlinator:
cr-ACK fa3e074304780549b1e7972217930e34fa55f59a
Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
faf70cc9941ce2b0ce4fd48ecfdbe28194adb8ba Remove wallet::ParseISO8601DateTime, use ParseISO8601DateTime instead (MarcoFalke)
2222aecd5f8059785e655da7b7e3fcc59204245c util: Implement ParseISO8601DateTime based on C++20 (MarcoFalke)
Pull request description:
`boost::posix_time` in `ParseISO8601DateTime` has many issues:
* It parses random strings that are clearly invalid and returns a time value for them, see [1] below.
* None of the separators `-`, or `:`, or `T`, or `Z` are validated.
* It may crash when running under a hardened C++ library, see https://github.com/bitcoin/bitcoin/issues/28917.
* It has been unmaintained for years, so reporting or fixing any issues will most likely be useless.
* It pulls in a third-party dependency, when the functionality is already included in vanilla C++20.
Fix all issues by replacing it with a simple helper function written in C++20.
Fixes https://github.com/bitcoin/bitcoin/issues/28917.
[1] The following patch passes on current master:
```diff
diff --git a/src/wallet/test/rpc_util_tests.cpp b/src/wallet/test/rpc_util_tests.cpp
index 32f6f5ab46..c1c94c7116 100644
--- a/src/wallet/test/rpc_util_tests.cpp
+++ b/src/wallet/test/rpc_util_tests.cpp
@@ -12,6 +12,14 @@ BOOST_AUTO_TEST_SUITE(wallet_util_tests)
BOOST_AUTO_TEST_CASE(util_ParseISO8601DateTime)
{
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("964296"), 242118028800);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("244622"), 15023836800);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("+INfINITy"), 9223372036854);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("7000802 01"), 158734166400);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("7469-2 +INfINITy"), 9223372036854);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("maXimum-datE-time"), 253402300799);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("577737 114maXimum-datE-time"), 253402300799);
+
BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z"), 0);
BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0);
BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:01Z"), 946684801);
```
ACKs for top commit:
hebasto:
ACK faf70cc9941ce2b0ce4fd48ecfdbe28194adb8ba, I have reviewed the code and it looks OK.
dergoegge:
utACK faf70cc9941ce2b0ce4fd48ecfdbe28194adb8ba
Tree-SHA512: 9dd745a356d04acf6200e13a6af52c51a9e2a0eeccea110093ce5da147b3c669c0eda918e46db0164c081a78c8feae3fe557a4759bea18449a8ff2d090095931
8f85d36d68ab33ba237407a2ed16667eb149d61f refactor: Clamp worker threads in ChainstateManager constructor (TheCharlatan)
Pull request description:
This ensures the options are applied consistently from contexts where they might not pass through the args manager, such as in some tests, or when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6, used to make applying the mempool options consistent.
---
This is part of the libbitcoinkernel project https://github.com/bitcoin/bitcoin/issues/27587
ACKs for top commit:
maflcko:
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳
achow101:
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
furszy:
Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
stickies-v:
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
Tree-SHA512: 32d7cc177d6726ee9df62ac9eb43e49ba676f35bfcff47834bd97a1e33f2a9ea7be65d0a8a37be149de04e58c9c500ecef730e498f4e3909042324d3136160e9
32fc59796f74a2941772b5ec2755b1319132cd9c rpc: Allow single transaction through submitpackage (glozow)
Pull request description:
There's no particular reason to restrict single transaction submissions with submitpackage. This change relaxes the RPC checks as enables the `AcceptPackage` flow to accept packages of a single transaction.
Resolves#31085
ACKs for top commit:
naumenkogs:
ACK 32fc59796f
achow101:
ACK 32fc59796f74a2941772b5ec2755b1319132cd9c
glozow:
ACK 32fc59796f74a2941772b5ec2755b1319132cd9c
Tree-SHA512: ffed353bfdca610ffcfd53b40b76da05ffc26df6bac4b0421492e067bede930380e03399d2e2d1d17f0e88fb91cd8eb376e3aabebbabcc724590bf068d09807c
73db95c65c1d372822166045ca8b9f173d5fd883 kernel: Make bitcoin-chainstate's block validation mirror submitblock's (TheCharlatan)
bb53ce9bdae2f02d7bd95cf5d8ca4ccf5136466a tests: Add functional test for submitting a previously pruned block (Greg Sanders)
1f7fc738255205a64374686aca9a4c53089360f1 rpc: Remove submitblock duplicate pre-check (TheCharlatan)
e62a8abd7df21795dcd173773f689b6d4c8feab6 rpc: Remove submitblock invalid-duplicate precheck (TheCharlatan)
36dbebafb9b54764005e6fffa7ad28d4cadfe5e4 rpc: Remove submitblock coinbase pre-check (TheCharlatan)
Pull request description:
With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden.
The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.
The coinbase check is repeated again early during ProcessNewBlock. Pre-checking it may also shadow more fundamental problems with a block. In most cases the block header is checked first, before validating the transactions. Checking the coinbase first therefore masks potential issues with the header. Fix this by removing the pre-check.
Similary the duplicate checks are repeated early in the contextual checks of ProcessNewBlock. If duplicate blocks are detected much of their validation is skipped. Depending on the constitution of the block, validating the merkle root of the block is part of the more intensive workload when validating a block. This could be an argument for moving the pre-checks into block processing. In net_processing this would have a smaller effect however, since the block mutation check, which also validates the merkle root, is done before.
Testing spamming a node with valid, but duplicate unrequested blocks seems to exhaust a CPU thread, but does not seem to significantly impact keeping up with the tip. The benefits of adding these checks to net_processing are questionable, especially since there are other ways to trigger the more CPU-intensive checks without submitting a duplicate block. Since these DOS concerns apply even less to the RPC interface, which does not have banning mechanics built in, remove them too.
Finally, also remove the pre-checks from `bitcoin-chainstate.cpp`.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
Sjors:
re-utACK 73db95c65c1d372822166045ca8b9f173d5fd883
achow101:
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
instagibbs:
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
mzumsande:
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
Tree-SHA512: 2d02e851cf402ecf6a1968c058df3576aac407e200cbf922a1a6391b7f97b4f42c6d9f6b0a78b9d1af0a6d40bdd529a7b11a1e6d88885bd7b8b090f6d1411861
-noconf would previously lead to an ifstream "successfully" being opened to the ".bitcoin"-directory (not a file). (Guards against the general case of directories as configs are added in grandchild commit to this one).
Other users of AbsPathForConfigVal() in combination with negated args have been updated earlier in this PR ("args: Support -nopid" and "args: Support -norpccookiefile...").
This makes the debug output mostly the same for -par=1 and parallel validation runs. Of course,
parallel validation is non-deterministic in what error it may encounter first if there are
multiple issues. Also, the way certain script-related and non-script-related checks are
performed differs between the two modes still, which may result in discrepancies.
The check type function now needs to return a std::optional<R> for some type R,
and the check queue overall will return std::nullopt if all individual checks
return that, or one of the non-nullopt values if there is at least one.
For most tests, we use R=int, but for the actual validation code, we make it return
the ScriptError.
The `ccoins_add` and `ccoins_write` tests check the actual exception error messages now instead of just that they fail for the given parameters.
This enables us testing different exceptions in a more fine-grained way in later changes.
We don't need so much access to the internals of CCoinsCacheEntry, since many tests are just exercising invalid combinations this way.
This implies that `AddFlags` has private access now.
CCoinsCacheEntry provided general access to its internal flags state, even though in reality it could only be clean, fresh, dirty or fresh|dirty.
After it got dirtied we couldn't set the state back to clean by AddFlags(0) - tests were explicitly checking against that.
This commit cleans up the public interface to make this distinction cleaner and invalid behavior impossible instead of just checked by tests.
This includes the removal of redundant `inline` qualifiers (we're inside a struct).
Also renamed `self` to `pair` to simplify the upcoming commits.
Also modernized `EmplaceCoinInternalDANGER` since it was already modified.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
ab5c63edccea7957e2c7b3b9d8b04655a7f82f22 cmake: Build `secp256k1` only when required (Hennadii Stepanov)
76a3a540a4c3ff42f84c4701cb2ca6e910a24f50 cmake: Ensure script correctness when no targets are specified (Hennadii Stepanov)
Pull request description:
When no build targets are specified, it is reasonable to expect the configuration step to succeed and produce a build system that does not build any targets.
This PR updates the code to ensure this behaviour:
```
$ cmake -B build -G "Ninja" -DBUILD_DAEMON=OFF -DBUILD_CLI=OFF -DBUILD_TX=OFF -DBUILD_UTIL=OFF -DENABLE_WALLET=OFF -DBUILD_TESTS=OFF
$ cmake --build build
ninja: no work to do.
```
ACKs for top commit:
TheCharlatan:
ACK ab5c63edccea7957e2c7b3b9d8b04655a7f82f22
tdb3:
light test ACK ab5c63edccea7957e2c7b3b9d8b04655a7f82f22
Tree-SHA512: 1b13f406c58b02768d9ba831413aeae1d7e03659e7101de8e598f906ba220f479ac06707965c96a14468ce4ba49011a1ab9adee9cee34ab1e8622f690b94dad8
b73d3319377a4c9d7e2dd279c3d106002585bc36 dbwrapper: Bump max file size to 32 MiB (Maciej S. Szmigiero)
Pull request description:
The default max file size for LevelDB is 2 MiB, which results in the LevelDB compaction code generating ~4 disk cache flushes per second when syncing with the Bitcoin network.
These disk cache flushes are triggered by `fdatasync()` syscall issued by the LevelDB compaction code when reaching the max file size.
If the database is on a HDD this flush rate brings the whole system to a crawl.
It also results in very slow throughput since 2 MiB * 4 flushes per second is about 8 MiB / second max throughput, while even an old HDD can pull 100 - 200 MiB / second streaming throughput.
Increase the max file size for LevelDB to 128 MiB instead so the flush rate drops to about 1 flush / 2 seconds and the system no longer gets so sluggish.
The max file size value chosen also matches the `MAX_BLOCKFILE_SIZE` file size setting already used by the block storage.
ACKs for top commit:
l0rinc:
ACK b73d3319377a4c9d7e2dd279c3d106002585bc36
davidgumberg:
ACK b73d331937
andrewtoth:
ACK b73d3319377a4c9d7e2dd279c3d106002585bc36
TheCharlatan:
ACK b73d3319377a4c9d7e2dd279c3d106002585bc36
willcl-ark:
ACK b73d3319377a4c9d7e2dd279c3d106002585bc36
tdb3:
ACK b73d3319377a4c9d7e2dd279c3d106002585bc36
laanwj:
ACK b73d3319377a4c9d7e2dd279c3d106002585bc36
Tree-SHA512: 5d8fb9ad1ea643fb3e42a9c59f6fc90cc5cc3b82c06d9b8d59de3a5a926fabaeb78efb51b608b1e7925f49d82dfcbd5b72c552993879789f33201efe57c278f3
935973b315fa3b99f5e79b360bcb66f473bb7b95 Remove `src/config` directory (Hennadii Stepanov)
Pull request description:
The `src/config` directory has not been used since the migration to CMake, which disables in-source builds.
ACKs for top commit:
TheCharlatan:
ACK 935973b315fa3b99f5e79b360bcb66f473bb7b95
BrandonOdiwuor:
ACK 935973b315fa3b99f5e79b360bcb66f473bb7b95
Tree-SHA512: cc5b405e39387673fa2fd1e96680295b6eb3dc49a5f9a4d288580b8ea83efba04c96132811ca2cec14bcca509dbaf20c390cd47dabeea2a6ebc973e364e7a43b
The default max file size for LevelDB is 2 MiB, which results in the
LevelDB compaction code generating ~4 disk cache flushes per second when
syncing with the Bitcoin network.
These disk cache flushes are triggered by fdatasync() syscall issued by the
LevelDB compaction code when reaching the max file size.
If the database is on a HDD this flush rate brings the whole system to a
crawl.
It also results in very slow throughput since 2 MiB * 4 flushes per second
is about 8 MiB / second max throughput, while even an old HDD can pull
100 - 200 MiB / second streaming throughput.
Increase the max file size for LevelDB to 32 MiB instead so the flush rate
drops significantly and the system no longer gets so sluggish.
The new max file size value chosen is a compromise between the one that
works best for HDD and SSD performance, as determined by benchmarks done by
various people.
160799d9135528dbdea40690f0bb0d56c6c4803a test: refactor: introduce `create_ephemeral_dust_package` helper (Sebastian Falbesoner)
61e18dec306cfb8bc17ad2133ea1867b78000c62 doc: ephemeral policy: add missing closing double quote (Sebastian Falbesoner)
Pull request description:
This small PR contains ephemeral dust follow-ups mentioned in #30329 that were not tackled in the first follow-up PR #31279:
https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828577696https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825279952
Happy to add more if I missed some or anyone has concrete commits to add.
ACKs for top commit:
rkrux:
tACK 160799d9135528dbdea40690f0bb0d56c6c4803a
instagibbs:
ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
tdb3:
Code review ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
Tree-SHA512: e9a80c6733f1e7fe9e834d81b404f6e8ef7a61fe986f61b3dcdbda1a0bc547145fc279ec02f54361df56cb4e62a6fedaa0f3991c6e084c3a703ed1b1bfbdbe4e
37a5c5d83664c31d83fc649d3c8c858bd5f10f21 doc: update descriptors.md for getdescriptoractivity (James O'Beirne)
ee3ce6a4f4d35afe7fcab16eff419a6788b02170 test: rpc: add no address case for getdescriptoractivity (James O'Beirne)
811f76f3a511d20750046319b390e225a1151caa rpc: add getdescriptoractivity (James O'Beirne)
25fe087de59e967ce968d35ed77138325eb9a9fa rpc: move-only: move ScriptPubKeyDoc to utils (James O'Beirne)
Pull request description:
The RPC command `scanblocks` provides a useful way to get a set of blockhashes that have activity relevant to a set of descriptors (`relevant_blocks`). However actually extracting the activity from those blocks is left as an exercise to the end user.
This process involves not only generating the (potentially ranged) set of scripts for the descriptor set on the client side (maybe via `deriveaddresses`), but then the user must retrieve each block's contents one-by-one using `getblock <hash>`, which is transmitted over a network link. And that's all before they perform the actual search over block content. There's even more work required to incorporate unconfirmed transactions.
This PR introduces an RPC `getdescriptoractivity` that [dovetails](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-08-16#1046393;) with `scanblocks` output, handling the process described above. Users specify the blockhashes (perhaps from `relevant_blocks`) and a set of descriptors; they are then given all spend/receive activity in that set of blocks.
This is a very useful tool when implementing lightweight wallets that want neither to require a third-party indexer like electrs, nor the overhead of creating and managing watch-only wallets in Core. This allows Core to be more easily used in a "stateless" manner by wallets, with potentially many nodes interchangeably acting as backends.
### Example usage
```
% ./src/bitcoin-cli scanblocks start \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' \
857263
{
"from_height": 857263,
"to_height": 858263,
"relevant_blocks": [
"00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"
],
"completed": true
}
% ./src/bitcoin-cli getdescriptoractivity \
'["00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88", "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"]' \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]'
{
"activity": [
{
"type": "receive",
"amount": 0.00002900,
"blockhash": "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"height": 857907,
"txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"vout": 254,
"output_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
},
{
"type": "spend",
"amount": 0.00002900,
"blockhash": "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb",
"height": 858260,
"spend_txid": "7f61d1b248d4ee46376f9c6df272f63fbb0c17039381fb23ca5d90473b823c36",
"spend_vin": 0,
"prevout_txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"prevout_vout": 254,
"prevout_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
}
]
}
```
ACKs for top commit:
instagibbs:
reACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
achow101:
ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
tdb3:
Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
rkrux:
re-ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
Tree-SHA512: 04aa51e329c6c2ed72464b9886281d5ebd7511a8a8e184ea81249033a4dad535a12829b1010afc2da79b344ea8b5ab8ed47e426d0bf2eb78ab395d20b1da8dbb
11f3bc229ccd4b20191855fb1df882cfa6145264 refactor: Reserve vectors in fuzz tests (Lőrinc)
152fefe7a22b7da3cfe2815083634bece9c5654e refactor: Preallocate PrevectorFillVector(In)Direct without vector resize (Lőrinc)
a774c7a339c26b1409c9a9572d2b52810ee64062 refactor: Fix remaining clang-tidy performance-inefficient-vector errors (Lőrinc)
Pull request description:
PR inspired by https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307 (and https://github.com/bitcoin/bitcoin/pull/29458, https://github.com/bitcoin/bitcoin/pull/29606, https://github.com/bitcoin/bitcoin/pull/29607, https://github.com/bitcoin/bitcoin/pull/30093).
The `clang-tidy` check can be run via:
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-inefficient-vector-operation' | grep -v 'clang-tidy'
```
which revealed 3 tests and 1 prod warning (+ fuzz and benching, found by hebasto).
Even though the tests aren't performance critical, getting rid of these warnings (for which the checks were already enabled via https://github.com/bitcoin/bitcoin/blob/master/src/.clang-tidy#L18, see below), the fix was quite simple.
<details>
<summary>clang-tidy -list-checks</summary>
```bash
cd src && clang-tidy -list-checks | grep 'vector'
performance-inefficient-vector-operation
```
</details>
<details>
<summary>Output before the change</summary>
```
src/test/rpc_tests.cpp:434:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
433 | for (int64_t i = 0; i < 100; i++) {
434 | feerates.emplace_back(1 ,1);
| ^
src/test/checkqueue_tests.cpp:366:13: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
365 | for (size_t i = 0; i < 3; ++i) {
366 | tg.emplace_back(
| ^
src/test/cuckoocache_tests.cpp:231:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
228 | for (uint32_t x = 0; x < 3; ++x)
229 | /** Each thread is emplaced with x copy-by-value
230 | */
231 | threads.emplace_back([&, x] {
| ^
src/rpc/output_script.cpp:127:17: error: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
126 | for (unsigned int i = 0; i < keys.size(); ++i) {
127 | pubkeys.push_back(HexToPubKey(keys[i].get_str()));
| ^
```
And the fuzz and benchmarks, noticed by hebasto: https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2483124499
</details>
ACKs for top commit:
maflcko:
review ACK 11f3bc229ccd4b20191855fb1df882cfa6145264 🎦
achow101:
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264
theuni:
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264
hebasto:
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264, tested with clang 19.1.5 + clang-tidy.
Tree-SHA512: 41691c19f35c63b922a95407617a54f9bff1af3f95f99d15642064f321df038aeb1ae5f061f854ed913f69036807cc28fa6222b2ff4c24ef43b909027fa0f9b3
c288c790cd9abe91e53164aba5d975ef1e26ee3f interpreter: Use the same type for SignatureHash in the definition (TheCharlatan)
Pull request description:
This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.
The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with:
```
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
/home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
```
With this patch it is possible to link against the static consensus library and produce a fully static executable.
ACKs for top commit:
l0rinc:
ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
maflcko:
review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f 🐺
achow101:
ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
theuni:
Obvious fix ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f.
BrandonOdiwuor:
Code Review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
Tree-SHA512: 74f283637f0a9cd0cab65d3502f2f8fc4fb983c7672f24e7a76ba2eb6e53b4a81cca0aacb610ef39ac0a454305be594ab440a697ae3718987bf5dbcbc7146a31
And under the hood suppoert single transactions
in AcceptPackage. This simplifies user experience
and paves the way for reducing number of codepaths
for transaction acceptance in the future.
Co-Authored-By: instagibbs <gsanders87@gmail.com>
* Since the main LIMITED_WHILE stated `outpoints.size() < 200'000`, I've presized outpoints accordingly.
* `tx_mut.vin` and `tx_mut.vout` weren't caught by the clang-tidy, but addressed them anyway.
This was missed during the original PR switching the nHashType argument
to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.
The problem was discovered after running into a linker error when
attempting to link this code as a static library using the header as a
declaration with a riscv32 bare metal toolchain. The compiler would
error with:
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
/home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
