💬 furszy commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651043128)
> I don't know how to explain it in a way that works for everyone and is still concise

No problem on leaving it as is if there is other RPC command using this term.
What I wrote seems generally useful to me; it means that the node might not be able to access the spent outputs script and value. Maybe adding few scenarios where the undo data is needed could clarify it usage.

💬 furszy commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651160369)
could be simpler:
```suggestion
if ((chain_tip->nStatus & BLOCK_HAVE_MASK) != BLOCK_HAVE_MASK) return chain_tip->nHeight;
```

👍 stickies-v approved a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2135982014)
ACK 8789dc8f315a9d9ad7142d831bc9412f780248e7

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651138486)
nit: missing some includes / fwd decls for the newly added line:

<details>
<summary>git diff on 8789dc8f31</summary>

```diff
diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h
index f6a7fe236c..23909c334e 100644
--- a/src/rpc/blockchain.h
+++ b/src/rpc/blockchain.h
@@ -9,15 +9,18 @@
#include <core_io.h>
#include <streams.h>
#include <sync.h>
+#include <threadsafety.h>
#include <util/fs.h>
#include <validation.h>

#include <any>
+#include <optional>
#include <s
...

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651153859)
nit
```suggestion
static void CheckGetPruneHeight(const node::BlockManager& blockman, const CChain& chain, int height) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
```

and related:

<details>
<summary>git diff on 8789dc8f31</summary>

```diff
diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp
index 2a63d09795..7a5d175c8c 100644
--- a/src/test/blockchain_tests.cpp
+++ b/src/test/blockchain_tests.cpp
@@ -96,8 +96,8 @@ static void CheckGetPruneHeight(node::BlockManage
...

✅ 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).