💬 Malk8628 commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283620563)
I've been hacked

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283633525)
In general, it's true that the new sending interface is overly complicated for how it is used initially, and then it only becomes apparent in the next commit.

Here specifically, indeed, everything returned by `GetBytesToSend` is initially always marked sent, but in the next commit that changes, as the buffering responsibility is moved from `vSendMsg` to `m_transport`. I'm happy to add (possibly temporary) comments to explain what's happening.

📝 hebasto opened a pull request: "scripted-diff: Specify Python major version explicitly on Windows"
(https://github.com/bitcoin/bitcoin/pull/28213)
On Windows, it is the accepted practice to use `py.exe` launcher:
- https://learn.microsoft.com/en-us/windows/python/faqs#what-is-py-exe-
- https://docs.python.org/3/using/windows.html#python-launcher-for-windows

One of its features is the correct handling of shebang lines like the one we use: `#!/usr/bin/env python3`.

However, Windows OS app execution aliases might [interfere](https://learn.microsoft.com/en-us/windows/python/faqs#why-does-running-python-exe-open-the-microsoft-store-) wi
...

💬 Malk8628 commented on pull request "refactor: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1283655386)
Bought

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283660977)
I don't reordering works without (even) more intermediary complexity. The issue is "net: move message serialization from PushMessage to SocketSendData" changes the data type of `vSendMsg` from bytes-to-be-sent to messages-to-be-sent, and the latter just don't have a known size-on-the-wire (unless an additional API to transports is added for that, or hardcoding the V1 message encoding size rules). That's why this PR first changes the notion of send buffer size: the old notion just doesn't really
...

💬 sipa commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#issuecomment-1664584088)
utACK f054bd072afb72d8dae7adc521ce15c13b236700

👍 john-moffett approved a pull request: "refactor: serialization simplifications"
(https://github.com/bitcoin/bitcoin/pull/28203#pullrequestreview-1561791609)
ACK f054bd072afb72d8dae7adc521ce15c13b236700

💬 instagibbs commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283672181)
if something should raise an error and a specific code/message, you use `assert_raises_rpc_error`. Just take a look on how it's used elsewhere?

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283674053)
Actually, that name would be confusing for V2, where it's possible that there is no pending message, but also no message can be provided (yet) because the handshake has not completed.

Perhaps `CanSetNewMessageToSend()` is better? (let the bikesheddening begin!)

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283682048)
The `ForEachNode` lambda is already annotated with `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` so all that 's needed is to add `m_connman.m_nodes_mutex` to that. That doesn't work directly, because `m_nodes_mutex` is private and the `ForEachNode` is in a different class, but you can work around that by declaring:

```c++
public:
// alias for thread safety annotations only, not defined.
RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);
```

in `CConnman`, and writing:
...

👍 ViljoPennanen approved a pull request: "scripted-diff: Specify Python major version explicitly on Windows"
(https://github.com/bitcoin/bitcoin/pull/28213#pullrequestreview-1561825694)
lgtm

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1561722494)
Updated 08f5febfc571220043436bbec96a326beebdee22 -> b0beb4c504da29c27358d4602a045aaab39305f6 ([`pr/bresult2.37`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.37) -> [`pr/bresult2.38`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.38), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.37..pr/bresult2.38)) moving a lot more functionality from the `Result` class to the `ResultBase` class so the new code can be compatible with the `ResultPtr` class from #26022
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283630635)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282487687

> Here and line 192 below: "earning"?

Thanks, rewrote these comments now

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283624276)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282502389

> also, per the lint CI / spelling linter

Thanks, fixed spelling

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283623667)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282503773

> Tidy CI iwyu suggestions

Thanks applied these

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283623420)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282499219

> [584e3fa](https://github.com/bitcoin/bitcoin/commit/584e3fa4ea8ca15b3a8b46e023f218c5e0ed73b0) Not sure, but these `if (!result) return result;` idioms (here and lines 228-229 below) seem "odd" enough that an explanatory comment might be helpful.

I think this will probably be a common pattern and it would be too verbose to explain what is happening each place it is used. If the code were misleading, I think I would w
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283629856)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282507096

> [6ddcc9b](https://github.com/bitcoin/bitcoin/commit/6ddcc9bec8f64148b7f4b6d03b8dd85a5e748175) `GetErrors()` and `GetWarnings()` don't seem to have any test coverage yet. Note that there is also already a `GetWarnings()` in `src/warnings.{h.cpp}`, perhaps disambiguate naming/grepping-wise in addition to namespace-wise.

Thanks, added some test coverage. On the `warnings.h` overlap, I think that `GetWarnings()` is not
...

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1283703466)
Done

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1283703631)
Done

✅ fanquake closed a pull request: "p2p: diversity network outbounds follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28189)