👍 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?

💬 TheCharlatan commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402766575)
Nit: Just do `Txid{}`, like you did in `bench/disconnected_transactions`?

🤔 pablomartin4btc reviewed a pull request: "Wallet: Functions to enable adding used balance to GUI overview page"
(https://github.com/bitcoin/bitcoin/pull/28776#pullrequestreview-1745476888)
Concept ACK

Light CR, left a suggestion if I understand it correctly.

Also, since this is a very small change on the `wallet` interface, perhaps makes more sense to add this commit first on the `GUI` PR [#775](https://github.com/bitcoin-core/gui/pull/775) and would be easier to test the `gui` as well.

💬 pablomartin4btc commented on pull request "Wallet: Functions to enable adding used balance to GUI overview page":
(https://github.com/bitcoin/bitcoin/pull/28776#discussion_r1402774586)
GetBalance receives `bool avoid_reuse` in the last argument, should not be `true` or just remove the if condition directly and pass the result of `IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)` entirely.
```suggestion
const auto full_bal = GetBalance(*m_wallet, 0, true);
```

💬 TheCharlatan commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1823599516)
Re https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1823577233

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

We're using the references now to keep track of the parents and children. I don't see what new assumption this patch is introducing compared to this existing practice.

> Are references guaranteed by Boost to stay valid the same as iters are?

I'd be interested in getting an answer to this though
...