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

💬 russeree commented on issue "Raise maximum -dbcache setting":
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1679735177)
Would the eBPF USDT tracepoints for the UTXO cache be of use here as a way to measure the overall impact of these flush operations during and IBD?

https://github.com/bitcoin/bitcoin/tree/master/contrib/tracing#log_utxocache_flushpy

💬 theStack commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1679759413)
Concept ACK

💬 ismaelsadeeq commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1295130079)
Coming from review club:
Should also be moved to secp256k1 right