✅ hebasto closed an issue: "Guix builds are affected by external environment"
(https://github.com/bitcoin/bitcoin/issues/29754)

💬 hebasto commented on issue "Guix builds are affected by external environment":
(https://github.com/bitcoin/bitcoin/issues/29754#issuecomment-2186764833)
> One might expect that not passing the `-Wl,-platform_version,macos` flag to a linker will cause the `check_MACHO_sdk` in the `symbol-check.py` to fail.
>
> That's true on Ubuntu 22.04 LTS:

It's weird, but I can't longer reproduce it with the same commit.

Closing.

👍 stickies-v approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2136180272)
ACK a2fc9ddee9cbaeffb3c460973bab3c2dd734db55

Nice improvement, a few suggestion nits but not blocking - happy to quickly re-review if you adopt them.

Tested that all the new examples work too.

💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1651259047)
nit: I think using `startswith` is a bit more intuitive here.

The entire logic can be simplified too, I think:

<details>
<summary>git diff on a2fc9ddee9</summary>

```diff
diff --git a/contrib/verify-binaries/verify.py b/contrib/verify-binaries/verify.py
index b7f64d4336..d09ae0dbfe 100755
--- a/contrib/verify-binaries/verify.py
+++ b/contrib/verify-binaries/verify.py
@@ -100,13 +100,10 @@ VERSION_FORMAT = "<major>.<minor>[.<patch>][-rc[0-9]][-platform]"
VERSION_EXAMPLE = "22.0 o
...

💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1651285054)
f-string nit
```suggestion
log.error(f"No files matched the platform specified. Did you mean: {closest_match}")
```

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1651314694)
sure, though i don't intend to add mocking for netlink sockets (that's just too OS specific), but for the UDP socket creation it'd be useful

📝 achow101 opened a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328)
The legacy wallet `IsMine` code will be removed with the legacy wallet, but is still partially needed for migration. Instead of using `IsMine` directly in migration, equivalent checks are performed by migration.

Builds on #26596

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2186998015)
> I believe this PR could get merged quite fast if you leave [8950371](https://github.com/bitcoin/bitcoin/commit/89503710386f52d9162f67fcd707eceffa954faa) for a follow-up and make `IsMine` compatible with `LegacyDataSPKM` in this PR.

Moved to #30328

> The `IsMine` removal is also nice but I wouldn't miss a deadline for it.

I think it needs to be in the next release as it is a critical part of migration. I would prefer to have it in a release that people can use before deleting the legac
...

💬 sipa commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2187013974)
Concept ACK

💬 sipa commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2187033512)
If you can benchmark pre-assumevalid IBD I think you'll see `FetchCoins` have a bigger impact than in `AssembleBlock`. It may need an actual `-reindex` though, I don't know if we have a benchmark with realistic workload.

💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1651366991)
Oh yeah, makes sense. TY.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1651381337)
Unfortunately, that gives a new warning, because the `int` isn't the exact type:
```
../../src/util/pcp.cpp:258:41: warning: narrowing conversion of ‘recvsz’ from ‘int’ to ‘std::size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
258 | if (check_packet({response, recvsz})) {
```
Could add a `static_cast` but i don't think it's much of an improvement in that case?

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1651401511)
It's an OS API, it's very unlikely to fail, so i don't want to add a specific log message for it. i'm fine with it just returning nullptr.

💬 mzumsande commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1651401792)
will change to that when taking it out of draft.

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1651420690)
In that case you could use `check_packet(Span(response, recvsz))` (the `<uint8_t>` can be automatically deducted).

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2187135835)
Force push: rebased to get `CreateSock()` from #30202.

🤔 Mtiisf125 reviewed a pull request: "doc: clarify Cirrus self-hosted workers setup"
(https://github.com/bitcoin/bitcoin/pull/30314#pullrequestreview-2136604935)
Mri>..ta**>_isf

📝 mzumsande opened a pull request: "fuzz: improve utxo_snapshot target"
(https://github.com/bitcoin/bitcoin/pull/30329)
Add the possibility of giving more guidance to the creation of the metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes successfully creates a valid snapshot.

This also changes the asserts for the success case that were outdated (after #29370) and only didn't result in a crash because the fuzzer wasn't able to reach this code before.

💬 achow101 commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1651551124)
Although `GetOSRand` is declared in `random.h`, it isn't actually used by anything outside of `random.cpp`, so it could remain inside the anonymous `namespace`.

💬 achow101 commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1651558600)
In cf04e6c90e1902be28a85ef181b8f1ee4b89e11d "random: make GetRand() support entire range (incl. max)"

Since this function is only used in a few places, would it make sense to just inline it where it is used?