🤔 ismaelsadeeq reviewed a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2384720527)
Concept ACK

💬 fanquake commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#issuecomment-2428976762)
Two other things that should happen at the same time as this are adding the linker (this is somewhat compensated for via adding the c/xx flags), as well as making the flag overriding work correctly.

💬 fanquake commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2428995399)
> It is definitely broken in https://github.com/bitcoin/bitcoin/pull/30997.

Shouldn't #30997 be based on this PR then (or at least mention in the PR description it's expected to be broken)?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1810527928)
In e0b79e029f15cf2e984c513e230b0d7c7914554d (build: Remove problematic code ): Can you add a sentence explaining why it's problematic?

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2429007032)
`6b10008441...ec7693a9aa`: take suggestions from https://github.com/bitcoin/bitcoin/pull/29420 - in the SOCKS5 proxy, put the sockets forwarding snippet from `socks5.py` to its own function and retry `send()`s.

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#issuecomment-2429009514)
`f93fab4c58...57529ac4db`: address suggestions

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810539480)
I moved it to its own function, but in `test/functional/test_framework/socks5.py` because it uses the newly created `sendall()` (due to the [suggestion below](https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1802683914)) which is very similar to `recvall()` which already exists in `socks5.py`.

I guess if this is to be moved to `test/functional/test_framework/netutil.py`, then all 3 of `recvall()`, `sendall()` and `forward_sockets()` should be moved. Leave it as is or move to `netutil
...

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810559925)
Ok, [`sendall()`](https://docs.python.org/3/library/socket.html#socket.socket.sendall) doc says "On error, an exception is raised, and there is no way to determine how much data, if any, was successfully sent" so this is useless for us. The doc says as well that since Python 3.5 the method will retry if the syscall is interrupted, but mentions nothing about `EAGAIN` or `EWOULDBLOCK`.

So I followed https://docs.python.org/3/howto/sockets.html#using-a-socket and used the basic `send()` with a m
...

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810578522)
Following discussion with @tdb3, how do we know this code even works and is not totally broken? That is a reasonable question because this PR does not demonstrate that this works.

https://github.com/bitcoin/bitcoin/pull/29415 (which includes this PR) exercises the newly added functionality. The tests from https://github.com/bitcoin/bitcoin/pull/29415 that use this depend on the private broadcast functionality so can't be included here.

💬 Christewart commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2429112141)
ACK bf46723537c37cb1ee7f84ffe7b75c253beadb89

💬 garlonicon commented on pull request "miner: Reorg Testnet4 minimum difficulty blocks":
(https://github.com/bitcoin/bitcoin/pull/31117#issuecomment-2429183236)
> The rationale was that this would be the only way developers could still get their non-standard transactions into the blockchain.

Well, the solution to that is quite simple: if non-standard transactions would be normally propagated, then developers could simply publish them, and have them confirmed. Unless you want to get an additional spam protection, by requiring every developer to provide 32 leading zero bits in Proof of Work. But in that case, just doing OP_SIZE on a signature will give
...

💬 hebasto commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1810669866)
CMake has some variables/properties that _must_ be set before the `project()` command. And the `CMAKE_OPTIMIZE_DEPENDENCIES` is not one of them. For the sake of clarity, I suggest to keep only necessary stuff before the `project()` command.

🤔 hebasto reviewed a pull request: "build: speed up by flattening the dependency graph"
(https://github.com/bitcoin/bitcoin/pull/30911#pullrequestreview-2385111644)
Tested and benchmarked on Ubuntu 23.04 with CMake 3.27.4 using the following command:
```
$ rm -rf build && cmake -B build -G Ninja --preset dev-mode -DWITH_MULTIPROCESS=OFF -DWITH_CCACHE=OFF && time cmake --build build -j 16
```

- The master branch @ 6fc4692797121b54de0c54e5b09ee47f322c038a:
```
real 6m44.237s
```

- 330d16e1aa8fe8d7a5bb755189d0d2991fef8a43:
```
real 6m45.264s
```

- 42edb2f4a5900e0b2451a7446d58962c0bf0095d
```
real 6m45.859s
```

I'm OK with the flattened
...

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1810686447)
Should this be just logged once; i.e after replacing the transactions.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1810694092)
Why are we only tracing the first tx in the package

👍 stickies-v approved a pull request: "[27.x] Prep for 27.2"
(https://github.com/bitcoin/bitcoin/pull/31101#pullrequestreview-2385156244)
ACK 0cdfb7e45c623b89d37b5785cae8f2111cb450cc

I'm getting the same manpages, and no diff since #30558. So unless more backports come in, this looks good to go.

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2385159381)
Concept ACK

The changes are straightforward to understand, and the large diff is primarily due to changes in the error message and the removal of `addUnchecked`.

🤔 hebasto reviewed a pull request: "RFC: build: support for pre-compiled headers."
(https://github.com/bitcoin/bitcoin/pull/31053#pullrequestreview-2385174326)
Concept ACK.

Tested and benchmarked on Ubuntu 23.04 with CMake 3.27.4 using the following command:
```
$ rm -rf build && cmake -B build -G Ninja --preset dev-mode -DWITH_MULTIPROCESS=OFF -DWITH_CCACHE=OFF && time cmake --build build -j 16
```

- The master branch @ 5fb94550638d0b01c184d2f3d5d97c8874c8c34b:
```
real 6m46.350s
```

- this PR @ 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8:
```
real 6m10.958s
```

---

Do you want to keep the `base_precompiled` target or use the `PRE
...

💬 hebasto commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#discussion_r1810708593)
nit:
```suggestion
message("Precompiled headers ................... ${pch_status}")
```

📝 darosior opened a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130)
This PR removes UPnP IGD support and drops our [miniupnp](https://github.com/miniupnp/miniupnp) dependency.

Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed [here](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=miniupnp)), some of which directly affected our software ([RCE in 2015](https://bitcoincore.org/en/2024/07/03/disclose_upnp_rce/), [OOM in 2020](https://bitcoincore.org/en/2024/07/31/disclose-u
...