💬 l0rinc commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#discussion_r2610463571)
Yes, it's an unused argument - it's clearer to remove the fake single-instance-abstraction

💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2610467129)
To use span-serialization, you can just type `std::span{bla}`, see also `git grep '<< std::span{'`.

I don't think it makes sense to use write_buffer for places where span-serialization is wanted. Otherwise, this leads to inconsistent code, such as:

```cpp
file << int_val;
file.write_buffer(vec_val);
file << int_other_val;
```

Also, this mutating the buffer (possibly) makes it doubly confusing.

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2610471488)
Yes, I copied that from the source to simplify greeping for code (since I couldn't adjust the source instead). If you insist, I can decouple the two if I need to touch again

💬 l0rinc commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2610503721)
> this mutating the buffer

There was no obfuscation enabled, so no mutation was happening.

> don't think it makes sense to use write_buffer for places where span-serialization is wanted

I dislike magical operators and needless serialization when we want to directly write content, but I'm also fine with `file << std::span{data}` - pushed.

💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#issuecomment-3641837244)
review ACK 93941dc74be8817463e024e5aa11ed0638fdc9f3 🐪

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 93941dc74be8
...

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#issuecomment-3641843791)
Clean rebase, reviews are welcome again.

💬 hebasto commented on pull request "cmake: Add fail pattern to `try_append_cxx_flags`":
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2610521348)
The PR description has been updated with the steps to reproduce the issue.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610543103)
This might do it - an earlier version of it passed Windows CI (https://github.com/hodlinator/bitcoin/actions/runs/20131062861):
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -7,9 +7,11 @@
from decimal import Decimal
from enum import Enum
from io import BytesIO
+from pathlib import Path
import http.client
import json
-import platform
+import os
+import re
import typing
import urllib.parse

@@ -496,15 +498,14 @@ class RESTTest (
...

💬 Raimo33 commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#issuecomment-3641908735)
re ACK 93941dc74be8817463e024e5aa11ed0638fdc9f3

💬 cedwies commented on pull request "log: check fclose() results and report safely in logging.cpp":
(https://github.com/bitcoin/bitcoin/pull/33646#discussion_r2610598466)
I put the `fclose` outside the lock to avoid holding the mutex during potentially slow I/O (like flushing the file). We swap `m_fileout` inside, so the state is safe, and then close the old file without blocking other threads. `ReportLogIOError` is designed to be lock-free anyway, if I am not missing something.

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610602576)
It's about failing with a clearer stack trace/message. An exception will unwind the stack before giving a stack-trace (depending upon how the compiler treats exceptions within `noexcept`?).

Tried adding `*e` in the unit tests and it doesn't pinpoint the offending line, but gets by on checkpoints.

```
₿ ./build/bin/test_bitcoin -t util_expected_tests
Running 9 test cases...
terminate called after throwing an instance of 'util::BadExpectedAccess'
what(): Bad util::Expected access
unk
...

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610606350)
Can't read that. :grin:

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610629951)
Yes, this is also the behavior that you get in C++23 (if you are lucky). If you are unlucky, you'll just the silent UB:

https://godbolt.org/z/jYxqvoejn

```
/../include/c++/v1/__expected/expected.h:811: libc++ Hardening assertion this->__has_val() failed: expected::operator* requires the expected to contain a value
Program terminated with signal: SIGSEGV

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610641558)
Yeah, we don't need to spoil ourselves before the switcharoo to `std::expected`. :)

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2610659650)
Rebased, fixed this, thanks!

💬 sipa commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2610667004)
Fixed.

💬 sipa commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2610672616)
Fixed.

💬 sipa commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#issuecomment-3642035985)
@Sjors Done.

👍 hodlinator approved a pull request: "fuzz: exercise `ComputeMerkleRoot` without `mutated` parameter"
(https://github.com/bitcoin/bitcoin/pull/34050#pullrequestreview-3567578900)
ACK 7e9de20c0c144f5ccea5efe6d90601dd72bc7461

💬 ajtowns commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2610717184)
Hmm; personally, I think if we can get compile time errors without too much hassle, that's much better than leaving it for CI to uncover.

Bumped the PR with a way of capturing succintly, but being very explicit about it when you're doing it.

👍 hodlinator approved a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3567608553)
re-ACK 9f39a8309cb0a23df03937b0b225aff8ce9e1ec6

Resolved conflict with #33805 and fixed code nit https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2602163619.