💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1294900640)
nit: I think this can be removed?

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1295144003)
unrelated: Maybe remove this and replace it with a check on CI_OS_NAME?

🤔 mzumsande reviewed a pull request: "Fix potential network stalling bug"
(https://github.com/bitcoin/bitcoin/pull/27981#pullrequestreview-1579515115)
Tested ACK 3388e523a129ad9c7aef418c9f57491f8c2d9df8

I now managed to reproduce the deadlock described in the OP by

1.) adding a debug-only `sendmsgtopeer` rpc as suggested by @ajtowns above (https://github.com/mzumsande/bitcoin/commit/9c90e5dc33051ff9d48160c831a63c434ddb4e4d)

2.) Creating a functional test that uses this rpc to simultaneously have two nodes send a large message (4MB) to each other. (https://github.com/mzumsande/bitcoin/commit/70e6752c3a66ecf414be1bd16ca80ba4c3c4fa63)

...

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295208216)
Done.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295208716)
I've made some significant changes to the code, and `DoneSendingMessage` is now gone: instead the caller just tries `SetMessageToSend`, and it'll fail if the message can't be set. This avoids a virtual function call in some cases, doing both at once.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295208895)
`DoneSendingMessage` is gone.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295208988)
I've added a comment to this effect.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209060)
Done.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209148)
Done.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209305)
Done (and also updated the one in the next commit).

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209655)
I've added some more comments in various places hopefully addresses this complexity, foreshadowing that some added code will be removed in a future commit.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209819)
I've added a comment to explain this better.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209963)
Not done yet; will do on a future push.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295210296)
The comment is gone (due to significant changes; the code also works differently now).

💬 theStack commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#discussion_r1295210529)
Strictly speaking, wouldn't this sentence belong to `STANDARD_NOT_MANDATORY_VERIFY_FLAGS` rather than to `STANDARD_SCRIPT_VERIFY_FLAGS`? The latter includes mandatory flags, for which a violation _does_ lead to a ban/disconnect, as just stated in the same commit a few lines above ("may trigger a DoS ban").

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295210753)
I haven't added a comment here as I'm not entirely sure under what scenarios `if (!node.m_sock)` triggers; it's code that existed beforehand, and is untouched by this PR.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295211040)
I've added a commit that renames all receive-side functions to have "Receive" somewhere in the name.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295211509)
It can be (and is), due to optimistic send logic triggering SocketSendData, which now moves `vSendMsg` objects into the transport.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295214603)
It's complicated. You're right that duplicating the old `prepareForTransport` here would work, but I feel that's not really the right approach, as it's kind of hiding what's really going on. It'd also be incompatible with a potential future upgrade to using v2 transports inside the affected unit and fuzz tests.

What's really going on is that we're using the CNode's own *sending* infrastructure to construct bytes, which are then fed to the same CNode's *receive* infrastructure. But there is al
...

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1679726704)
I've made some significant changes to this PR:
* `DoneSendingMessage` is gone (the caller can just call `SetMessageToSend` directly, which will fail if nothing can be sent at that time).
* `HaveBytesToSend` is gone (the caller can just call `GetBytesToSend`, which will report an empty span if nothing is to be sent).
* `GetBytesToSend` now takes a `have_next_message` as input, which lets its prediction for whether there are more bytes to send afterwards be more accurate (letting it take into a
...