💬 fjahr commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#discussion_r1651063291)
Maybe add the count of addresses that were unassigned here specifically in the summary. Those may be among the most interesting to look at so I think it could be highlighted.

💬 jlopp commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2186630679)
Concept ACK.

For reference, I ran several sync tests with different dbcache sizes to see how much of an effect they have on performance. A good 25% improvement vs default when my machine abstained from flushing to disk. https://blog.lopp.net/effects-dbcache-size-bitcoin-node-sync-speed/

💬 fanquake commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2186671730)
> These changes result in a modest performance improvement:

I ran the benchmark on an aarch64 machine, and saw mostly worse performance when using the latest GCC, or Clang, with libstdc++. There was a small improvement when using Clang and libc++.

GCC 14.1.1 20240620 (Red Hat 14.1.1-6)

Master
```bash
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|-------------
...

✅ fanquake closed an issue: "Check usages of `#if defined(...)`"
(https://github.com/bitcoin/bitcoin/issues/16419)

🚀 fanquake merged a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876)

💬 fanquake commented on pull request "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core"":
(https://github.com/bitcoin/bitcoin/pull/30308#issuecomment-2186691312)
> Changing it would break that test.

Not in our repository though? In any case, this could be revisited if someone follows up with a CI change.

💬 paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2186691697)
Thanks for checking @fanquake!
As the profiler showed, this is only 5% of `AssembleBlock`, and the benchmark varied a lot on my machine as well, it might be why you also see the discrepancy.
Do you think I should add a dedicated benchmark for this only?

🚀 fanquake merged a pull request: "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core""
(https://github.com/bitcoin/bitcoin/pull/30308)

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