💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2641120104)
> Did anyone test this on Windows?

Of course would be nice to test this on windows but it should work there according to cmake release notes: https://cmake.org/cmake/help/v3.13/release/3.13.html?highlight=release%20notes#command-line

💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2641150108)
Converting to a draft after today's offline discussion with @theuni.

📝 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`.