📝 hebasto converted_to_draft a pull request: "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows"
(https://github.com/bitcoin/bitcoin/pull/31158)
On the master branch @ 9a7206a34e3179d7280de98a818a13374178ee7b, linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` fails:
```
> cmake -B build --preset vs2022-static -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
> cmake --build build --config Release -t bitcoin-chainstate
...
LINK : fatal error LNK1181: cannot open input file 'kernel\Release\bitcoinkernel.lib' [C:\Users\hebasto\source\repos\bitcoin\build-static\src\bitcoin-chainstate.vcxproj]
```

This PR
...

🤔 fjahr reviewed a pull request: "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds"
(https://github.com/bitcoin/bitcoin/pull/31588#pullrequestreview-2600123724)
Concept ACK

💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1945553930)
In `gen-manpages.py` and `gen-bitcoin-conf.sh` we use `BUILDDIR` fwiw.

💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1945549602)
I agree, hardcoding a different path isn't much of a fix even if it's the one we use in all our examples. Better to have a way to pass the path in as a parameter.

💬 Willrok91 commented on issue "Generalized fee bumping":
(https://github.com/bitcoin-core/gui/issues/822#issuecomment-2641233162)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7

💬 Willrok91 commented on issue "Send: ability to (re)view automatically selected coins ":
(https://github.com/bitcoin-core/gui/issues/778#issuecomment-2641236859)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2641303013)
> It might be good to also adjust the title of this test for CoinGrinder to "3) Test selection when some selections with sufficient funds would exceed the maximum allowed weight"

I find the use of "selection" twice in the sentence doesn't add much clarity. Now with the added assertions, I feel like the tittle fits the test however I'm open to a better title.

> Not really. The weight-optimality of CoinGrinder solutions is shown per the fuzz target coin_grinder_is_optimal.

Interesting.
...

💬 jarolrod commented on pull request "Prepare "Open Transifex translations for v29.0" release step":
(https://github.com/bitcoin/bitcoin/pull/31809#issuecomment-2641784769)
post merge ack

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

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945976586)
No, we do not want to remove entries from `m_listen_permissions` or from `m_listen` when disconnecting a single peer. The entries in those two maps are per listening address. E.g. if we listen on `1.1.1.1:8333` and designate that every peer that connects to that address gets `permissions1` and listen on `2.2.2.2:8333` and peers that connect to that address get `permissions2`. This stays unchanged as peers connect and disconnect. We would want to remove entries from those maps if we stop listenin
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945983707)
`addr` is our listening address. We can't listen on the same address two times:

If I put this in my `bitcoind.conf`:
```
bind=127.0.0.1:20001
bind=127.0.0.1:20002
bind=127.0.0.1:20003
bind=127.0.0.1:20003
```

I get:

```
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20001
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20002
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20003
2025-02-07T05:39:56Z [net:error] Cannot
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945989789)
The proposed snippet is more elegant and performant, I like it. It relies, however, on `None` being `0` and will break in a subtle way if that is changed. I will leave it as it is. This is executed once per newly accepted connection which is not super-often and the performance gain would be negligible.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945991471)
Agree, will leave it as it is.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946000478)
This method was only dealing with listening sockets, so the name `CloseSockets()` with a comment "Close all sockets" was confusing. Renamed to `StopListening()`.

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