💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2581238936)
what's the reason for separating this work from `SetTip`, if it's related to it?

💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2581208502)
We're introducing dead code in the first commit without context about where these values are coming from.
What if instead we extract the internal checks from `IsInitialBlockDownload` and slowly migrate that behavior away from there.

Note also that `ActiveTip()` already returns the tip we need.
I'm also not exactly sure why we're calling the current state "cached".
And we're already in `ChainstateManager`, simply referring to "tip" is already unambiguous.

The first commit could lay the groundwo
...

💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2581226462)
should be const and needs some comment (and I'd specialize it to just return the value instead of mutating the state, we can do that in the `SetTip` method instead)
```suggestion
/** Check whether the active chain tip exists, has enough work, and is recent. */
bool IsTipRecent() const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
```

💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2581859069)
no need to mention `chain` here and we can use `std::atomic_bool` instead and should add some description to it

💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2581603770)
shouldn't we guard this method with this being false?

👍 hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3530944965)
ACK f33eafff0f3c2698ee69ccef800bbc70a4eeae86

Provides a REST API useful to external indexers, that shouldn't be a source of egregious log spam (https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564184625).

Measured performance before/after PR change to confirm no slowdown happened when fetching large blocks (URL: http://127.0.0.1:8332/rest/block/0000000000000000000515e202c8ae73c8155fc472422d7593af87aa74f2cf3d.bin).

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2581862320)
nit on my own suggestion if you re-touch :grimacing: - consistent ordering of offset/size:
```diff
- return RESTERR(req, HTTP_NOT_FOUND, strprintf("Bad block part size/offset %d/%d for %s", part_size.value_or(0), part_offset, hashStr));
+ return RESTERR(req, HTTP_NOT_FOUND, strprintf("Bad block part offset/size %d/%d for %s", part_offset, part_size.value_or(0), hashStr));
```

💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2582097283)
Agree the previous checks can be removed. Rationale is "transactions that would be individually accepted may be rejected in a package erroneously" which is addressed more comprehensively by trying individual transactions if they don't need to be grouped with others and calculating precise cluster limits through the staging txgraph.

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3603102740)
[53ef86f](https://github.com/bitcoin/bitcoin/commit/53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e) to [4b47113](https://github.com/bitcoin/bitcoin/commit/4b4711369880369729893ba7baef11ba2a36cf4b): rebased

👍 ryanofsky approved a pull request: "mining: fix -blockreservedweight shadows IPC option"
(https://github.com/bitcoin/bitcoin/pull/33965#pullrequestreview-3530907741)
Code review ACK 1f2f4ab9d3fbdd3071810fa78a17563df3f56f91. This is a pretty clean way of letting the IPC reserved weight value take precedence over the `-blockreservedweight`, and a good first step towards making the IPC value optional in the future.

re: https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3601375713

> For that to work however, we'd also need to change mining.capnp, e.g.:

Yes, sorry, I should have mentioned that. I think it makes sense to add support in the c++ implement
...

💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2581818910)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)

Is there any particular reason to apply DEFAULT_BLOCK_RESERVED_WEIGHT in ApplyArgsManOptions? It would seem better to leave the option unset if the argument is unset, and only apply the default one place instead of two.

Would suggest:

```diff
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -78,9 +78,6 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)

static BlockAssembler::Options
...

💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2581852549)
In commit "mining: fix -blockreservedweight shadows IPC option" (1f2f4ab9d3fbdd3071810fa78a17563df3f56f91)

Might be good to clarify this to say something like "Cap'n Proto IPC clients do not currently have a way of leaving this field unset and will always provide a value."

Otherwise it sounds likes requiring a value is actually intentional. Also documenting an optional field as not optional sounds contradictory and is potentially confusing.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582130115)
Sounds good - reordered in https://github.com/bitcoin/bitcoin/compare/f33eafff0f3c2698ee69ccef800bbc70a4eeae86..6826375e85a690d583cdeae3ff68d8703b1802ca.

💬 jbesraa commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603140030)
> > Can you please provide the logs and setups accessible in a text file and not a discord link I think some people might not have discord.
>
> yeah sorry I was in a hurry for the workshop, indeed pasting Discord links is not ideal
>
> this is what [@jbesraa](https://github.com/jbesraa) said:
>
> > I think handling IBD and other initialization aspects is critical when working in production env.
> > As these two services need to set on the same machine you usually deploy them through the same
...

💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2582181728)
The docs look great. I agree the could be consolidated. Maybe even just one file, since the rules are becoming simpler and more consistent across package and single tx.

The `getmempoolfeeratediagram` RPC is hidden/experimental, so maybe it's fine? We could rename it to "adjusted_weight" to not invalidate 'In the RPCs, "weight" refers to the weight as defined in BIP 141.'

👍 theStack approved a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-3531361196)
re-ACK cb7d5bfe4a59d6034e3a77486cbd423e1a3bfa44
(as per `$ git range-diff 55c6a69f...cb7d5bfe`)

Thanks for also tackling the nits.

💬 ryanofsky commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3603248146)
> > Before, there was a clear separation of which options were controllable by `interfaces/mining.h` and which options were internal and not part of the public interface. But now all options are mixed together in `node/mining.h`.
>
> What would be a good way to organize these things? E.g. having `BlockCreateOptions`, `BlockWaitOptions` and `BlockCheckOptions` defined in `node/types.h` wasn't great either, because Mining interface clients don't care the other structs in that file.

Oh I see.
...

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3603285581)
Rebased with trivial conflicts, ready for re-review.

👍 theStack approved a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3531547322)
re-ACK 4b4711369880369729893ba7baef11ba2a36cf4b

(as per `$ git range-diff 53ef86f0...4b471136`)

🤔 cedwies reviewed a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/33680#pullrequestreview-3531563691)
reACK 14abac1