📝 theuni opened a pull request: "refactor: include the proper header rather than forward-declaring RemovalReasonToString"
(https://github.com/bitcoin/bitcoin/pull/31058)
Trivial no-op fixup.

This was pointed out by #31053, which causes the include order to be shuffled around:

```
[21:49:26.130] /ci_container_base/src/validationinterface.cpp:22:13: error: redundant 'RemovalReasonToString' declaration [readability-redundant-declaration,-warnings-as-errors]
[21:49:26.130] 22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
[21:49:26.130] | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[21:4
...

💬 dergoegge commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400174059)
> Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?

What I meant, is that we no longer build the fuzz binary by default i.e. `-DBUILD_FOR_FUZZING=ON` or `-DBUILD_FUZZ_BINARY=ON` has to be added. Prior to cmake `./configure && make` would produce the fuzz binary, while now `cmake -B build/ && cmake --build build/` will not.
> How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn't compiled and
...

💬 maflcko commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400183947)
> > Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
>
> What I meant, is that we no longer build the fuzz binary by default i.e. `-DBUILD_FOR_FUZZING=ON` or `-DBUILD_FUZZ_BINARY=ON` has to be added. Prior to cmake `./configure && make` would produce the fuzz binary, while now `cmake -B build/ && cmake --build build/` will not.

I think the vanilla `cmake -B build` is for some imaginary end-user, not for developers. I imagine devs
...

💬 maflcko commented on pull request "refactor: include the proper header rather than forward-declaring RemovalReasonToString":
(https://github.com/bitcoin/bitcoin/pull/31058#issuecomment-2400190661)
lgtm ACK ca2e4ba352cb0a3b5983c002b09ce15ebbf95177

This should be fine, because the circular dependency no longer exists since commit fad8c36aa9011c3f7b1183f8380577e16a2167a6

🤔 naumenkogs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2354761295)
Reviewed up to 3b78d8d5dc325bb32ea9ff167617e4ef80110301

Still learning all the stuff i missed with 1p1c, so my eyes were mostly focused on the refactoring correctness.

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792054515)
6fbb2463840f47f9f0632431e98a68f6747805ea

This variable is kinda synchronous with `m_peer_info`.
Would be unfortunate to decouple it accidentally in future code. Perhaps comment on it?

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792037989)
6fbb2463840f47f9f0632431e98a68f6747805ea

Since this is a new variable... why not using a name more aligned with the description above? Otherwise it's hard too tell what this is about.

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792083012)
1dd47af96cdc37706cd6d3f47892822e785b6fbe
What does the following mean: `p2p_inv=false` but `AlreadyHaveTx` is true?

💬 hodlinator commented on pull request "test: Fix copy-paste in wallet/test/db_tests ostream operator":
(https://github.com/bitcoin/bitcoin/pull/31038#discussion_r1792120512)
Sorry, I wasn't fast enough.

🚀 ryanofsky merged a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967)

💬 hebasto commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2400257076)
[Qt 6.8 LTS](https://doc.qt.io/qt-6.8/supported-platforms.html#macos) dropped support for macOS 11.

💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1792155583)
Think I agree with this as well.

👍 ryanofsky approved a pull request: "Mining interface: getCoinbaseMerklePath() and submitSolution()"
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2354931253)
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5. Left minor suggestions but none are important, and looks like this could be merged as-is

💬 ryanofsky commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#discussion_r1792150348)
In commit "Move BlockMerkleBranch back to merkle.{h,cpp}" (63d6ad7c89cd682f6e779968c4861ea085058356)

Not important, but IMO it would be clearer to drop position = 0 default argument so transaction has to be specified explicitly, and rename s/Block/Transaction/ to be clear this is returning merkle path to a specific transaction.

💬 ryanofsky commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#discussion_r1792162474)
In commit "Add submitSolution to BlockTemplate interface" (525e9dcba0b8c6744bcd3725864f39786afc8ed5)

It might make more sense for this function to take a CTransactionRef parameter instead of a CMutableTransaction parameter, since it is just going to turn the CMutableTransaction into a CTransactionRef anyway.

💬 ryanofsky commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#discussion_r1792144432)
In commit "Move BlockMerkleBranch back to merkle.{h,cpp}" (63d6ad7c89cd682f6e779968c4861ea085058356)

Not important, and this is not really an uninformed opinion, but to me the phrase "merkle path" is more descriptive than "merkle branch" and I think it might be worth adding a followup commit to replace branch with path everywhere in this file so it is using consistent terminology.

Specifically might suggest: ComputeMerkleBranch -> ComputeMerklePath, BlockMerkleBranch -> TransactionMerklePa
...

📝 dollarparity opened a pull request: "build: Disallow building fuzz binary without -DBUILD_FOR_FUZZING"
(https://github.com/bitcoin/bitcoin/pull/31059)
This pull request addresses [issue #31057](https://github.com/bitcoin/bitcoin/issues/31057).

Build System:
- Modified `CMakeLists.txt` to prevent building the fuzz binary unless `BUILD_FOR_FUZZING` is enabled.
- Added a fatal error message if an attempt is made to build the fuzz binary without `BUILD_FOR_FUZZING`.

Building the fuzz binary without `-DBUILD_FOR_FUZZING` results in a binary that is less effective for testing because it won't crash on `Assume` statements and won't bypass
...

📝 dollarparity opened a pull request: "ci: Add separate fuzz-only CI jobs with -DBUILD_FOR_FUZZING"
(https://github.com/bitcoin/bitcoin/pull/31060)
Certainly! Here is a title and description for your pull request that you can copy and paste:

---

**Title:**

ci: Add separate fuzz-only CI jobs with -DBUILD_FOR_FUZZING

---

**Pull Request Description:**

### ci: Add separate fuzz-only CI jobs with -DBUILD_FOR_FUZZING

This pull request addresses [issue #31057](https://github.com/bitcoin/bitcoin/issues/31057) by updating the Continuous Integration (CI) workflow to disallow building the fuzz binary without `-DBUILD_FOR_FUZZING`
...

✅ fanquake closed a pull request: "ci: Add separate fuzz-only CI jobs with -DBUILD_FOR_FUZZING"
(https://github.com/bitcoin/bitcoin/pull/31060)

✅ fanquake closed a pull request: "build: Disallow building fuzz binary without -DBUILD_FOR_FUZZING"
(https://github.com/bitcoin/bitcoin/pull/31059)