💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879236472)
@maflcko I've added a commit to verify that... it adds a hardware-representation equivalence test again, but starting from a `double` like the fuzz tests do.

💬 tcharding commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879236759)
> Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus
>
> Ping @apoelstra @tcharding

`rust-bitcoinconsensus` can just do a "final" release using v27 and keep existing if folk want to use it, its trivial to maintain.

👍 maflcko approved a pull request: "Weaken serfloat tests"
(https://github.com/bitcoin/bitcoin/pull/29192#pullrequestreview-1806821660)
lgtm

💬 maflcko commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443356658)
I think in C++20 a static_assert is evaluated even in a non-instantiated context, so could either make this global use the "template-hack" to not evaluate it if the context isn't instantiated either.

💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443361139)
Right.

I'll address this (and update the description of the PR which is now outdated with the last commit) when @fanquake reports this fixes #28941.

💬 sr-gi commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1879253826)
Concept ACK. Just a single consideration: wouldn't it also make sense to test this with `-logips=1`?

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1443367173)
`self.def_wallet` is not equal to `self.wallet`, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?

💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879268385)
> * Identifying extra data yet removing the witness discount rather than filtering it out entirely. It's not clear this would be effective alone, but is supported by Knots v25.1.
> * Adding a second `datacarriersize` with a broader scope like in [datacarriersize: Match more datacarrying #28408](https://github.com/bitcoin/bitcoin/pull/28408) ([suggested by glozow](https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1878972398)).

It seems like an acceptable compromise solution would b
...

🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1806935444)
Pre-review note: have you considered that `wallet_import_rescan.py` is a legacy wallet only test?
If it's placed here due to a lack of alternatives, I think this could be moved to a separate `wallet_mempool.py` file, where we could continue adding more cases related to the wallet-mempool interaction (or.. we could upgrade this file to run on a descriptors wallet).

🤔 jonatack reviewed a pull request: "doc/reduce-traffic: update/clarify max outbound connection count"
(https://github.com/bitcoin/bitcoin/pull/29052#pullrequestreview-1806956443)
Post-merge review ACK with logging sanity check

```
2024-01-05T21:54:39.437118Z [Init] m_max_automatic_connections: 125
2024-01-05T21:54:39.437125Z [Init] m_max_outbound_full_relay: 8
2024-01-05T21:54:39.437133Z [Init] m_max_outbound_block_relay: 2
2024-01-05T21:54:39.437139Z [Init] m_max_automatic_outbound: 11
2024-01-05T21:54:39.437145Z [Init] m_max_inbound: 114
```

💬 jonatack commented on pull request "doc/reduce-traffic: update/clarify max outbound connection count":
(https://github.com/bitcoin/bitcoin/pull/29052#discussion_r1443441377)
Perhaps just my read on this, but the following may have been clearer with respect to the inbound number also being the default setting only, and not an absolute limit.
```suggestion
which are outbound and 114 are inbound.
```

💬 jonatack commented on issue "Nit: Inconsistency in the docs regarding block-relay-only connections":
(https://github.com/bitcoin/bitcoin/issues/29046#issuecomment-1879334672)
> ...so reduce-traffic.md could be updated to mention that there are occasionally up to 11 outbound nodes, which it appears @MarnixCroes has just done :)

If we wanted to be pedantic, 8 full-relay + 2 block-relay-only + 1 feeler + 1 extra block-relay-only peer would be an occasional max of 12 outbound peers. The change I made in `reduce-memory.md` was off by one in stating 11 🤠

💬 furszy commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1443490686)
In c9746711:

Need to flush the validation queue before checking the wallet balance. The transaction could have not been processed by the validation interface worker thread. Call `self.wallet.syncwithvalidationinterfacequeue()`.

(saw it failing locally).

💬 furszy commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1443495832)
> `self.def_wallet` is not equal to `self.wallet`, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?

Need to call `syncwithvalidationinterfacequeue()` to flush the validation queue before checking the wallet balance. The transaction could have not been processed by the validation interface worker thread which is the one dispatching the tx to the second wallet.

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1443510518)
Did it. I was thinking of moving `master_key` outside as well but since it is `const` it wouldn't make sense. Should I add a function in `CallOneOf` that updates the `random_key` occasionally as well?

🤔 mzumsande reviewed a pull request: "rpc: add 'getnetmsgstats' RPC"
(https://github.com/bitcoin/bitcoin/pull/28926#pullrequestreview-1807023183)
Concept ACK. We are using this (in an adjusted form with another dimension for txrelay) for analysing the traffic implications of #28463, and it works great so far.

💬 mzumsande commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1443508738)
Would be nice to have some validation of user input here - `getnetmsgstats '["foo","bar"]'` could return an error message, listing the supported dimensions.

💬 mzumsande commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1443492341)
nit: capital M, here and in `messageTypeFromIndex`

💬 mzumsande commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1443485004)
I agree: Code and constants that are only used in RPCs are not part of the protocol and therefore don't belong in `protocol.h` / `protocol.cpp`.

💬 achow101 commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#issuecomment-1879421327)
Marking as up for grabs.