🤔 l0rinc requested changes to a pull request: "Cache m_cached_finished_ibd where SetTip is called."
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-3530040461)
Concept ACK, but we need to restructure it slightly so that it tells a story of what we're extracting, delegating and caching exactly.

I have implemented that in https://github.com/l0rinc/bitcoin/pull/60, please add me as coauthor if you decide to take it.

💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2581119097)
I find the existence of his whole method very hacky, we're testing something that cannot happen in reality so if the test passes or fails after this, it won't increase my confidence in the product.
But if you insist on updating it, we should likely update `JumpOutOfIbd` as well for symmetry.

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