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

🤔 ajtowns reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1745623209)
Concept ACK

💬 ajtowns commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1402864403)
Dropping `SERIALIZE_METHODS_PARAMS` doesn't seem like an improvement?

I would have expected something more like:

```c++
// this pr: #define SER_PARAMS(type) (s.template GetParams<type>())
// instead:
template<typename T, typename Stream>
const auto& GetSerParams(const Stream& s)
{
return s.template GetParams<T>();
}

// FORMATTER_METHODS_PARAMS has the same signature, but now uses the new GetSerParams<>()
#define FORMATTER_METHODS_PARAMS(cls, obj, paramcls, paramobj) \
t
...

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1402865607)
I'm just saying what my opinion is; doesn't mean I'm necessarily right. What you've come up with looks pretty good to me though.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1402993598)
I'm going to drop this test, unless you know a way how to handle this better.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403006667)
You're right.

My fear with this is unexpected behavior for tx sender: e.g., you craft a "package" thinking parent always goes ahead, but then child gets ahead (potentially with the attacker's help) and dropped on the floor due to some policy. Something along this, but maybe I'm making it up.
Are these concerns at least semi-valid? @glozow

I can add "see whether a parent is in the set already" check, when looking at a child, if we think it's worth it.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403013326)
I think i'd rather drop it from `TryRemovingFromSet`. It's not that strong anymore anyway, doesn't help much and pretty obvious i think. Let me know if you think differently.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403014150)
took this, but generally i think we have these mistakes all over the code and it's fine :)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403018216)
You're right, i shall think how to make it better.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403018671)
no, it's legacy i guess. wondering why the compiler haven't found it.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403019080)
i think it's from the latter commits. can be dropped here.

🤔 Zemaya366 reviewed a pull request: "[25.1] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/28655#pullrequestreview-1745902980)
I need help

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1403082895)
nvm, I don't think this is actually an improvement.

💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403130801)
This comment should be adjusted, since there is no `arith_uint256(const std::string& str)` constructor anymore.