💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679360517)
@murchandamus I got a crash with 5000 as well, perhaps we should increase the value even more.

💬 MarcoFalke commented on pull request "ci: Fix macOS-cross SDK rsync":
(https://github.com/bitcoin/bitcoin/pull/28273#issuecomment-1679380245)
Still fails:

```
clang: warning: no such sysroot directory: '/tmp/cirrus-ci-build/depends/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers' [-Wmissing-sysroot]
ld: library not found for -lc++
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

https://cirrus-ci.com/task/6482314977345536?logs=ci#L676

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1679417885)
> Approach ACK. I wouldn't say the API here is perfectly ideal because it doesn't check everything at compile time that could theoretically be checked at compile time.

I don't think any API changes will be required, assuming C++20 const(expr/init/eval), to turn run-time checks into compile-time checks, but I haven't confirmed this.

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679417244)
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?

I attempted to calculate `cost_of_change` instead from `m_change_fee + input_size×LTFRE`, leaving the fuzz input consumption unchanged. It made the test pass:

```diff
@@
...

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

...