🤔 furszy reviewed a pull request: "test: add coverage for bech32m in `wallet_keypool_topup`"
(https://github.com/bitcoin/bitcoin/pull/28928#pullrequestreview-1745180193)
we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.

✅ fanquake closed a pull request: "@kaaid"
(https://github.com/bitcoin/bitcoin/pull/28496)

📝 fanquake locked a pull request: "@kaaid"
(https://github.com/bitcoin/bitcoin/pull/28496)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

👍 TheCharlatan approved a pull request: "Improve peformance of CTransaction::HasWitness (28107 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/28766#pullrequestreview-1745321741)
ACK af1d2ff88344e1545ac8d9ad09f8e37e264da712

Nit: Maybe add a `HasWitness` check somewhere in the `transaction_tests`?

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402468881)
nit
```suggestion
g_available_coins.emplace_back(Txid{}, i);
```

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402689558)
Doesn't it make more sense to keep this as `std::vector<uint256> chain_txids;` for now?

👍 stickies-v approved a pull request: "Use Txid in COutpoint"
(https://github.com/bitcoin/bitcoin/pull/28922#pullrequestreview-1745013646)
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402468172)
nit
```suggestion
const auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
```

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402690528)
nit: Would simplify this to

```cpp
for (const auto& [txid, wtx] : wallet.mapWallet)
```

<details>
<summary>git diff on 9e58c5bcd9</summary>

```diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 35583642a5..0e72efe0ba 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -319,11 +319,8 @@ CoinsResult AvailableCoins(const CWallet& wallet,
std::vector<COutPoint> outpoints;

std::set<uint256> trusted_parents;
- for (const auto& entry : wa
...

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402684591)
nit
```suggestion
txids.emplace_back(Txid::FromUint256(ConsumeUInt256(fuzzed_data_provider)));
```

💬 mzumsande commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1823476778)
Since it comes with a memory cost, I wonder if the the performance gain we get for it can be measured. Does this speed up any benchmarks / IBD?

💬 willcl-ark commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1402731108)
Thanks for the idea Vasil.

I implemented it in this [tag](https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:netmsgstats-vd-array) to see what it would look like.

Overall, I prefer the design, but in writing the commit message for the change I began to feel unsure that it _actually_ results in `g_all_net_message_types` being a compile-time constant. IIUC the array will reference a `const char*`. The pointer addresses can only be allocated at runtime and in turn `g_all_
...

💬 sipa commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1402733334)
Make it `constexpr` instead of `const`. That'll force the compiler to evaluate the array at compile-time (or give an error if that's not possible).

💬 willcl-ark commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1402735834)
Thanks, I forgot to mention I had tried that. It results in something like this:

```log
$ bear -- make
Making all in src
make[1]: Entering directory '/home/will/src/bitcoin/src'
make[2]: Entering directory '/home/will/src/bitcoin/src'
make[3]: Entering directory '/home/will/src/bitcoin'
make[3]: Leaving directory '/home/will/src/bitcoin'
CXX bitcoind-bitcoind.o
In file included from ./netstats.h:10,
from ./net.h:22,
from ./interfaces/node.h:1
...

💬 sipa commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1402736925)
Oh, yes, you cannot have `constexpr` std::string until C++20.

💬 theuni commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1823568385)
Concept ACK. Nice.

@stickies-v Thanks! That's super helpful.

I agree that crypto isn't the most ideal home for hex/string functions, but it _does_ actually make it easier to use the lib as a standalone and not have to write your own versions. I don't really see any downside to having them there (no nasty dependencies, nothing stateful, etc) other than being out of place. Curious if @sipa has any specific opposition.

I'd also like to hear what @ryanofsky things about this PR, as he's bee
...

💬 theuni commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1823577233)
Concept ACK on avoiding exposing iterators externally.

Exposing references instead seems like a bit of a lateral move though, and I think it's going to take a good bit of convincing to prove that this is at least as safe than what we have now. Are references _guaranteed_ by Boost to stay valid the same as iters are?

👍 TheCharlatan approved a pull request: "Use Txid in COutpoint"
(https://github.com/bitcoin/bitcoin/pull/28922#pullrequestreview-1745358151)
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e

💬 TheCharlatan commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402699885)
NIt: Since you are switching to bracket style initialization here, do it consistently in all the initialization lines you touch?

💬 TheCharlatan commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402731424)
Nit: Maybe remove this line altogether?