💬 pablomartin4btc commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402451595)
Are you keeping this and removing it in a follow-up?

https://github.com/bitcoin/bitcoin/blob/9e58c5bcd96e7ff2062274868814ccae0626589e/src/util/transaction_identifier.h#L53-L60

Same for the comparisons above those lines (#L18 `// TODO`).

📝 brunoerg opened a pull request: "test: add coverage for bech32m in `wallet_keypool_topup`"
(https://github.com/bitcoin/bitcoin/pull/28928)
0dcac51049cdd924a50d62629757effc8d542046 added coverage for all keypool addresses types in `wallet_keypool_topup` (4y agp). Now we have bech23m, so this PR adds it.

👍 TheCharlatan approved a pull request: "[26.x] Changes for rc3"
(https://github.com/bitcoin/bitcoin/pull/28872#pullrequestreview-1745009742)
ACK 2f86d3053314c382dfce5cf314e98811ed433859

💬 pinheadmz commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1823254133)
> cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-)

Hi, post merge code review ACK.

I have a FreeBSD container on google cloud I just fire up when needed.

The bug you're fixing here is apparently not an issue on my OS:
`FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64`

The bitcoin unit test passes without this patch and `ping 127.1` also works fine.

🚀 fanquake merged a pull request: "[26.x] Changes for rc3"
(https://github.com/bitcoin/bitcoin/pull/28872)

✅ fanquake closed a pull request: "Create master"
(https://github.com/bitcoin/bitcoin/pull/28593)

📝 fanquake locked a pull request: "Create master"
(https://github.com/bitcoin/bitcoin/pull/28593)
<!--
*** 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
...

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