💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2087500872)
In commit "test: more template verification tests" (c1939c43c3addb17c4316d49580762a1e0ec4504)

Might be nice to split this test up into different methods since most of the checks seem independent. (Could pass in any objects they are sharing in common as arguments)

💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2087339443)
re: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2035713597

> Making this return a `BlockValidationState` means we have to pass it over the interface, which means we can't drop the special handling

Agree it's not good to pass `BlockValidationState` over the IPC interface (and the earlier change I posted didn't do that). I just think it it's best if validation.h/cpp uses BlockValidationState to be internally consistent and return the most information to callers.

> None of th
...

👍 pinheadmz approved a pull request: "net: replace manual reference counting of CNode with shared_ptr"
(https://github.com/bitcoin/bitcoin/pull/32015#pullrequestreview-2837749970)
ACK 8b93e0894f94883f76b7df5d50a5f1a58544fb6c

Built on macos/arm64, ran all unit and functional tests, reviewed each commit and left a few questions.

Also ran on mainnet for a few hours to watch peers come and go.

This is a great refactor, manually refcounting is bound to be error prone and the patch gives us cleaner code with some performance optimizations. Since `CNode` was mostly handled by raw pointer most of the handling logic didn't have to change to support `std::shared_ptr` inste
...

💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087411896)
4326f97dc6dccc9e558d99931f8235fc6b6af405

Why change this from span to vector?

💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087412742)
4326f97dc6dccc9e558d99931f8235fc6b6af405

Commit message says deletion is unchanged.

💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087433170)
4326f97dc6dccc9e558d99931f8235fc6b6af405

How did this not deadlock before? `FindNode()` called on the next line locks `m_node_mutex`

💬 pinheadmz commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2087458896)
4326f97dc6dccc9e558d99931f8235fc6b6af405

Why did you choose to pass the raw pointer instead of refactoring `ProcessMessages()` to accept the shared_ptr?

💬 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