💬 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

💬 hebasto commented on pull request "build: promote -Wunused-member-function to -Wunused":
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641810745)
For clang, [`-Wunused`](https://clang.llvm.org/docs/DiagnosticsReference.html#wunused) does not control `-Wunused-member-function`, no?

💬 hebasto commented on pull request "depends: xcb-proto 1.15.2":
(https://github.com/bitcoin/bitcoin/pull/28097#issuecomment-1641813666)
Guix builds:
```
6d1725c9346bdf04e1eeec2deffda90957bd081700ba896834a143756410fb0e guix-build-7cb88c8b4672/output/aarch64-linux-gnu/SHA256SUMS.part
34486c39daea8a3ce8d92e9c1501f26e5f5e300d9612a25bd56c48ab56353329 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu-debug.tar.gz
cb0080d75d0fc4fc5c7fc022b5771470c60609c35836e7b66c4f479a2edcd098 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu.tar.gz
395c332f83206125819
...

💬 fanquake commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1641813690)
> I don't care about updating the existing code,

To be clear, I agree, and am still a NACK to any code changes here.