👍 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)

🤔 mzumsande reviewed a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046#pullrequestreview-2355081461)
one more small thing that I noticed reviewing #30968 (it wasn't really on-topic there, but fits with the last commit here): I don't think that we should return `ChainstateLoadStatus::FAILURE` in https://github.com/bitcoin/bitcoin/blob/5837e3463fe188a65d67f96e84cbcca674d61d9e/src/node/chainstate.cpp#L239 (snapshot validation failure). This is not a problem where a general reindex would typically help, and is handled inside of validation (`InvalidateCoinsDBOnDisk`) anyway.

👍 ryanofsky approved a pull request: "Add waitFeesChanged() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31003#pullrequestreview-2354996475)
Code review ACK 9a723c784ad5170b1e01e91f018e5794a51dd038, but implementation has a few rough edges described below, which would recommend fixing.

Also I think it would be a lot clearer to squash the two commits. If second commit is a problem for "(very) low end hardware" as described, I think it might make sense to make fee_threshold a std::optional parameter so callers running on slow hardware can skip the fee calculation. Alternately could tweak sleep_until to make sure the function is sle
...

💬 ryanofsky commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1792221843)
In commit "Have waitFeesChanged() frequently make a template" (9a723c784ad5170b1e01e91f018e5794a51dd038)

It seems like there is a bug here (also present in the previous commit) where `last_mempool_update` variable is not updated during the loop, so once the value changes a single time, this check will always be true, and the same block could be assembled wastefully every tick.

Also, it seems like there is also a race condition in the opposite direction that could lead to stale results, whe
...

💬 ryanofsky commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1792180834)
In commit "Introduce waitFeesChanged() mining interface" (a87589abfde4f2f1dfba0f3f1f5745f7acfef365)

Would suggest deleting this. I don't think this can do anything because the value passed to the sleep function is already capped by `now + tick`

💬 LarryRuane commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1792260314)
Thanks, I don't mind removing the code changes at all (I was on the fence on this part of the change), should we wait a little while to see if there are other opinions?

🤔 maflcko reviewed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2355129479)
Left a nit.

Only change is adding `throw` when `DEBUG`, which seems fine.

re-ACK 49f73071a8ec4131595090a619d6198a5f8b7c3c 🎖

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP
...

💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1792263531)
nit in 4ce60d82a0c1e21ee7ecba69e4c8926720829429: Could split the changes to make the format string a bit more const, and never `translated` in real code into a separate commit? The benefit would be that the doc changes and (constroversial?) c_str changes are in a separate commit. It would also allow easier cherry-picking into other pulls.

👍 hebasto approved a pull request: "refactor: include the proper header rather than forward-declaring RemovalReasonToString"
(https://github.com/bitcoin/bitcoin/pull/31058#pullrequestreview-2355137114)
ACK ca2e4ba352cb0a3b5983c002b09ce15ebbf95177, IWYU seems [agree](https://cirrus-ci.com/task/6170839912022016):
```
[16:19:43.440] The full include-list for /ci_container_base/src/validationinterface.cpp:
[16:19:43.440] #include <validationinterface.h>
[16:19:43.440] #include <chain.h> // for CBlockIndex
[16:19:43.440] #include <consensus/validation.h> // for BlockValidationState
[16:19:43.440] #include <kernel/mempool_entry.h> // for RemovedMemp
...

💬 ryanofsky commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400458885)
@dergoegge can you clarify what change is being proposed and how it would affect me if I am building with:

```
BUILD_FOR_FUZZING:BOOL=OFF
BUILD_FUZZ_BINARY:BOOL=ON
```

I build with these options to be able to be able to know if changes not related to fuzzing will break the build.

🤔 tdb3 reviewed a pull request: "Add waitFeesChanged() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31003#pullrequestreview-2349619672)
Approach ACK

Cherry-picked bitcoin-mine from #30437 to exercise some of the interface (on top of 9a723c784ad5170b1e01e91f018e5794a51dd038).

```
git cherry-pick aec7e7d897741fe25fbf54995e1825772e0872d7
git cherry-pick 2227afac0372358287fb879f3b0bd07fd653f3f8

make -C depends NO_QT=1 MULTIPROCESS=1
HOST_PLATFORM="x86_64-pc-linux-gnu"
cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake
```

Added the following to `bitcoin-mine.cpp`
```diff
diff --git a/src/bitcoin-min
...

💬 tdb3 commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1788625591)
> calls getTransactionsUpdated once per second. This returns true anytime a transaction is added to the mempool, even if it has no impact on fees in the best block.

nit: The description in the first commit could be updated to be more accurate (so even if the 2nd commit isn't included, the interface is documented accurately). Maybe just add a note, e.g. `(current interim behavior is to return true for any mempool change, not just fee increased)`.

💬 tdb3 commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1788625072)
non-blocking nitty nit: Could include all function arguments as params in the doxygen comments. Feel free to ignore.