💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2642142041)
lgtm ACK 517d30b097d17f86d40beff679f62287776c459a

I haven't tested this (I am using a modified script locally anyway), but the code changes look harmless and reasonable.

💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946078456)
Thanks for the reviews. The build path can now specified via the `BUILDDIR` environment variable (set to "build" relative to the top-level-dir by default) and the script can be called from anywhere within the repository.

@maflcko
> This would also allow to remove mktemp and just use the build dir.

Done.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1946082845)
Right, but shouldn't `getaddressinfo` hide the field entirely then? (obviously not in this PR)

💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1946126512)
Done. Also renamed the variable to `_crc32_src`.

💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1946127776)
I just reused the existing style and just renamed the target and the keyword. Reformatting that line has more ripple effects in the lines that follow.

💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2642221055)
> I think that APPENDing to the COMPILE_OPTIONS property should be preferred over overriding it.

The command line flags that are passed to the compiler are constructed by several variables and properties that are global, target specific, and file specific. Since this is the only file specific option, APPENDing has no different effect as setting.

💬 Sjors commented on issue "Support validating a PoW-free block over over RPC":
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2642228270)
#31564 adds this to the Mining interface. It's not needed for the Stratum v2 Template Provider, so lower priority for me.

Additionally it introduces a `checkblock` RPC, which seems easier to use and easier to expand later.

Finally it can verify weak block proof-of-work.

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