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

💬 maaku commented on issue "Release schedule for 26.0":
(https://github.com/bitcoin/bitcoin/issues/27758#issuecomment-1823701056)
It is now 2023-11-23. Should the release schedule be updated?

📝 ryanofsky opened a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929)
Currently it is only possible to attach one serialization parameter to a stream at a time. For example, it is not possible to set a parameter controlling the transaction format and a parameter controlling the address format at the same time because one parameter will override the other.

This limitation is inconvenient for multiprocess code since it is not possible to create just one type of stream and serialize any object to it. Instead it is necessary to create different streams for differen
...

💬 ajtowns commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1402847278)
This is more generalising, than dropping...

💬 ryanofsky commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1402853197)
I guess the idea of having default parameters is off the table then. So probably the way forward to make IPC code safer is have an IPC serialization parameters like your suggestion.

For now though I left the safety issue aside and implemented a way for IPC code to pass multiple stream parameters to ParamsStream in #28929 (used in #10102 [here](https://github.com/ryanofsky/bitcoin/blob/7ba123f1b47a4a790370c9a5975b1e0b4026a241/src/ipc/capnp/common-types.h#L52-L56)), so the same stream can be us
...