💬 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

💬 ryanofsky commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603399341)
re: https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602538081

> Makes sense. We'll have to find another method of preventing a burst of new templates while the node churns through the last day worth of blocks.

I think the fact that we have separate `createNewBlock` and `waitNext` to functions to fetch the initial block template and subsequent ones should make this easier to solve, because `createNewBlock` can be conservative and wait to return until after some cooldown period, an
...

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582425482)
Since `-blockreservedweight` has a default value, I don't think the absence of this setting should behave any different from when the user manually sets it to the default. This makes that intention clear.

It also makes the transition to `ApplyMiningDefaults` in https://github.com/bitcoin/bitcoin/pull/33966/commits/5f3e67efddd2a8a8743bb19765a5b38755d3f460 simpler, because `default_block_max_weight` and `default_block_reserved_weight` on `MiningArgs` are _not_ optionals. That's for the same rea
...

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582434748)
I took the suggested message.

💬 Sjors commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603530038)
@ryanofsky wrote:

> `GetTime() - last_block_connected_time < COOLDOWN`

@jbesraa:

That looks nice, and would keep client code simple.

> We run both sv2-tp(by sjors, version 1.0.4)

You should upgrade to [v1.0.5](https://github.com/stratum-mining/sv2-tp/releases/tag/v1.0.5) :-)

> and core 30

At this point I think it's better to run the `30.x` branch built from source if you can. It has quite a few IPC related bug fixes. Otherwise you're going to hunt for bugs that have already been fixed. On
...

💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3603546302)
push to 22ec2f95eecddd5dc6ea53f670f232a7e425a8d0:
- fix conflicts with master, which were all logging fixes from https://github.com/bitcoin/bitcoin/pull/33960

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

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582602195)
there's a **lot** happening in a single commit - please consider splitting the low risk changes such as this line from introducing a new feature or a doc change. It would help guide the reviewers if it were split into trivial-to-review chunks where the commit message explains the thinking.

👍 sedited approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531852343)
Re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca