💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581576868)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)

Was it intentional not to include the "note deprecated: use getCoinbase()" comment on this method? Would be good to include if not.

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579000680)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)

Instead of having a warning here it would seem better to document the compatibility concern directly in the code generating the scriptsig. This way, a potentially breaking change would be obvious just looking at this code and not rely on noticing a warning elsewhere. Also it would seem more future-proof for IPC documentation to just say that the final scriptsig needs to be <= 100 bytes, instead of trying to guara
...

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581511244)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514301870

> As a progression towards `getCoinbase` & deprecation, this implementation could internally also do what is suggested in the deprecation node: invoke `ExtractCoinbaseTemplate`, scan outputs for the SegWit marker, and return the index.

I don't see a clean way to get the index from ExtractCoinbaseTemplate, but even if there were, calling `GetWitnessCommitmentIndex` seems like the simplest implementation of this method.
...

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581564930)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533740877

> I added documentation there, but we also need it here. The `output` field is not limited to `OP_RETURN`, that's just a heuristic used by `ExtractCoinbaseTemplate`.

Seem like with latest update, this comment is no longer accurate and should be removed? I guess still I don't understand why it would be important to document the outputs field in this function declaration instead of only in the struct definition, but may
...

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579049804)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075


> The BIP says:
>
> > For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.
>
> So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".

Wow, I see. S
...

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581390462)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533772625

Thanks for spelling out the hypothetical here. The new comments are also nice and make intent more clear.

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579073847)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075

> IMO `witness_reserved_value` would be a more descriptive name than just `witness`.

Other response https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075 why this name would be inconsistent with terminology of BIP141.

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581294471)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177

> That seems incorrect, because a pool (like Ocean or P2pool) might let you take part of the reward directly in an output.

Hmm, it sounds like the idea here is maybe that pools will reuse this `CoinbaseTemplate` struct, and prepend their own outputs into `required_outputs` and subtract their own rewards from `value_remaining` leaving the individual miner to take the rest? I'm not sure how realistic it is to expect poo
...

📝 fanquake opened a pull request: "contrib: fix manpage generation"
(https://github.com/bitcoin/bitcoin/pull/33996)
0972f5504021b482b27523fd3bcb8036cf6b439c from #33229 broke manpage generation, because the assumption that the last word in the line containing the version number, was the version number, no-longer holds for some binaries. i.e `bitcoind`.

💬 fanquake commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3602751889)
Fixed in #33996.

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