💬 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

💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2611095788)
I agree that the fundamental behavior didn't change, both read and write threw instead of returning. Can we adjust the comments at least to reflect this?

💬 maflcko commented on pull request "cmake: Add fail pattern to `try_append_cxx_flags`":
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2611119220)
> This output shows that the `-Wduplicated-branches` flag is incorrectly treated as recognised by the compiler.

Yes, but this is what the user asked for by setting `-Wno-error=unknown-warning-option`?

I don't think we should add code to silently undo switches that the user turned on.

Maybe I am missing something obvious, but I don't see the risk or the downside of just leaving this as-is.


> Noticed while working on the https://github.com/google/oss-fuzz/pull/14457 to Ubuntu 24.04
...

✅ plebhash closed an issue: "consider adding a new `interface RawTxFeed` on Mining IPC"
(https://github.com/bitcoin/bitcoin/issues/34030)

💬 plebhash commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3642581846)
ok, I guess we can stick with `getTransactionsByWitnessId` (to be potentially introduced via https://github.com/bitcoin/bitcoin/pull/34020) for https://github.com/stratum-mining/sv2-apps/issues/24

if these IPC round-trips ever prove to be an undesirable bottleneck, we can revisit the ideas proposed here.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2611161028)
Updated the commit message in f195f46a996b9803d26865cdcef185806c22a723.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2611169064)
Fixed using `Path.glob()` in https://github.com/romanz/bitcoin/blob/6e71622d8526ec682375479ecabc6426409410f2/test/functional/interface_rest.py#L499-L505