This is used for argument parsing in the retry script, however we don't
use the script with any arguments. So remove the unused code, and the
dependency on gnu-getopt.
This came up in the context of adding new CI jobs, where gnu-getopt
might not be available, or working properly. It seemed easier to just
remove the unused code, than look for more workarounds.
14f99cfe53f07280b6f047844fc4fba0da8cd328 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595be7053b17895f7ee36652bac24be6e util: add `TicksSeconds` (Lőrinc)
Pull request description:
### Problem
`bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP).
This breaks the expectation that uptime reflects process runtime.
### Fix
Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC.
GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections.
### Reproducer
Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`):
Or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
DATA_DIR=$(mktemp -d)
./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime
sleep 1
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 ))
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop
```
<details>
<summary>Before (uptime jumps with wall clock)</summary>
```bash
Bitcoin Core starting
0
20000001
Bitcoin Core stopping
```
</details>
<details>
<summary>After (uptime stays monotonic)</summary>
```bash
Bitcoin Core starting
0
1
Bitcoin Core stopping
```
</details>
----------
Issue: https://github.com/bitcoin/bitcoin/issues/34326
ACKs for top commit:
maflcko:
review ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328 🎦
willcl-ark:
tACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
w0xlt:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
sedited:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
b39291f4cde03d5aa7936bf5aa7cc4fa18f65cad doc: fix `-logips` description to clarify that non-debug logs can also contain IP addresses (Lőrinc)
c7028d3368e90fef2dd2a7ae68877767d602eff0 init: log that additional logs may contain privacy-sensitive information (Lőrinc)
31b771a9425dace38582e0de0fb468f388df170c net: move `privatebroadcast` logs to debug category (Lőrinc)
Pull request description:
### Motivation
The recently merged [private broadcast](https://github.com/bitcoin/bitcoin/pull/29415) is a privacy feature, and users may share `debug.log` with support.
Unconditional `LogInfo()` messages that mention private broadcast and/or include (w)txids can leak sensitive context (e.g. which transactions a user originated).
Since it's meant to be a private broadcast, we should minimize leaks.
It's a best effort, it's not invalidated by other logs possibly leaking identifiable information, those can be addressed separately.
We're not promising that the logs won't ever contain data that could be used against the user, but we should still try to minimize that data, especially for a feature that's advertised as privacy-focused.
Follow up to [#29415 (comment)](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2637012294)
### Changes
* Move private-broadcast event logs from `LogInfo()` to `LogDebug(BCLog::PRIVBROADCAST, ...)`, so they are only emitted when `-debug=privatebroadcast` was explicitly provided.
* Remove hardcoded `"[privatebroadcast]"` log-string prefixes (category logging already adds the prefix).
* Keep warning at the default log level for startup failures.
* Add an init log (not a warning since that would require excessive test framework updates) when any `-debug` categories are enabled that additional logs may contain privacy-sensitive information and should not be shared publicly.
* Update a related startup arg (`-logips`) to clarify that clarify that non-debug logs can also contain IP addresses.
### Reproducer
The new warning can be checked with:
```bash
./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 | grep 'Debug logging is enabled' | wc -l
0
./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 -debug | grep 'Debug logging is enabled' | wc -l
1
```
ACKs for top commit:
janb84:
re ACK b39291f4cde03d5aa7936bf5aa7cc4fa18f65cad
vasild:
ACK b39291f4cde03d5aa7936bf5aa7cc4fa18f65cad
andrewtoth:
ACK b39291f4cde03d5aa7936bf5aa7cc4fa18f65cad
frankomosh:
crACK b39291f4cde03d5aa7936bf5aa7cc4fa18f65cad .The approach and implementation look good. Moving private broadcast logs to debug only would effectively reduce privacy leaks for users sharing logs.
sedited:
ACK b39291f4cde03d5aa7936bf5aa7cc4fa18f65cad
Tree-SHA512: feca25ebe72a03948ba436e25f9a682947966c4c09627e8f20201ef3872ddbce1c636cd82f06be1afdc09cb80da305058667c0c2eaeadeb351311155325ea06f
3f5211cba8e73e8eb03781e6ec32ba9c4a263782 test: remove child_one/child_two (w)txid variables (naiyoma)
7cfe790820cf247e8a27bb8091defc54c74d6aec test: replace ValidWitnessMalleatedTx class with function (naiyoma)
81675a781f3ab62a0576a9739d13b4997b63230d test: use pre-generated chain (naiyoma)
Pull request description:
This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated: `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.)
Together, these changes reduce complexity and improve test runtime.
ACKs for top commit:
stratospher:
reACK 3f5211c.
cedwies:
reACK 3f5211c
maflcko:
review ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782 👥
rkrux:
ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782
Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc
a73a3ec5532ddc05c1b013d868d9994f2889c9cf doc: fix invalid arg name hints for bugprone validation (Lőrinc)
Pull request description:
The extra leading `=` or missing trailing `=` prevented clang-tidy's `bugprone-argument-comment` check from validating the parameter name, as it only matches comments formatted strictly as `/*parameter_name=*/` (see https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html).
I have considered doing a scripted diff, but the values I found aren't so numerous and can easily be reviewed manually.
ACKs for top commit:
b-l-u-e:
ACK a73a3ec tested and saw that argument comments now use the strict "/*param=*/" format required by bugprone-argument-comment
Sjors:
utACK a73a3ec5532ddc05c1b013d868d9994f2889c9cf
maflcko:
review ACK a73a3ec5532ddc05c1b013d868d9994f2889c9cf 🍦
Tree-SHA512: 31177934d645116f381668a0f945028d7e04fab1fc6185dd0e3b7451aab71f89f1e4dd07246db667d1c4734eea3e5d73433c8b0e09181b3ece47dacc8677401e
fa37928536e0048a262260baf998ead026b14bb9 build: Temporarily remove confusing and brittle -fdebug-prefix-map (MarcoFalke)
Pull request description:
The compiler option `-fdebug-prefix-map` is unconditionally set by the build system. This is problematic for many reasons:
* Users and devs have no easy way to disable it without modifying the build system source code
* The mapping is broken since the cmake migration, and requires manual fixups such as https://github.com/bitcoin/bitcoin/issues/31204 or https://github.com/bitcoin/bitcoin/issues/31957
Fix all issues by temporarily removing it.
Though, the option is kept for the guix build, so that no change in behavior is observed for the release binaries.
Fixes https://github.com/bitcoin/bitcoin/issues/31957
Fixes https://github.com/bitcoin/bitcoin/issues/31204
The option can be added back in the future, if there is any need to. Though, adding it back should ideally work out of the box, or at least provide easy workarounds for all commonly used tooling.
ACKs for top commit:
pinheadmz:
ACK fa37928536e0048a262260baf998ead026b14bb9
l0rinc:
ACK fa37928536e0048a262260baf998ead026b14bb9
hebasto:
ACK fa37928536e0048a262260baf998ead026b14bb9.
Tree-SHA512: 5c76faab36ec516b286c2b5b2404e1488c0c4fbc678904593b0acb9c8da9b1db1b41436a22e6aa2f2671650288ccf635554773ef3144dc1df6ea838afce07ecb
eeee3755f8c415b227820479b5492261f3a8aa08 fuzz: Return chrono point from ConsumeTime(), Add ConsumeDuration() (MarcoFalke)
faa5a9ebad15fe41e8ddf45f11ad72bdc5aabf99 fuzz: Use min option in ConsumeTime (MarcoFalke)
Pull request description:
Returning a raw i64 is a bit confusing when it comes to chrono types. For example, in the addrman fuzz tests, the `time_penalty` is not a time point, but a duration.
Also, all call-sites assume second resolution right now, so document that better by returning `NodeSeconds` from `ConsumeTime(...)` and `std::chrono::seconds` from `ConsumeDuration(...)`.
ACKs for top commit:
l0rinc:
ACK eeee3755f8c415b227820479b5492261f3a8aa08
Crypt-iQ:
crACK eeee3755f8c415b227820479b5492261f3a8aa08
Tree-SHA512: 25dd779a1bf79fa42c6e69db0f0593ad4daa4c0d746e8e82a26bdd65391a27c38e484431056d4e2207b542c511a71cb536c259809728a7166b8d304c0490e321
ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f util: Remove `FilterHeaderHasher` (rustaceanrob)
Pull request description:
With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.
Further, the hasher for the key already exists via `BlockHasher`.
ref: https://en.cppreference.com/w/cpp/container/unordered_map.html
ACKs for top commit:
andrewtoth:
ACK ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f
maflcko:
lgtm ACK ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f
ismaelsadeeq:
ACK ccf9172ab3bbd6d6979acb9b02bc36ca55ab031f 👍🏾
Tree-SHA512: 607602391bf337d4e25b04a6a643fa32c3ab4599009b181b46ecdb0705e8ff2af89a6192042453c9e8e44abcb2150589019f02c5c944ecdff41322c3e0ad45ac
fa15a8d2d03b0099b97262e47a8cbf685c29dd49 doc: Explain that low-effort pull requests may be closed (MarcoFalke)
Pull request description:
Lately, there seems to be a rise in low-effort pull requests. For example, where a contributor does not seem to understand the changes they are submitting, or it becomes clear that they have not tested the changes at all.
I don't think such pull requests are helpful, as they extract precious review time, which could be better spent on reviewing pull requests by reviewers who care about understanding the changes they are submitting, and who ensure their changes are sound and tested.
So document that such low-effort pull request may be closed.
ACKs for top commit:
l0rinc:
ACK fa15a8d2d03b0099b97262e47a8cbf685c29dd49
willcl-ark:
ACK fa15a8d2d03b0099b97262e47a8cbf685c29dd49
dergoegge:
ACK fa15a8d2d03b0099b97262e47a8cbf685c29dd49
pinheadmz:
ACK fa15a8d2d0
Tree-SHA512: ba880f61c90c95e1e9007e337bad1a612a53ca85448f0ebfe97b34139489f22e5f709b8a0e302b11f71213e3b7863ab36ebd89b5c11cd550022d96493f917dd7
b261100e71697dd4859cdeb5a69f5a1d6c557099 [qt] Set peer version and subversion to N/A when not available or detecting (WakeTrainDev)
Pull request description:
In the debug console peer detail window, display "N/A" for the User Agent and Version when the peer is still detecting or the information is unavailable, instead of retaining the previous values.
ACKs for top commit:
maflcko:
lgtm ACK b261100e71697dd4859cdeb5a69f5a1d6c557099
luke-jr:
utACK b261100e71697dd4859cdeb5a69f5a1d6c557099
Tree-SHA512: ffcba716fe6173062fe00e2d428d41bbdcaebfe8c76c804519e46a448ade2785ae32efb1a30322adc19cf29e07ea8ab4d7593ef5a17b6c418c8dd77b381e4f77
be2b48b9f3e56b672e391e18a09f2d5f4e78d74e test: allow overriding tar in get_previous_releases (fanquake)
Pull request description:
Facilitate use on distros that might have `tar` as something else, such as `gtar`, i.e Chimera.
ACKs for top commit:
maflcko:
lgtm ACK be2b48b9f3e56b672e391e18a09f2d5f4e78d74e
hebasto:
ACK be2b48b9f3e56b672e391e18a09f2d5f4e78d74e, I have reviewed the code and it looks OK.
sedited:
ACK be2b48b9f3e56b672e391e18a09f2d5f4e78d74e
Tree-SHA512: e5da14ef17f37dc52e438f191efbe9041477246bab04b9e166cf2104389e97bfc6b41005c9d79a99f193e8d7b24ecd32dbd8344c65878e92fd94c77d3efbbe8c
40735450c00b10baa03e3a7f1e2bee439077e356 Remove unused epochguard.h (Suhas Daftuar)
1a8494d16c7b1c21dec384438c18ac08a469bb61 Rework CTxMemPool::GetChildren() to not use epochs (Suhas Daftuar)
Pull request description:
Since #33591, the epoch-based graph traversal optimization logic is only used for `CTxMempool::GetChildren()`, a function that is only used in RPC code and tests. Rewrite it without epochs, and remove `util/epochguard.h` itself, as that was its last use.
This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don't foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.
ACKs for top commit:
ajtowns:
ACK 40735450c00b10baa03e3a7f1e2bee439077e356
instagibbs:
ACK 40735450c00b10baa03e3a7f1e2bee439077e356
l0rinc:
code review ACK 40735450c00b10baa03e3a7f1e2bee439077e356
Tree-SHA512: 7ce7c04835cd2425a71c4fd47f316b6fb7381caa27383de7ecc4aa81100fcf7bc5e062699b307c08e0b853b35f06710d9ac761d6e660af9f9331e708d36f2fe0
9a9d797ef6ed8e1b3e876fc93cf1a6395ab270e9 kernel: Add support for block headers (yuvicc)
b851ff6cae71934bf2389d109908339d60ec6e5b kernel: Add Handle/View pattern for BlockValidationState (yuvicc)
Pull request description:
Adds a new `btck_BlockHeader` type and associated functions to create, access, and validate block headers. Block headers will have their own type (`btck_BlockHeader`) that can be created from raw data, copied, and queried for all the standard header fields (hash, prev hash, timestamp, bits, version, nonce). We can also extract headers from full blocks or block tree entries.
The first commit here refactors `BlockValidationState` to use Handle/View pattern so external code can own them, which is required for the header processing in the API.
#### New Block Header API
- **`btck_BlockHeader` type**: Opaque handle for block headers
- **Header methods**:
- `btck_block_header_create()`: Create header from 80-byte serialized data
- `btck_block_header_copy()`: Copy block headers
- `btck_block_header_destroy()`: Destroy header object
- `btck_block_header_get_hash()`: Calculate block hash
- `btck_block_header_get_prev_hash()`: Get previous block hash
- `btck_block_header_get_timestamp()`: Get block timestamp
- `btck_block_header_get_bits()`: Get difficulty target (compact format)
- `btck_block_header_get_version()`: Get block version
- `btck_block_header_get_nonce()`: Get proof-of-work nonce
- `btck_block_get_header()`: Extract header from a full block
- `btck_block_tree_entry_get_block_header()`: Get header associated with a block tree entry
- **Header Processing Methods:**
- **`btck_chainstate_manager_process_block_header()`**: Validates and processes a block header without requiring the full block. This performs proof-of-work verification, timestamp validation, and updates the internal chain state.
- **`btck_chainstate_manager_get_best_entry()`**: Returns the block tree entry with the most cumulative proof-of-work.
Why `btck_chainstate_manager_get_best_entry()` is included alongside header validation? Just as we have logic to get the tip for block validation (so you can request more blocks extending your best from your peers), we need the equivalent for header validation. To make header validation worthwhile, knowing what the best current header is seems useful—it tells you what headers to request next from peers.
### Testing
Added tests in `test_kernel.cpp` that cover creating headers from raw data, extracting all header fields, and processing headers through the chainstate manager.
CC sedited
ACKs for top commit:
stringintech:
re-ACK 9a9d797e
sedited:
Re-ACK 9a9d797ef6ed8e1b3e876fc93cf1a6395ab270e9
janb84:
ACK 9a9d797ef6ed8e1b3e876fc93cf1a6395ab270e9
Tree-SHA512: 1dde9ef860543c906d1bb5e604f0d2956e7382fcbb55090686261b2277270a1fd3826f02ecf1749b2774da66e88f686c7845172b4c68b62259e7a7aee0825fa2
1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9 string: add LineReader (Matthew Zipkin)
ee62405cce2bf3d14117bdb327832f12584968d6 time: implement and test RFC1123 timestamp string (Matthew Zipkin)
eea38787b9be99c3f192cb83fc18358397e4ab52 string: add AsciiCaseInsensitive{KeyEqual, Hash} for unordered map (Matthew Zipkin)
4e300df7123a402aef472aaaac30907b18a10c27 string: add `base` argument for ToIntegral to operate on hexadecimal (Matthew Zipkin)
0b0d9125c19c04c1fc19fb127d7639ed9ea39bec Modernize GetBindAddress() (Matthew Zipkin)
a0ca851d26f8a9d819708db06fec2465e9f6228c Make GetBindAddress() callable from outside net.cpp (Matthew Zipkin)
Pull request description:
This is a component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194). It is the first six commits of #32061 and provides a string-parsing utility (`LineReader`) that is also consumed by #34158.
These are the functions that are added / updated for HTTP and Torcontrol:
- `GetBindAddress()`: Given a socket, provides the bound address as a CService. Currently used by p2p but moved from `net` to `netbase` so other modules can call it.
- `ToIntegral()`: Already used to parse numbers from strings, added new argument `base = 10` so it can also be used to parse hexadecimal integers. HTTP chunked transfer-encoding uses hex-encoded integers to specify payload size: https://datatracker.ietf.org/doc/html/rfc7230.html#section-4.1
- `AsciiCaseInsensitive` comparators: Needed to store HTTP headers in an `unordered_map`. Headers are key-value pairs that are parsed with case-insensitive keys: https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
- `FormatRFC1123DateTime()`: The required datetime format for HTTP headers (e.g. `Fri, 31 May 2024 19:18:04 GMT`)
- `LineReader`: Fields in HTTP requests are newline-terminated. This struct is given an input buffer and provides methods to read lines as strings.
ACKs for top commit:
maflcko:
review ACK 1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9 👲
furszy:
utACK 1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9
sedited:
ACK 1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9
Tree-SHA512: bb8d3b7b18f158386fd391df6d377c9f5b181051dc258efbf2a896c42e20417a1b0b0d4637671ebd2829f6bc371daa15775625af989c19ef8aee76118660deff
fab2f3df4beb230eef63bdcf5042b6417c0012dc fuzz: Exclude too expensive inputs in descriptor_parse targets (MarcoFalke)
Pull request description:
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example, those will take several seconds (!) and the flamegraph shows that base58 encoding is the cause:
```
curl -fLO 'f5abf41608'
curl -fLO '78cb317546'
FUZZ=mocked_descriptor_parse ./bld-cmake/bin/fuzz ./f5abf41608addcef3538da61d8096c2050235032
FUZZ=descriptor_parse ./bld-cmake/bin/fuzz ./78cb3175467f53b467b949883ee6072e92dbb267
```
This will also break 32-bit fuzzing, see https://github.com/bitcoin/bitcoin/issues/34110#issuecomment-3759461248.
Fix all issues by checking for `HasTooLargeLeafSize`.
Sorry for creating several pull requests to fix this class of issue, but I think this one should be the last one. 😅
ACKs for top commit:
brunoerg:
reACK fab2f3df4beb230eef63bdcf5042b6417c0012dc
frankomosh:
re-ACK fab2f3df4beb230eef63bdcf5042b6417c0012dc
Tree-SHA512: 4ecf98ec4adc39f6e014370945fb1598cdd3ceba60f7209b00789ac1164b6d20e82a69d71f8419d9a40d57ee3fea36ef593c47fe48b584b6e8344c44f20a15c1
c9ce1c7c4a12b54ada7d48f100ec3a141ae99f86 test: Fix P2PK script test (billymcbip)
Pull request description:
I found another script_tests case that isn't behaving the way it was meant to. It's a P2PK spend where we add an `OP_NOP8` to the scriptSig to make it non-push-only. The test should check that [`scriptSig.IsPushOnly()`](691dc830c6/src/script/interpreter.cpp (L2055)) is only enforced in P2SH mode when the scriptPubKey actually matches the P2SH pattern. To test this, we need to **turn on the P2SH flag**.
ACKs for top commit:
sipa:
ACK c9ce1c7c4a12b54ada7d48f100ec3a141ae99f86
darosior:
utACK c9ce1c7c4a12b54ada7d48f100ec3a141ae99f86
Tree-SHA512: 0af1d7b4651478349abc97cf0c009488cf5af5f97135382f7dd37cef0ef9b563192244330899a54ee7e0296bf03ba702e37a7aa15248c5c0ab4745095efc2402
This is likely slightly slower, but this was the last place we were using
epochs instead of sets to deduplicate, and this is only used by the RPC code
and in tests, and should not be CPU-performance critical. Eliminating this
allows us to save 8 bytes in CTxMemPoolEntry.
Co-Authored-By: Pieter Wuille <bitcoin-dev@wuille.net>
8b9d30e3facff0cd132dc3faf6282d75b1f9b532 bench/test: clarify merkle bench and witness test intent (Lőrinc)
Pull request description:
Follow-up to #32497.
Clarify why the witness merkle test uses an odd leaf count (it exercises leaf duplication in `ComputeMerkleRoot()`), and make the coinbase witness hash initialization explicit.
Also simplify the leaf-copy loop in the `MerkleRoot` benchmark for readability.
No production code is changed in this follow-up, for simplicity and safety.
ACKs for top commit:
optout21:
ACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
maflcko:
lgtm ACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
achow101:
ACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
w0xlt:
ACK 8b9d30e3fa
danielabrozzoni:
tACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
Tree-SHA512: 6efca7c19ebf96bb8d0def4217ed30d3b74b58a7be15566967e98aba9b03aaddd0e0ebb3b8f43130b5f397a7d9eed0470a48a55438f440e0bceefb87edd16b27
This is a helper struct to parse HTTP messages from data in buffers
from sockets. HTTP messages begin with headers which are
CRLF-terminated lines (\n or \r\n) followed by an arbitrary amount of
body data. Whitespace is trimmed from the field lines but not the body.
https://httpwg.org/specs/rfc9110.html#rfc.section.5
https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
Field names in HTTP headers are case-insensitive. These
structs will be used in the headers map to search by key.
In libevent field names are also converted to lowercase for comparison:
evhttp_find_header()
evutil_ascii_strcasecmp()
EVUTIL_TOLOWER_()
d94d7b1a4b70cc93296c19fd221f8b8ac8ab90ee guix: stop passing depends sources to codesigning (fanquake)
Pull request description:
I think this is just a copy-pasta from the build container (which has existed since this file was introduced in 38eb91eb0616ed6dbe34c23e11588d130fef07f8). I don't see why we'd need the depends sources available when performing codesigning.
ACKs for top commit:
hebasto:
ACK d94d7b1a4b70cc93296c19fd221f8b8ac8ab90ee, I have reviewed the code and it looks OK.
willcl-ark:
ACK d94d7b1a4b70cc93296c19fd221f8b8ac8ab90ee
sedited:
tACK d94d7b1a4b70cc93296c19fd221f8b8ac8ab90ee
Tree-SHA512: 972b15aa022b79602f40c198187a54d85ceeee0014fd2232ca967bb52e4624cbb85b3ef1cdeac3ccd8c7b337a13c3be9c90291141495c8136a8e72ad2cd4ec4a
1137debb85306063f1660bc15850979e7db88fb8 doc: mempool: fix `removeUnchecked` incorrect comment (ismaelsadeeq)
Pull request description:
`CTxMemPool::removeUnchecked` description comment is stale and incorrect; the behaviour being described no longer applies in the post-cluster world. This PR is a simple fix that attempts to correctly describe what is being done in removeUnchecked.
ACKs for top commit:
instagibbs:
ACK 1137debb85306063f1660bc15850979e7db88fb8
sipa:
ACK 1137debb85306063f1660bc15850979e7db88fb8
Tree-SHA512: e410be57a83df50df01fcd6d7b07d08f0fe5a2abd229974f1ad269bb2e301608fd0d3912af349e2971f9a8abdbaa8e90c46d4832ec7b6858639642742b31a618
Introduces btck_BlockHeader type with accessor methods and btck_chainstate_manager_process_block_header() for validating headers without full blocks. Also, adds btck_chainstate_manager_get_best_entry() to query the header with most cumulative proof-of-work.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
- CTxMemPool::removeUnchecked description comment is stale and incorrect
after cluster mempool.
This commit fixes the issue by deleting the stale comment and describing
only the implicit behaviour triggered by the method.
e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478 test: Rename wallet name in restore attempt in wallet_assumeutxo (Fabian Jahr)
Pull request description:
I hope this fixes#34354
Based on this error from the logs `filesystem error: cannot remove: The process cannot access the file because it is being used by another process` it looks like there still exists a wallet file by the same name from the previous test case hasn't been cleaned up yet by it's process fully. This should be fixed by giving the failing `restorewallet` case a different wallet name and this shouldn't have any further effects on the rest of the test because is expected to fail anyway. The following (successful) call already uses a different wallet name.
ACKs for top commit:
achow101:
ACK e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478
w0xlt:
ACK e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478
rkrux:
ACK e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478
Tree-SHA512: b5c53252a3b71fde150b29cc90cfd80a8678e3d7a39bcd6038e6722f2ac50d0a0db480e0a8ad43e39d4738971c39280415822e4d64c02895cbb6bd05ff3fc02e
fa61fadad1c3df0274c3ddd351b8d68a0c4fe644 doc: Fix wrong code in WITH_LOCK doxygen comment (MarcoFalke)
Pull request description:
The typo is harmless, but a bit confusing every time i read it
ACKs for top commit:
hebasto:
re-ACK fa61fadad1c3df0274c3ddd351b8d68a0c4fe644.
l0rinc:
ACK fa61fadad1c3df0274c3ddd351b8d68a0c4fe644
Tree-SHA512: 302a284198178954512267e8c0a5708738d77aac1cf609d8cbb386bee78d705f7e0df42a7bd8300afc18d42fa271c7f4cda932b1cbea33385622b3760bb95fad
6a8dbf9b9352db48580cd81c8dac027f3138fedf p2p: add validation check for initial self-announcement (frankomosh)
Pull request description:
This is a follow up to #34146 . Adds validation check to the initial self-announcement code path. `IsAddrCompatible()` check can prevent sending non-routable addresses to peers that don't support addrv2.
ACKs for top commit:
fjahr:
utACK 6a8dbf9b9352db48580cd81c8dac027f3138fedf
Crypt-iQ:
crACK 6a8dbf9b9352db48580cd81c8dac027f3138fedf
stratospher:
ACK 6a8dbf9. preserves the existing behaviour. also learnt that Addr-fetch ADDR processing logic allows receiving a self-announcement with 1 address [without disconnecting](b6c5d1e450) and won't be affected.
sedited:
ACK 6a8dbf9b9352db48580cd81c8dac027f3138fedf
Tree-SHA512: 988110d72fd698634111eb68c0204f42457b9b9b3d7b6ca3e11815cc702f6921266ae8f27f27aa31c3672efdb99478870fc4d1e8f5fa63aceae6f81521b31d8b
This prevents potential intermittend failures on windows when the wallet by the same name from the previous test case hasn't been cleaned up yet by it's process.
Follow-up to bitcoin/bitcoin#32497.
Clarify why the witness merkle test uses an odd leaf count (it exercises leaf duplication in `ComputeMerkleRoot()`), and make the coinbase witness hash initialization explicit.
Also simplify the leaf-copy loop in the MerkleRoot benchmark for readability.
No production code is changed in this follow-up, for simplicity and safety.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
0aba464ce76522f1be3bb9e471b45438738de492 test: switch order of error code and message check (rkrux)
Pull request description:
I feel it'd be easier to debug intermittent test failures if the error message is present in the logs instead of error code. So, switching order of error code and message in the `try_rpc` function to aid error debugging.
Should help in debugging #34354 IMO. It's an intermittent failure on Windows that I can't reproduce and it's more difficult to figure out what could have gone wrong only by seeing the error code like below in the CI logs. Given that the functional tests pass, I don't see a harm in checking for error message first and throwing it in case of a mismatch.
```python
AssertionError: Unexpected JSONRPC error code -1
```
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
maflcko:
lgtm ACK 0aba464ce76522f1be3bb9e471b45438738de492
polespinasa:
lgtm ACK 0aba464ce76522f1be3bb9e471b45438738de492
fjahr:
utACK 0aba464ce76522f1be3bb9e471b45438738de492
brunoerg:
code review ACK 0aba464ce76522f1be3bb9e471b45438738de492
sedited:
ACK 0aba464ce76522f1be3bb9e471b45438738de492
Tree-SHA512: b09ba4b5d13a2c93a4a28a5c1b06af44a91295974236bb8326b74a988878c431e9ce0e19ec14bb98ac2b002da877abaa7da6a9851424453bcb494c0317b57227
75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb wallettool: Disallow creating new unnamed wallets (Ava Chow)
5875a9c502632eb5c74df07e41af38582da6e884 wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow)
d30ad4a9129da04e249e3938f643dc47bf060e7e wallet, rpc: Use HandleWalletError in createwallet (Ava Chow)
Pull request description:
We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets. `createwallet`, `restorewallet`, and the wallet tool's `create` and `createfromdump` all now require the user to provide a non-empty wallet name when creating/restoring a wallet.
The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying `CreateWallet` and `RestoreWallet` functions.
Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to `RestoreWallet` to explicitly allow that behavior for migration only.
ACKs for top commit:
rkrux:
lgtm ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
polespinasa:
re ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
Tree-SHA512: 8bde76d0b091e9276788c69412934af3426da2a7a69a00f94072d36c1a075cd41744ecdd5fef2b72870c1351b76aae061f124f716bb23f4839be20c464fc5ebd
fab055c907f1ab9ecac49e3d72909289a3b08c2d test: Scale NetworkThread close timeout with timeout_factor (MarcoFalke)
Pull request description:
Not sure if this fixes https://github.com/bitcoin/bitcoin/issues/34248, but scaling here probably makes sense, considering some CI setups run in nested VMs with a different arch system-qemu.
ACKs for top commit:
hebasto:
ACK fab055c907f1ab9ecac49e3d72909289a3b08c2d, the diff looks reasonable.
Tree-SHA512: 98f9b0bdc3b02b692a14129f88c05f2df0d1e11e4167ff5d0cc6a3a6efd8994a743e969e83c71cb534537f134e07ba9a5cba3eb2010a6b6cf69bec959faf2c43
faa18dceba1dd69703a252f751c233d227164689 refactor: Use std::bind_front over std::bind (MarcoFalke)
Pull request description:
`std::bind` has many issues:
* It is verbosely listing all placeholders, but in a meaningless way, because it doesn't name the args or their types.
* It silently ignores args passed to it, when one arg is overridden. For example [1] compiles fine on current master.
* Accidentally duplicated placeholders compile fine as well.
* Usually the placeholders aren't even needed.
* This makes it hard to review, understand, and maintain.
Fix all issues by using `std::bind_front` from C++20, which allows to drop the brittle `_1, _2, ...` placeholders. The replacement should be correct, if the trailing placeholders are ordered.
Introducing the same silent bug on top of this pull request [2] will now lead to a compile failure.
----
[1]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 694fb535b5..7661dd361e 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, CTxDestination{}, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2));
```
[2]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 578713c0ab..84cced741c 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind_front(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this, CTxDestination{}));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind_front(NotifyTransactionChanged, this));
ACKs for top commit:
janb84:
cr ACK faa18dceba1dd69703a252f751c233d227164689
fjahr:
Code review ACK faa18dceba1dd69703a252f751c233d227164689
hebasto:
ACK faa18dceba1dd69703a252f751c233d227164689, I have reviewed the code and it looks OK.
Tree-SHA512: 9dd13f49527e143a2beafbaae80b1358981f07a2ce20d25cffb1853089a32ff71639e6d718d1d193754522f9ac04e3e168ba017d5fc67a11a5918e79a92b3461
Add C API functions for managing BlockValidationState lifecycle:
- btck_block_validation_state_create()
- btck_block_validation_state_copy()
- btck_block_validation_state_destroy()
Introduce BlockValidationStateApi<> template to share common getter methods between BlockValidationState (Handle) and BlockValidationStateView (View) classes in the C++ wrapper. This enables external code to create and own BlockValidationState objects needed for the new process_block_header() API.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
ryanofsky:
Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
I feel it'd be easier to debug intermittent test failures if the
error message is present in the logs instead of error code. So,
switching order of error code and message in the `try_rpc` function
to aid error debugging.
