💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2391079413)
`b98320ceff...54f4f3b05e`: address suggestions and blindly try to fix a bogus warning about uninitialized optional (didn't work)

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1786005133)
Nice, I will address it.

🤔 ismaelsadeeq reviewed a pull request: "test: Treat exclude list warning as failure in CI"
(https://github.com/bitcoin/bitcoin/pull/31018#pullrequestreview-2345259658)
utACK fa6d14eacb2a8c1c3243e3075254dfdebaa9290e

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1786018407)
> You should be able to use -DAPPEND_CPPFLAGS="-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION" (I see where the confusion comes from, as -D is accepted by compilers to define macros and by cmake to set build flags but they are different things).

Ah yes, thank you. Just saw some examples from the CI scripts.

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2391109001)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497 and https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785885817. Thanks, @dergoegge.

🤔 ismaelsadeeq reviewed a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2345306765)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241

💬 ismaelsadeeq commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1786039900)
nit: maybe just say hundreds
```suggestion
Bitcoin Core is the original Bitcoin client and it builds the backbone of the network. It downloads and, by default, stores the entire history of Bitcoin transactions, which requires hundreds of gigabytes of disk space. Depending on the speed of your computer and network connection, the synchronization process can take anywhere from a few hours to several days or more.
```

💬 ismaelsadeeq commented on issue "ci: macOS 14 CI failure `Invalid value detected for '-wallet' or '-nowallet'`":
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2391146953)
@tdb3 it might be helpful to share link to the full C.I logs?

📝 Sjors opened a pull request: "Add -pausebackgroundsync startup option"
(https://github.com/bitcoin/bitcoin/pull/31023)
When a UTXO snapshot is loaded, this option lets the user pause the verification of historical blocks in the background.

💬 Sjors commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391203396)
It might make sense to change this into an RPC method.

🤔 danielabrozzoni reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2345399717)
ACK 98c1536852d1de9a978b11046e7414e79ed40b46

Code looks good to me, I have a few nits, but feel free to ignore them.

💬 danielabrozzoni commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786093549)
nit: we could also test that verbosity > 2 is treated as 2

💬 danielabrozzoni commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786092641)
nit: this blank line seems unnecessary

💬 danielabrozzoni commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786107786)
nit: we could test that it's hidden in the functional test, similarly to `addpeeraddress`/`getrawaddrman` (https://github.com/bitcoin/bitcoin/blob/cfb59da4b3bb34afae467691a3e901f2b5a186f3/test/functional/rpc_net.py#L338-L341)

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786141210)
The `QMAKE_*` variables are used by the legacy `qmake` build tools, which we neither build nor use.

From https://doc.qt.io/qt-6/qt6-buildsystem.html:
> The Qt 5 build system was built on top of [qmake](https://doc.qt.io/qt-6/qmake-manual.html). In Qt 6, we ported the build system to CMake.
>
> **Note:** This only affects users that want to build Qt from sources. You can still use qmake as a build tool for your applications.

👍 tdb3 approved a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015#pullrequestreview-2345481436)
ACK 2957ca9611916efb570d157b9c7a0b188161660d

Not sure how many people this would affect in the near term (e.g. I've experienced Debian 12 (with apt) and Fedora 40 (with dnf) default to make versions under 3.29), but seems like a useful improvement nonetheless.

Tested with Fedora 40 manually install a more recent cmake version (3.30.4).
Saw that `make -C build test` force recompile of transaction.cpp.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786143393)
> All `top_level_*` files are not "patches". They are indeed copy-pasted from https://github.com/qt/qt5 to reproduce Qt's full source infrastructure, which is required to configure multiple separate modules at once.

These files are fetched from the Qt repository as extra sources now.

📝 AgusR7 opened a pull request: "mempool_accept.py changes"
(https://github.com/bitcoin/bitcoin/pull/31025)
refactoring check_mempool_result to check if the key doesn't exists.
max_vin precalculated directly.
some changes on outputs of logs.

Cleaned up imports and code formatting for readability, refactored methods (like check_mempool_result) for clarity, improved error handling, added comments, and ensured adherence to PEP 8 standards for better maintainability.
I hope it helps!

💬 RandyMcMillan commented on pull request "doc/build-osx.md:brew relinking note":
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1786165857)
In scenarios like this - the build just fails - a few extra lines for someone actually using the build instructions "may" find it useful.

Additionally a similar note "may" be useful in a scenarios where linuxbrew is being used.

I dont see much downside in trying to make build instructions a little more friendly to users - with minimal experience - building from source.

I'd like to think it is encouraged.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786175592)
> Is LLVM only needed for building the Qt documentation?

Additionally, it allows building a Clang-based `lupdate` parser. We explicitly disabled this feature in:https://github.com/bitcoin/bitcoin/blob/d663b154ba05ec93514125936d92a0c37a291444/depends/packages/native_qt.mk#L80