💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#issuecomment-2877805608)
_<ins>Updates</ins>_:
* Addressed @achow101's [feedback](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087371964) by adding an `Assert()` on `CreateWallet()` enforcing only descriptor wallets can be created.

🤔 musaHaruna reviewed a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2837998074)
Concept ACK [5134492](https://github.com/bitcoin/bitcoin/pull/32378/commits/51344920ebe1772d8dd3bece789a6510d774ab59)

Code review: After reading through [internal-interface-guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines), the new code change adheres to interface guideline and correctly abracts away implementation details to the appropraite files. Naming of function is very descriptive which is good.

I compiled each commit su
...

💬 theuni commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877835364)
Hmm, not relevant to what's being addressed here, but couldn't the per-tx `cache->hashPrevouts` and `cache->hashSequence` be replaced with a similar midstate cache trick?

📝 l0rinc opened a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487)
Eliminate one a block header hash calculation per block read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.

This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.

💬 achow101 commented on pull request "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors":
(https://github.com/bitcoin/bitcoin/pull/32475#issuecomment-2877842291)
CI should be fixed

💬 glozow commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2877848547)
reACK 8673e8f0191

🤔 glozow reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2838031475)
reACK 8673e8f0191

🚀 glozow merged a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444)

🚀 glozow merged a pull request: "test: Fix feature_pruning test after nTime typo fix"
(https://github.com/bitcoin/bitcoin/pull/32312)

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-2877875187)
Now ready for review, with #31444 merged.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087588925)
As I understand it it returns different values across restarts (which suffices here) but I may be wrong. The doc is confusing. Anyway, your solution doesn't hurt.

🤔 musaHaruna reviewed a pull request: "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`"
(https://github.com/bitcoin/bitcoin/pull/32432#pullrequestreview-2838067718)
tACK [2fee8f4z](https://github.com/bitcoin/bitcoin/pull/32432/commits/2fee8f4886e1421099dfb8ba959b9d0c7e4b0aa8)

Compiled Successfully without errors
Test the code functionality using:
`build/bin/bitcoin-cli -regtest help getnewaddress `
Result:
`Arguments:
.....
2. address_type (string, optional, default=set by -addresstype) The address type to use. Options are "legacy", "p2sh-segwit", "bech32", "bech32m".`
.....

🤔 glozow reviewed a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386#pullrequestreview-2838070577)
ACK 0750249289c092fc8e2e29669fec73a58b873767, seems sensible

🚀 glozow merged a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386)

💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087601815)
Yeah, I had hoped for the same :(

The problem here (as usual with these annotations) is aliasing.

As a simplified example:
```c++
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
```

Here, clang has no idea that `&control.m_lock.m_control_mutex` == `&control2.m_lock.m_control_mutex`.

The only alternative, really, would be something like:

```diff
diff --git a/src/checkqueue.h b/src/chec
...

🤔 glozow reviewed a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2838089123)
ACK 1e0de7a6ba926487c8a075856b74af2a3a0eb8ef

✅ glozow closed an issue: "Bitcoin is returning higher fees for 36 block window than 2 block window (on testnet)"
(https://github.com/bitcoin/bitcoin/issues/11800)

🚀 glozow merged a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080)

💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087617462)
Done.

💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2877918269)
Updated to address @TheCharlatan's nit.