💬 dplusplus1024 commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1679571264)
NACK - Let's keep mining decentralized.

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679607077)
> I don’t think that an absolute value will work. My guess is that the crash may be caused by m_cost_of_change being generated randomly independent from m_change_fee. Usually, cost_of_change cannot ever be smaller than m_change_fee, so if it happens to be smaller here, it may interfere with the change calculation?

Bingo, I think so!

💬 achow101 commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#issuecomment-1679618148)
ACK faaba770e11352ddf6414b9855f4baa46a967580

🚀 achow101 merged a pull request: "ci: Run "macOS native x86_64" job on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28187)

💬 achow101 commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1679610740)
ACK 9658d0dc17c270138523c41a982425e276b24271

🚀 achow101 merged a pull request: "refactor: Remove unused boost signals2 from torcontrol"
(https://github.com/bitcoin/bitcoin/pull/28240)

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679645106)
That was loosely inspired by the [vsize of a P2TR input](https://bitcoin.stackexchange.com/q/111395/5406) (rounded up).

I’m just seeing that we are calculating the cost_of_change actually with the `discard_feerate` rather than the `LTFRE`, though:

`src/wallet/spend.cpp: coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee;`

You could then also make the `change_spend_size`
...

💬 stickies-v commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1679647671)
Force pushed to update documentation [addressing this review comment](https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1293632896).

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1295144563)
Should also abort on a force push, to avoid wasting CPU, no?

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