💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1267534028)
`DisconnectAndReleaseNode()` is safe because its body is protected by `m_nodes_mutex`. But `DeleteDisconnectedNode()` is not - two threads could race to call `delete` on the `CNode` object. It looks ok to iterate the whole `m_disconnected_nodes` here to avoid that, like in the other place.

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1266975864)
```
with lock: copy x to temp
process temp without lock
with lock: clear x <-- some element may have been inserted between the locks and it will be removed without processing
```

better do:
```cpp
{
LOCK(m_nodes_disconnected_mutex);
disconnected_nodes = m_nodes_disconnected;
m_nodes_disconnected.clear();
}

for (CNode* pnode : disconnected_nodes) {
DeleteNode(pnode);
}
```

this will avoid the double-free as well

💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1641518418)
Maybe Azure Pipelines can be used as an alternative to GitHub CI? Likely they use the same virtual machine pools, but the permissions and write access might be better handled?
Also, apparently all of Circle CI, Appveyor CI, and Travis CI now support macOS+Windows, so maybe they can be considered as alternative as well?

> Cirrus CI's ["Dockerfile as a CI env](https://cirrus-ci.org/guide/docker-builder-vm/#dockerfile-as-a-ci-environment)

Same here. But Linux builds should be trivial to keep
...

💬 S3RK commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1267625784)
I mean `ComputeChangeParams` computes some of `CoinSelectionParams`, but it's not immediately obvious which ones without reading the code of the function

👍 hebasto approved a pull request: "contrib: move user32.dll from bitcoind.exe libs"
(https://github.com/bitcoin/bitcoin/pull/28099#pullrequestreview-1536579264)
ACK 8c3850923300352f14dc3dde6a6ce6689ddef185, I've verified imported libraries on a Windows machine with the `dumpbin /imports` command.

Guix build:
```
f1a74cd08c886713c7d1ea90ff0119aa227f3cec21a35a68943357cc70dbe658 guix-build-8c3850923300/output/dist-archive/bitcoin-8c3850923300.tar.gz
8d1777279d231b0b85251194b7cc872d197f2c41df6dc053791f9be80d3b9523 guix-build-8c3850923300/output/x86_64-w64-mingw32/SHA256SUMS.part
b78e0f599553ce238dffe810984449a346e842d9fe16ab4b9231612089f5c717 guix
...

💬 MarcoFalke commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1641679359)
Given that the config will be bumped to gcc-12 anyway (https://github.com/bitcoin/bitcoin/pull/27897), how hard would it be to bump mingw in guix and CI, and at the same time drop this completely?

💬 fanquake commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1641683753)
> how hard would it be to bump mingw in guix

I have done the bump in Guix: https://lists.gnu.org/archive/html/guix-patches/2023-07/msg00159.html, so we'll have that once the time-machine is bumped.

💬 MarcoFalke commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1641704810)
> once the time-machine is bumped.

Doesn't this break other builds? So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?

💬 MarcoFalke commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1641709498)
(I mean, obviously my preference would be to use clang for Windows cross-builds, because mingw doesn't seem to be used by any large/relevant software project anymore, so using it at all is becoming increasingly fragile, but doing another bump shouldn't hurt too much for now)

🤔 jonasnick reviewed a pull request: "subtree: update libsecp256k1 to latest master"
(https://github.com/bitcoin/bitcoin/pull/28093#pullrequestreview-1536639285)
ACK 5080c9c25f44ae9d16a69d41f0da1d1e06483bf7

🚀 fanquake merged a pull request: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425)

📝 fanquake opened a pull request: "build: promote -Wunused-member-function to -Wunused"
(https://github.com/bitcoin/bitcoin/pull/28102)
This doesn't produce any additional warnings for me under GCC 13 or Clang 16, so it'd seem useful to use the less restricted unused code warning.

`-Wno-unused-parameter` is still required.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1267822413)
I think I messed this up in the [bad rebase](https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253703097) :( Should be fixed now.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1267822689)
Dropped

💬 kristapsk commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1641766302)
Concept ACK

📝 MarcoFalke opened a pull request: " lint: Add missing `set -ex` to ci/lint/06_script.sh "
(https://github.com/bitcoin/bitcoin/pull/28103)
Requested in https://github.com/bitcoin/bitcoin/pull/28083#pullrequestreview-1535304219.

Also, one doc commit.

💬 MarcoFalke commented on pull request "ci: Use DOCKER_BUILDKIT for lint image":
(https://github.com/bitcoin/bitcoin/pull/28083#issuecomment-1641769027)
Done in https://github.com/bitcoin/bitcoin/pull/28103

👍 hebasto approved a pull request: "depends: xcb-proto 1.15.2"
(https://github.com/bitcoin/bitcoin/pull/28097#pullrequestreview-1536745294)
ACK 7cb88c8b46723d306b96953a6a60c90a4ab211e3, tested on Ubuntu Mantic with:
```
$ python3 --version
Python 3.12.0b4
```

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1641806637)
Here is a patch on top of this PR that should address all concerns above about thread safetyness.

<details>
<summary>[patch] thread safe</summary>

```diff
diff --git i/src/net.cpp w/src/net.cpp
index c2160e945f..058f615dd5 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -886,12 +886,14 @@ size_t CConnman::SocketSendData(CNode& node) const
* to forge. In order to partition a node the attacker must be
* simultaneously better at all of them than honest peers.
* If we find
...

💬 MarcoFalke commented on pull request "build: promote -Wunused-member-function to -Wunused":
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641808324)
For gcc this is already in Wall. Not sure for clang.

Not sure if related: https://cirrus-ci.com/task/6059497324544000?logs=ci#L2338