💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917234974)
Could you add an LDFLAGS param to the function as `LINK_OPTIONS` and use that here instead?

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917262536)
`LDFLAGS` might make more sense because, for example, in the OSS-Fuzz environment, the `SANITIZER_LDFLAGS` variable can be set to `-fsanitize=fuzzer` or `/usr/lib/libFuzzingEngine.a`.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker build`.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917272201)
> Does this env var work for podman as well?

The "podman" cli frontend doesn't use this environment variable[^1], but I tested locally (on Fedora 40) that this PR works for the build phase when using docker cli to interact with podman container host:

```console
$ sudo dnf install podman-machine
$ podman machine init
$ podman machine start
Starting machine "podman-machine-default"
API forwarding listening on: /run/user/1000/podman/podman-machine-default-api.sock
$ env -i HOME="$HOME"
...

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2593855664)
Force-pushed simplifying the approach. Now it just checks the whitespace in the `ParsePubkeyInner` function.

💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2593858698)
Updates:
- Rebased (fixed previous CI failure - _ASan + LSan + UBSan..._ ).

💬 ryanofsky commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2593868257)
Rebased ebb77ab77df4081f27ce55e8b28b0d35990caac8 -> a46ec1dece8d8a1b16ff1fc3d2932bca130bf67f ([`pr/dtran.2`](https://github.com/ryanofsky/bitcoin/commits/pr/dtran.2) -> [`pr/dtran.3`](https://github.com/ryanofsky/bitcoin/commits/pr/dtran.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/dtran.2-rebase..pr/dtran.3)) due to conflict with #31061

💬 pablomartin4btc commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2593886226)
Updates:
- Rebased (fixed previous CI failure - _previous releases, depends DEBUG..._ ).

👍 ryanofsky approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2553888604)
Code review ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc. Changes since last review are fixing size options to use size_t instead of int64_t again, simplifying CheckedLeftShift more, and making other minor suggested cleanups

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593891189)
@theuni

Thank you for your review! Your comments have been addressed.

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917307456)
Thanks! [Done](https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593891189).

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917309058)
The `LDFLAGS` option has been [added](https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593891189).

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2593925274)
Thanks for the suggestions @laanwj,

Rebased f157b0cbc7d90075858a6522d13a7bc4f0b25a5f -> f25616bec485ee6a70e4b797758d4987d25a7c25 ([kernelApi_14](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_14) -> [kernelApi_15](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_15), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_14..kernelApi_15))

* Fixed conflict with #31061

Updated f25616bec485ee6a70e4b797758d4987d25a7c25 -> 4dde75858a3b08f84d71176c7be14bae62020b1f
...

🤔 jonatack reviewed a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2554013096)
re-ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3

🤔 darosior reviewed a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2553493851)
Some more nits i noticed when reusing some of this code for the fuzz target. I don't think they warrant retouching, but figured i'd post them anyways.

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917096066)
nit in the context of this test but i think you meant:
```suggestion
if (service.SetSockAddr(sa, sa_len)) {
// Can only bind to one of our local ips
if (!service.IsBindAny() && service != m_local_ip) {
return -1;
}
m_bound = service;
return 0;
}
return -1;
```

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917096949)
nit
```suggestion
assert(false && "Move of Sock into PCPTestSock not allowed.");
```

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917105856)
nit: it's never used, any reason for defining it?

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917273199)
Isn't this redundant with `~Sock`?

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917172067)
nit
```suggestion
0x02, 0x81, 0x00, 0x42, // version, opcode, result error
```

👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2553957182)
Code review ACK 712e3ffde5fa4fd26c0f217942b4a289cf37f361. Main change since last review is mining_testnet.py test being replaced by mining_mainnet.py test with a much smaller data file. It was also rebased and there were small documentation changes and string format tweak in mining_mainnet.py