💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1946163054)
Actually it's not needed. This code is for the proposal mode, where we verify a block given to us. It's not _our_ node error if this block is invalid.

When we generate a block template ourselves through `getblocktemplate` or `createNewBlock()`, it goes through `BlockAssembler::CreateNewBlock()` which by default runs `TestBlockValidity` at the end, and throws if this return an error. No need to log if we're already crashing with `std::runtime_error`.

💬 l0rinc commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1946169077)
This PR was meant to fix the example only, I don't have enough context to update the docs as well.

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1946211743)
done

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1946213790)
my bad, my clangd add it automatically and I don't notice.
I drop it as it seems redundant

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946227366)
Changed to `LogError()` and removed the newline. I did not know the trailing newlines are now unnecessary (since bbbb2e43ee95c9a8866aa1f65e3f001f752dfed2).

💬 dergoegge commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642410797)
ACK faca7ac13215fd88b420feea8f06d7404f8fd067

💬 dergoegge commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2642418075)
> corecheck is back up and integrated into the DrahtBot comment, so is anything left to be done here?

I think the only thing missing is an alert system or even just a better overview over all benchmarks? Seems difficult to spot regressions in here manually: https://corecheck.dev/benchmarks

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946240012)
Done.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946244171)
Done.

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#issuecomment-2642428740)
tahnks for your time @hodlinator!

🚀 fanquake merged a pull request: "ci: Bump fuzz task timeout"
(https://github.com/bitcoin/bitcoin/pull/31814)

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946265398)
:facepalm: makes total sense, I was thinking about the remote sockets.

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946267787)
Yeah, sorry, me confusing sockets again. An `Assume()` would be nice though.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946271517)
Done.

💬 maflcko commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2642488461)
Good point. I guess there could be a comment (or the summary comment could be edited) to encourage people to check the result link of a pull request if the difference is larger than some percent.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946284407)
Removed `struct` from `BindAndStartListening()` in commit `style: modernize the style of SockMan::BindListenPort()`.

`GetBindAddress()` was just moved verbatim without any mods from `net.cpp` to `sockman.cpp` in another commit. This makes it easier to review with
```
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```
in `~/.gitconfig`. Will leave it as it is. Don't want to bloat this PR further with one more "style changing" commit.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946299108)
In general, less indentation makes the code more readable, so I prefer to have `return;` and then the subsequent code to be with less indentation.

Anyway this code was changed and now uses `switch` (see `CConnman::EventI2PStatus()` after I push).

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946303871)
```
error: delete called on non-final 'CConnman' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-abstract-non-virtual-dtor]
```

💬 fanquake commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2642520881)
> Submitted patch in https://github.com/capnproto/capnproto/pull/2235

Thanks. Pulled that into #29796.

💬 maflcko commented on pull request "build: fix MSVC ccache reporting":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2642527478)
Looks like there are several general problems:

* The summary is just AI generated to repeat the code changes (which are also AI generated?). This is problematic, because if someone wanted to look at AI summaries or generations, they could just ask an LLM themselves.
* The summary repeats the code changes. This is problematic, because the code changes should be explained: Why they are the correct approach (in light of alternatives) and why they fix the problem.
* All tests are failing. This
...