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

🤔 pablomartin4btc reviewed a pull request: "Add used balance to overview page"
(https://github.com/bitcoin-core/gui/pull/775#pullrequestreview-1745481409)
Concept ACK

As mentioned on the first part/ core PR, please consider to add the wallet interface commit as the very first commit here on this PR so it's easier to test this new feature.

💬 amitiuttarwar commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1823616924)
yay!! approach ACK

💬 brunoerg commented on pull request "test: add coverage for bech32m in `wallet_keypool_topup`":
(https://github.com/bitcoin/bitcoin/pull/28928#issuecomment-1823652748)
> we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.

sgtm, perhaps 2 nodes? why only one? Also, I have no problem in addressing it here, but perhaps we could leave it for a follow-up/other PR?