💬 jonatack commented on pull request "refactor: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#issuecomment-1664459802)
Updated the pull description and repushed to take @MarcoFalke's review feedback (thanks!)

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283585262)
updated in latest push. I think I caught them all

💬 instagibbs commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283587279)
`assert_raises_rpc_error` is probably what you want

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283594372)
updated to an `std:array` in the latest push

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283595087)
incorporated here in latest push

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283596113)
done in latest push

💬 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