💬 fanquake commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2186716560)
I think that rather than trying to micro-optimise these somewhat sensitive functions, if you're interested in benchmarking / performance related changes, your time might be better spent reviewing Pull Requests like #28280 (which your changes conflict with).

I'd also note that all of your benchmarking / profiling seems to be being done on an arm64 macOS machine, using (I assume) Apple Clang as the compiler, and libc++ as the standard library. So when you create changes like these, even if you
...

🤔 hodlinator requested changes to a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2135964825)
Concept ACK a0dffe6318d32b7b49b7d0be6d45b59ee3e0f4ec

`git range-diff ee15876~1..ee15876 a0dffe6~1..a0dffe6`
Agree that "regular" wording wasn't needed.

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1651128863)
Would have written it closer to something like:
```suggestion
fs::path absolutePath;
#ifdef WIN32
if (const char* homedrive = std::getenv("HOMEDRIVE"), * homepath = std::getenv("HOMEPATH");
homedrive != nullptr && homedrive[0] != 0 && homepath != nullptr && homepath[0] != 0)
absolutePath = fs::path(homedrive) / fs::path(homepath);
else
absolutePath = fs::path("C:\\Users\\Satoshi");
#else
if (const char* home = std::getenv("HOME"); home != nullptr
...

💬 fanquake commented on issue "Check usages of `#if defined(...)`":
(https://github.com/bitcoin/bitcoin/issues/16419#issuecomment-2186722083)
Going to close this now that #29876 is merged.

🤔 furszy reviewed a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2135821455)
Code review ACK 8789dc8f315a9d9ad7142d831bc9412f780248e7.
Left few nits.

💬 furszy commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651079954)
little topic:
maybe changing the RPC docs to say that "pruneheight" returns "the first block unpruned, all previous blocks were pruned" is more useful than saying "height of the last block pruned, plus one" as is now.

💬 furszy commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1651156361)
```suggestion
const CBlockIndex* first_unpruned{Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))};
if (first_unpruned == first_block) {
```

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