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

💬 hebasto commented on pull request "depends: bump boost to 1.86.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#discussion_r1810740623)
It seems that the previous variant is correct:
```suggestion
$(package)_config_opts_darwin=-DCMAKE_INSTALL_NAME_TOOL=true
```

💬 TheCharlatan commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2429305648)
Concept ACK

💬 hebasto commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2429319684)
TO fix CI failure:
```diff
--- a/src/mapport.cpp
+++ b/src/mapport.cpp
@@ -2,8 +2,6 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

-#include <bitcoin-build-config.h> // IWYU pragma: keep
-
#include <mapport.h>

#include <clientversion.h>
```

💬 hebasto commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#issuecomment-2429329095)
My Guix build:
```
aarch64
6361934f5c9bd884aff2ef18000ca718974d8ea564939db6acf3f3ac6faa5e35 guix-build-cd048e03e258/output/aarch64-linux-gnu/SHA256SUMS.part
d3be037f7cd976b315f96303a56952e3eb5bb8f1963d01c0ede701af2cfee83a guix-build-cd048e03e258/output/aarch64-linux-gnu/bitcoin-cd048e03e258-aarch64-linux-gnu-debug.tar.gz
7c6e75134c8b6614758c024b7bdfcd3bddd0b652d374cbb852e365f00b360a18 guix-build-cd048e03e258/output/aarch64-linux-gnu/bitcoin-cd048e03e258-aarch64-linux-gnu.tar.gz
4514a22f
...

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2429342997)
Big concept ACK on this. As mentioned, MiniUPnP is some pretty dangerous code that resulted in the only known exploitable RCE in bitcoin core in history. It does brittle C-based string manipulation to generate and parse XML and HTTP which still has a high risk of having further bugs.

For that reason it's been disabled by default for years, there is no path toward doing so, and meanwhile i doubt many people have been going through the hassle of enabling UPnP in the settings to open a port.


...

💬 hebasto commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1810790477)
I'm not sure about that as none of the `{CPP,C,CXX,LD}FLAGS` variables is propagated to the native packages:
```
$ cd depends
$ make HOST=arm64-apple-darwin MULTIPROCESS=1 print-native_libmultiprocess_cxxflags CXXFLAGS=-some-fancy-flag
native_libmultiprocess_cxxflags=
```

💬 hebasto commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1810791006)
nit:
```suggestion
# [ CPPFLAGS=... ] [CFLAGS=...] [CXXFLAGS=...] [LDFLAGS=...] \
```

---

While touching this code, the comment can be improved:
```diff
--- a/depends/gen_id
+++ b/depends/gen_id
@@ -3,7 +3,7 @@
# Usage: env [ CC=... ] [ C_STANDARD=...] [ CXX=... ] [CXX_STANDARD=...] \
# [ CPPFLAGS=... ] [CFLAGS=...] [CXXFLAGS=...] [LDFLAGS=...]
# [ AR=... ] [ NM=... ] [ RANLIB=... ] [ STRIP=... ] [ DEBUG=... ] \
-# [ LTO=... ] [ NO_
...

💬 TheCharlatan commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1810793581)
Nit: While you are at it maybe break before `./gen_id` too for readability purposes?

💬 TheCharlatan commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1810796779)
Mmh, isn't that intentional?

💬 mzumsande commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2429390190)
> In my understanding, what you're describing is the following:

This looks like a separate issue. I was talking about orphan resolution removing the tx, you are talking about `TxRequestTracker` expiration removing it.

> I can go to any of your PRs too, change the code with one line, and say the code is failing dozen of tests

I wasn't changing a random line of code: the added code is guarded by a future P2P version upgrade and therefore unreachable and untestable as is - one way to expos
...