💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283597304)
@paplorinc I like the second approach, it seems a bit cleaner and clearer to me. However, it doesn't seem to be used in other functional tests so maybe the first approach is better to be more consistent?

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283599071)
added the network to print statement

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283599461)
@instagibbs Thanks! I was looking at that option too, however I couldn't see a way to use it in this context without having to change a lot of the code, but maybe I'm missing something obvious? 🤔

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283605312)
We can - In fact the handshake itself is sent this way (that's the nice part about this abstraction, the caller doesn't know or care whether bytes being sent are on behalf of a message we're trying to send or something else).

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283606280)
Yeah, it should be 0 at this point, but that won't remain with the next commit. A temporary explanation message could be added to explain that here.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283610490)
It's because I don't want a recursive lock, and I'm introducing a caller of `CompleteInternal()` that already holds `m_cs_recv`.

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283615894)
good catch! totally agree with the suggestion - def agree that we shouldn't introduce more uses of recursive mutexes, esp when not necessary.

that said, I'm struggling a bit to communicate with the compiler-

when I add both the runtime `AssertLockHeld` and annotation of `EXCLUSIVE_LOCKS_REQUIRED`, I'm getting a compile time warning from `-Wthread-safety-analysis`

<details>
<summary>diff</summary>


```
diff --git a/src/net.cpp b/src/net.cpp
index a504289fa5..228d081aa6 100644
--
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1664500630)
thanks for the reviews & feedback @naumenkogs, @vasild, @willcl-ark & @fanquake

I've updated this PR to incorporate all the open feedback & fix up the commit messages. everything should be addressed except for these two open items:
1. question about locking improvement https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283615894
2. release note which I will do in the follow-up, to prevent possible review churn on language feedback

otherwise, should be ready for re-review. thank
...

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