💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953144139)
I wonder if this can be made dumb stupid and reduce it to just call `execvp()`. That is, reduce `ExecCommand()` to just 1 - 5 lines of code and deal with the consequences of it...

🚀 fanquake merged a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840)

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654464619)
Rebuilt with @pinheadmz's committed sigs

```
$ find guix-build-096525e92cc2/output -wholename "*codesigned*" -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
111016205f0a2ac732feb934acb3e8a36d5251f119d8fa9215790310ba46c31d guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz
a1228d
...

👍 theuni approved a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379#pullrequestreview-2612828482)
utACK c4c5cf174883cb53256e869f0d1673e29576a97c.

It makes sense to forward `APPEND_LDFLAGS` and `APPEND_CFLAGS` to their secp equivalents for convenience and consistency for tests.

💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1953186524)
@fanquake Are you ok if we break OSS-Fuzz here with a proper fix (to avoid future technical debt), and PR the one-liner to fix it there post-merge?

💬 andrewtoth commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953187368)
I think the scope of this PR is to reduce chainstate writes, which occur when we need to prune blocks. Pruning blocks means we can't reindex them and would have to redownload, so we write the chainstate then to be safe.

If we were to not store blocks, we would have to write the chainstate after indexing every block to avoid potentially redownloading them.

👍 theuni approved a pull request: "cmake: Add `CheckLinkerSupportsPIE` module"
(https://github.com/bitcoin/bitcoin/pull/31359#pullrequestreview-2612881018)
utACK 81c174e3186efae084829dcde314b081cad3d3cb.

Nice work upstreaming the correct fix. Makes sense for us to carry the workaround for a few years. No opinion on the second commit.

💬 theuni commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2654583997)
I don't understand this change. It seems setting CC_FOR_BUILD to CC is exactly the opposite of what we want to do?

@hebasto's original change makes more sense to me:
```
BUILD = $(shell env --unset CC ./config.guess)
```

That ignores what's set in CC when detecting the native platform, which seems like the correct behavior to me.

What was your intention with this patch, @fanquake ?

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953239521)
You are eaten by a grue.

🤔 theuni reviewed a pull request: "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets"
(https://github.com/bitcoin/bitcoin/pull/30901#pullrequestreview-2612945548)
Concept ACK.

💬 theuni commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1953250620)
These are now missing `DEPENDS_EXPLICIT_OPT` from #30911.

💬 theuni commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1953249473)
I think this would make sense as 2 separate functions. Seems a weird mix of args otherwise.
`target_raw_data_source` and `target_json_data_source`?

I think it'd be fine to call both funcs for targets that need both types of files generated.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953254091)
:man_shrugging: I hope the code is clear enough that reviewers are convinced this is impossible. The `Assume` hopefully helps with that convincing. I hope an assertion doesn't add to that.

💬 theuni commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2654622689)
Also, since you're messing with this, please consider the `CODEGEN` option for `add_custom_command`, which would need feature-gating same as `DEPENDS_EXPLICIT_OPT`.

👍 laanwj approved a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2612972081)
re-ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2654640516)
@maflcko Mind taking a look at the first commit here to see if you'd prefer a different approach?

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2612848923)
Code review ACK ce9c2e17d5d4ef99cba82521f2515e34130a02a1. I don't think this needs to depend on the other PR #31785, because I don't see how the assume check https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937596836 could fail. Left some minor comments below though, including one about how assume check could be handled better.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953193832)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

This may all be true, but I don't think it clearly explains the behavior when MAX_MONEY is specified. Would suggest a simpler comment:

* The wait method will not return a new template unless it has fees at least `fee_threshold` sats higher than the current template, or unless the chain tip changes and the previous template is no longer valid. Determining whether `fee_threshold` is reached is ex
...

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953241280)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

Any thoughts on whether it makes sense for this polling interval to be configurable? I could imagine a miner not really caring how expensive this check is and just wanting it to be as fast as possible?

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953250463)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

The "// This just adds coverage" comment should be moved above the waitTipChanged call since it does not apply to the waitNext call, which is needed to set `block_template` for the next loop iteration.