💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1266938623)
`GlobalMutex` silences some of the thread safety analysis and was introduced to overcome some limitations on those, is supposed to be used only for mutexes that are defined globally. I agree it is confusing. I don't like it and have a plan to remove `GlobalMutex` but it is stuck at https://github.com/bitcoin/bitcoin/pull/25390.

<details>
<summary>[patch] change to Mutex</summary>

```diff
diff --git i/src/net.cpp w/src/net.cpp
index c2160e945f..c8ee95a3d0 100644
--- i/src/net.cpp
+++ w
...

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1267530707)
I love this lock free pattern!

The variable used as a guard (`nRefCount`) has to be outside of the object being protected. Otherwise if two threads execute it concurrently the first one may swap from 0 to -1 and `delete` the object. Then the second thread will end up reading freed memory when it tries to use `nRefCount`.

This should be safe:

```cpp
// Delete disconnected nodes. Call DeleteNode() without holding m_nodes_mutex or m_nodes_disconnected_mutex.
std::vector<CNode*> t
...

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