Some uses of non-threadsafe `strerror` have snuck into the code since
they were removed in #4152. Add a wrapper `SysErrorString` for
thread-safe strerror alternatives and replace all uses of `strerror`
with this.
2ff8f4dd81 Add tests for addr destination rotation (Gleb Naumenko)
77ccb7fce1 Use std::chrono for salting when randomizing ADDR destination (Gleb Naumenko)
Pull request description:
We currently assign a destination peer for relaying particular addresses of nodes every 24 hours, and then rotate. This is done for rate-limiting (ultimately for privacy leak reduction I think?).
Before this change, 24 hours was defined as uint. I replaced it with std::chrono, which is mockable and type-safe.
Also added couple tests for this behavior.
ACKs for top commit:
jonatack:
ACK 2ff8f4dd81
Tree-SHA512: 16f703ef3ffee13ce3afa82ca7b4baa27308af18cd2eececdce5565badfb68656a2ad9c4594b73772e4bfa99b3fb15f8e4089c1cb4be98c0bae6730a9d2f8a25
fa7078d84f scripted-diff: Rename ValidAsCString to ContainsNoNUL (MacroFake)
e7d2fbda63 Use std::string_view throughout util strencodings/string (Pieter Wuille)
8ffbd1412d Make DecodeBase{32,64} take string_view arguments (Pieter Wuille)
1a72d62152 Generalize ConvertBits to permit transforming the input (Pieter Wuille)
78f3ac51b7 Make DecodeBase{32,64} return optional instead of taking bool* (Pieter Wuille)
a65931e3ce Make DecodeBase{32,64} always return vector, not string (Pieter Wuille)
a4377a0843 Reject incorrect base64 in HTTP auth (Pieter Wuille)
d648b5120b Make SanitizeString use string_view (Pieter Wuille)
963bc9b576 Make IsHexNumber use string_view (Pieter Wuille)
40062997f2 Make IsHex use string_view (Pieter Wuille)
c1d165a8c2 Make ParseHex use string_view (Pieter Wuille)
Pull request description:
Make use of `std::string_view` and `std::optional` in the util/{strencodings, string} files.
This avoids many temporary string/vector objects being created, while making the interface easier to read. Changes include:
* Make all input arguments in functions in util/strencodings and util/string take `std::string_view` instead of `std::string`.
* Add `RemovePrefixView` and `TrimStringView` which also *return* `std::string_view` objects (the corresponding `RemovePrefix` and `TrimString` keep returning an `std::string`, as that's needed in many call sites still).
* Stop returning `std::string` from `DecodeBase32` and `DecodeBase64`, but return vectors. Base32/64 are fundamentally algorithms for encoding bytes as strings; returning `std::string` from those (especially doing it conditionally based on the input arguments/types) is just bizarre.
* Stop taking a `bool* pf_invalid` output argument pointer in `DecodeBase32` and `DecodeBase64`; return an `std::optional` instead.
* Make `DecodeBase32` and `DecodeBase64` more efficient by doing the conversion from characters to integer symbols on-the-fly rather than through a temporary vector.
ACKs for top commit:
MarcoFalke:
re-ACK fa7078d84f only change is rebase and adding a scripted-diff 🍲
martinus:
Code review ACK fa7078d84f, found no issue
laanwj:
Code review ACK fa7078d84f
sipa:
utACK fa7078d84f (as far as the commit that isn't mine goes)
Tree-SHA512: 5cf02e541caef0bcd100466747664bdb828a68a05dae568cbcd0632a53dd3a4c4e85cd8c48ebbd168d4247d5c9666689c16005f1c8ad75b0f057d8683931f664
ab73d5985d Do not pass `WalletModel*` to queued connection (Hennadii Stepanov)
fdf7285950 refactor: Make `RPCExecutor*` a member of the `RPCConsole` class (Hennadii Stepanov)
61457c179a refactor: Guard `RPCConsole::{add,remove}Wallet()` with `ENABLE_WALLET` (Hennadii Stepanov)
Pull request description:
On master (094d9fda5c), the following queued connection 094d9fda5c/src/qt/rpcconsole.cpp (L1107) uses a `const WalletModel*` parameter regardless whether the `ENABLE_WALLET` macro is defined.
Although this code works in Qt 5, it is flawed. On Qt 6, the code gets broken because the fully defined `WalletModel` type is required which is not the case if `ENABLE_WALLET` is undefined.
This PR fixes the issue described above.
ACKs for top commit:
promag:
ACK ab73d5985d
jarolrod:
code review ACK ab73d5985d
Tree-SHA512: 544ba984da4480aa34f1516a737d6034eb5616b8f78db38dc9bf2d15c15251957bc0b0c9b0d5a365552da9b64a850801a6f4caa12b0ac220f51bd2b334fbe545
Base32/base64 are mechanisms for encoding binary data. That they'd
decode to a string is just bizarre. The fact that they'd do that
based on the type of input arguments even more so.
fa1970f075 Make BlockManager::LoadBlockIndex private (MarcoFalke)
Pull request description:
* After commit fa27f03b49 `BlockManager::LoadBlockIndex` is only called by `BlockManager::LoadBlockIndexDB`. Thus, it can be made `private`.
* After commit c600ee3816 `m_best_invalid` is no longer accessed by `BlockManager::LoadBlockIndex`. Thus, the unused `friend` can be removed.
ACKs for top commit:
mruddy:
ACK fa1970f075 I verified by double checking references, then applying the patch, and running `make check`. LGTM.
Tree-SHA512: 9b36b4c59bf7ad01171764ce61b1be9750fc92d105c4fe939b1a6a70027ab6300d5d2a2fc3e82f981e22c3987f2ca84e092d2e1f8463fa320af9f05048580c0a
71c3f0356c move-only: Rename index + pruning functional test (Fabian Jahr)
de08932efa test: Update test for indices on pruned nodes (Fabian Jahr)
825d19839b Index: Allow coinstatsindex with pruning enabled (Fabian Jahr)
f08c9fb0c6 Index: Use prune locks for blockfilterindex (Fabian Jahr)
2561823531 blockstorage: Add prune locks to BlockManager (Fabian Jahr)
231fc7b035 refactor: Introduce GetFirstStoredBlock helper function (Fabian Jahr)
Pull request description:
# Motivation
The main motivation of this change and only behavior change noticeable by user is to allow running `coinstatsindex` on pruned nodes as has been requested [here for example](https://twitter.com/benthecarman/status/1388170854140452870?s=20).
# Background
`coinstatsindex` on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run `blockfilterindex`on pruned nodes was added in #15946 but it also added the `blockfilterindex` as a dependency to `validation` and it introduced two new circular dependencies. Enabling `coinstatsindex` on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.
Instead, this PR introduces a `m_prune_blockers` map to `BlockManager` as a flexible approach to block pruning. Entities like `blockfilterindex`, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.
# Alternative approach
Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:
- Usage of globals
- Blocks pruning with a start and a stop height
- Can persist blockers across restarts
- Blockers can be set/unset via RPCs
Personally, I don't think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.
ACKs for top commit:
mzumsande:
Code review ACK 71c3f0356c
ryanofsky:
Code review ACK 71c3f0356c. Changes since last review: just tweaking comments and asserts, and rebasing
w0xlt:
tACK 71c3f0356c on signet.
Tree-SHA512: de7efda08b44aa31013fbebc47a02cd2de32db170b570f9643e1f013fee0e8e7ca3068952d1acc6e5e74a70910735c5f263437981ad73df841ad945b52d36b71
ab5af9ca72 test: Add test for coinselection tracepoints (Andrew Chow)
ca02b68e8a doc: document coin selection tracepoints (Andrew Chow)
8e3f39e4fa wallet: Add some tracepoints for coin selection (Andrew Chow)
15b58383d0 wallet: compute waste for SelectionResults of preset inputs (Andrew Chow)
912f1ed181 wallet: track which coin selection algorithm produced a SelectionResult (Andrew Chow)
Pull request description:
Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:
1. After `SelectCoins` returns in order to observe the `SelectionResult`
2. After the first `CreateTransactionInternal` to observe the created transaction
3. Prior to the second `CreateTransactionInternal` to notify that the optimistic avoid partial spends selection is occurring
4. After the second `CreateTransactionInternal` to observe the created transaction and inform which solution is being used.
This PR also adds an algorithm enum to `SelectionResult` so that the first tracepoint will be able to report which algorithm was used to produce that result.
The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.
ACKs for top commit:
jb55:
crACK ab5af9ca72
josibake:
crACK ab5af9ca72
0xB10C:
ACK ab5af9ca72. Code reviewed, ran the `interface_usdt_coinselection.py` test, and tested with the above bpftrace script (updated `%d` -> `%ld` where necessary, ty achow101).
Tree-SHA512: a4bf7a910cdf464622f2f3b5d44c15b891f24852df6e7f8c5b177fe3d8aaa4a1164593a24c3960eb22b16544fa7140e5c745345367b9e291b78395084c0ac8ff
bae4561938 scripted-diff: rename BytePtr to AsBytePtr (João Barbosa)
Pull request description:
Building with iPhoneOS SDK fails because it also has `BytePtr` defined
in [/usr/include/MacTypes.h](https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/MacTypes.h.auto.html):
```cpp
typedef UInt8 * BytePtr;
```
ACKs for top commit:
MarcoFalke:
cr ACK bae4561938
laanwj:
Code review ACK bae4561938
sipa:
utACK bae4561938
prusnak:
Approach ACK bae4561938
Tree-SHA512: fb4d4a94c9c2238107952c071bae9bf6bbde6ed6651f6d300f222adf8a6a423f0567cbbcc3102d4167ef2e4e6f9988a2f91d75a5418bf6bcd64eebb4bcd1077f
4637bbe448 rpc: Explain active and internal in listdescriptors (Andrew Chow)
Pull request description:
The current help text for active and internal in listdescriptors is not particularly helpful. They require the reader to already know what those terms mean. This help text is updated to actually explain the definitions of those words in context of a descriptor wallet.
ACKs for top commit:
S3RK:
ACK 4637bbe448
jarolrod:
ACK 4637bbe448
w0xlt:
ACK 4637bbe448
Tree-SHA512: 0af2c04f3b9920799cf616ad618bde9248eb9f74cc28f443b5b0f6646deba76e9b1415aca0865ad3bcc24aa6af0e9d07ad7b7cd80f0fe80838cf847f1b944426
9c96f1008b tidy: enable modernize-use-nullptr (fanquake)
e53274868e Don't use zero as null pointer constant (-Wzero-as-null-pointer-constant) (practicalswift)
Pull request description:
Alternative to #15112 which uses `clang-tidy` to do perform the checking, rather than `-Wzero-as-null-pointer-constant`, and avoids having to uses pragmas, i.e:
```cpp
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
#if defined(HAVE_W_ZERO_AS_NULL_POINTER_CONSTANT)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
#pragma GCC diagnostic ignored "-Wunknown-pragmas"
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wzero-as-null-pointer-constant"
#endif
```
to suppress warnings coming from upstream code.
Can be tested by dropping the preceding commit. Should produce errors like:
```bash
clang-tidy-14 --use-color -p=/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu /home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/netbase.cpp
/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/netbase.cpp:678:36: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
if (!Socks5(strDest, port, 0, sock)) {
^
nullptr
```
ACKs for top commit:
laanwj:
Concept and code review ACK 9c96f1008b
Tree-SHA512: d822a354e44ba8f7fc53da9a4be7de5c25cc4ffc7c57651b76fdd1a030764b0390cfd79fca94685b8a3ff4f4d13054764f12d1f0d8c2a1b9ba519a7524f7f5bf
fa870e3d4c Remove not needed clang-format off comments (MarcoFalke)
Pull request description:
It seems odd to disable clang-format and force manual formatting when there is no need for it. So remove the clang-format comments and other unneeded comments.
Can be reviewed with `--word-diff-regex=. --ignore-all-space`
Looks like this was initially added in commit d9d79576f4 to accommodate a linter that has since been removed and replaced by a functional test.
ACKs for top commit:
laanwj:
Code review ACK fa870e3d4c
fanquake:
ACK fa870e3d4c
Tree-SHA512: 0f8f97c12f5dbe517dd96c10b10ce1b8772d8daed33e6b41f73ea1040e89888cf3b8c0ad7b20319e366fe30c71e8b181c89098ae7f6a3deb8647e1b4731db815
dac44fc06f init: disallow reindex-chainstate with optional indexes (Martin Zumsande)
62e14285f9 doc: Add note that -reindex will rebuild optional indexes (Martin Zumsande)
Pull request description:
When started together with `-reindex-chainstate`, currently coinstatsindex gets corrupted and the blockfilterindex flatfiles duplicated. See the OP of #24630 for more a more detailed explanation on why this happens.
This is an alternative to #24630 which does not wipe and rebuild the indexes but returns an `InitError` when they are activated, thus requiring the user to deactivate them temporarily until the `-reindex-chainstate` run is finished.
This also disallows `-reindex-chainstate` in combination with `-txindex`, which is not leading to corruption, but currently still rebuilds the index unnecessarily and unexpectedly.
As a long-term goal, it would be desirable to have the indexes tolerate `reindex-chainstate` by ignoring their `BlockConnected` notifications (there is discussion in #24630 about this) or possibly move `reindex-chainstate` option into a `bitcoin-chainstate` executable, which could also solve the problem. But these would be larger projects - until then, it might be better to disallow the interaction than having corrupted indexes.
The first commit adjusts the `-reindex` doc to mention that this option does rebuild all active indexes.
ACKs for top commit:
ryanofsky:
Code review ACK dac44fc06f. Just fixed IsArgSet call and edited error messages since last review
Tree-SHA512: c1abf7d350648ae227c3fd6c95d9a54c3bac9de70915275dea1c87cca6d9a76a056c0e306d95ef8cfe4df1f8525b418e0e7a4f52ded3be464041c0dc297f8930
df08c23f01 Precomputed hashes are note #16 in BIP341 (Gregory Sanders)
Pull request description:
Seems to have drifted one space
ACKs for top commit:
fanquake:
ACK df08c23f01
Tree-SHA512: f0e959743f67ad4b46584f44305d27a89b52874d70091e004ec05dfd2f8c6481e9edceecb0af98f519ad3debb0c0bb26fa27f370545b6e15f366bd0af1158bab
a62e84438d fuzz: add `SplitString` fuzz target (MarcoFalke)
4fad7e46d9 test: add unit tests for `SplitString` helper (Kiminuo)
9cc8e876e4 refactor: introduce single-separator split helper `SplitString` (Sebastian Falbesoner)
Pull request description:
This PR adds a simple string split helper `SplitString` that takes use of the spanparsing `Split` function that was first introduced in #13697 (commit fe8a7dcd78). This enables to replace most calls to `boost::split`, in the cases where only a single separator character is used. Note that while previous attempts to replace `boost::split` were controversial (e.g. #13751), this one has a trivial implementation: it merely uses an internal helper (that is unit tested and in regular use with output descriptiors) and converts its result from spans to strings. As a drawback though, not all `boost::split` instances can be tackled.
As a possible optimization, one could return a vector of `std::string_view`s (available since C++17) instead of strings, to avoid copies. This would need more carefulness on the caller sites though, to avoid potential lifetime issues, and it's probably not worth it, considering that none of the places where strings are split are really performance-critical.
ACKs for top commit:
martinus:
Code review ACK a62e84438d. Ran all tests. I also like that with `boost::split` it was not obvious that the resulting container was cleared, and with `SplitString` API that's obvious.
Tree-SHA512: 10cb22619ebe46831b1f8e83584a89381a036b54c88701484ac00743e2a62cfe52c9f3ecdbb2d0815e536c99034558277cc263600ec3f3588b291c07eef8ed24
Building with iPhoneOS SDK fails because it also has `BytePtr` defined
in /usr/include/MacTypes.h.
-BEGIN VERIFY SCRIPT-
sed -i 's/BytePtr/AsBytePtr/' $(git grep -l "BytePtr" src)
-END VERIFY SCRIPT-
709af67add p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex (w0xlt)
8be75fd0f0 p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex` (w0xlt)
a237a065cc scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex (w0xlt)
Pull request description:
Related to #19303, this PR gets rid of the RecursiveMutex `cs_totalBytesSent` and also adds `AssertLockNotHeld` macros combined with `LOCKS_EXCLUDED` thread safety annotations to avoid recursive locking.
ACKs for top commit:
jonatack:
ACK 709af67add per `git range-diff 7a4ac71 eff7918 709af67`, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative
vasild:
ACK 709af67add
hebasto:
ACK 709af67add, tested on Ubuntu 22.04.
Tree-SHA512: 560b4e6c92b1511911d69185207df6ee809db09b96d97f96430d8d2595dc05c98cc691aaec8a58ef87cf2ab0a98675c210b8ce0be3dedb81e31114bbbfdfd8be
This change also introduces an aditional buffer of 10 blocks (PRUNE_LOCK_BUFFER) that will not be pruned before the best block.
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
The current help text for active and internal in listdescriptors is not
particularly helpful. They require the reader to already know what those
terms mean. This help text is updated to actually explain the
definitions of those words in context of a descriptor wallet.
ee02c8bd9a util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès)
Pull request description:
This PR replaces the macro `CHECK_NONFATAL` with an identity function.
I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: https://github.com/bitcoin/bitcoin/pull/24804#discussion_r846182474.
Also adds `UNREACHABLE_NONFATAL` macro.
ACKs for top commit:
jonatack:
ACK ee02c8bd9a
MarcoFalke:
ACK ee02c8bd9a🍨
Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2
6958a26aa1 Revert "qt: Add ObjectInvoke template function" (Hennadii Stepanov)
249984f4f9 qt: Replace `GUIUtil::ObjectInvoke()` with `QMetaObject::invokeMethod()` (Hennadii Stepanov)
Pull request description:
A comment in 5659e73493 states that `GUIUtil::ObjectInvoke`
> can be replaced by a call to the QMetaObject::invokeMethod functor overload after Qt 5.10
ACKs for top commit:
w0xlt:
tACK 6958a26aa1 on Ubuntu 21.10, Qt 5.15.2.
promag:
Code review ACK 6958a26aa1.
Tree-SHA512: 6a840289568113cf38df6c1092821d626c2d206768a21d4dc6846b9dcccb4130477adb45ba718bb6bc15a3041871a7df3238983ac03db80406732be597693266
36f814c0e8 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e63 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4580 [init] Add netgroupman to node.context (John Newbery)
9b3836710b [build] Add netgroup.cpp|h (John Newbery)
Pull request description:
The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.
This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.
ACKs for top commit:
mzumsande:
Code Review ACK 36f814c0e8
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c0e8, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e8
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
fad6d4f952 Remove not needed ArithToUint256 roundtrips in tests (MarcoFalke)
fa456ccb22 Remove duplicate static_asserts (MarcoFalke)
Pull request description:
No need to go from `arith_uint256`->`uint256` when a `uint256` can be constructed right away.
ACKs for top commit:
laanwj:
Code review ACK fad6d4f952
Tree-SHA512: bea901ea5904bf61a0dadf7168c6b126f7e62ff1180d4aa72063c28930a01a8baa57ab0d324226bd4de72fb59559455c29c049d90061f888044198aae1426dcb
3ae7791bca refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bca
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
