💬 achow101 commented on pull request "test: check backup from `migratewallet` can be successfully restored":
(https://github.com/bitcoin/bitcoin/pull/28257#issuecomment-1679485865)
ACK 769f5b15f207ce6d1067672ea5e195541c97de6b

💬 achow101 commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1295036091)
I think the since the libevent structs in this file are only pointers, the actual definitions aren't needed hence the include is not strictly necessary?

💬 theuni commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1295048904)
Yeah, iirc everything in libevent can be forward-declared except for `evutil_socket_t`. It's very annoying.

Because everything else is already forward-declared as @achow101 mentioned, only the definition of `evutil_socket_t` is needed. `evutil_socket_t` is actually defined in `event2/util.h` (note that `event.h` includes `util.h`.

So I agree with this change. It's a small reduction in needed includes.

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1679547504)
Updated for @vasild's review feedback to change `m_added_nodes` from a vector to an unordered set (https://github.com/bitcoin/bitcoin/pull/28248/commits/3f4a1f0bc98159f86d910227fd57d05d10bb8889), which simplifies and speeds up the `AddNode()` and `RemoveAddedNode()` methods along with the commit that follows it, `p2p: do not make automatic outbound connections to addnode peers` (048e1af).

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