💬 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.

🤔 rkrux reviewed a pull request: "test: address self-announcement"
(https://github.com/bitcoin/bitcoin/pull/34039#pullrequestreview-3567579542)
Code review fb9c393564ef098267ad2a921b75e0716f38b047

💬 rkrux commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2610698038)
In fb9c393564ef098267ad2a921b75e0716f38b047 "test: address self-announcement"

Is it intentional to mention outbound twice in this log message and deviate from the similar log message of the inbound test?

💬 rkrux commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2610717344)
In fb9c393564ef098267ad2a921b75e0716f38b047 "test: address self-announcement"

I noticed some duplication in the test that could be removed with the following diff. I don't feel too strongly about this suggestion and if you think such a de-duplicated code might make it more difficult to update the test in the future, then feel free to not implement.

```diff
diff --git a/test/functional/p2p_addr_selfannouncement.py b/test/functional/p2p_addr_selfannouncement.py
index 65527d1e77..aba83c73c3
...

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

i'd say it says that `error()` in this context is `(*this).error()`. So it calls `error()&` and not `error()&&`. If you want to (incorrectly) recursively call error(), you'd have to add a cast: `std::move(*this).error();`

💬 fjahr commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#issuecomment-3642318494)
utACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c