💬 furszy commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1902153333)
Just as a fun fact: this function could be reduced to a single line:
```c++
bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
{
return m_xonly ? arg.GetKeyByXOnly(XOnlyPubKey(m_pubkey), key) :
arg.GetKey(m_pubkey.GetID(), key);
}
```

💬 tdb3 commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902173650)
We repeat this pattern throughout the file.
Unless I'm missing something, seems like a good opportunity to de-duplicate with something like:

```c++
CBlock CreateAndGetBlock(const node::BlockCreateOptions& options)
{
auto mining{MakeMining()};
BOOST_REQUIRE(mining);
std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
BOOST_REQUIRE(block_template);
return block_template->getBlock();
}
```

👍 tdb3 approved a pull request: "test: have miner_tests use Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2529729064)
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

Nice updates. Left a minor non-blocking comment.

💬 laanwj commented on pull request "[28.x] 28.1 backports and final changes":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2569868533)
> Are these significant enough to warrant a rc3 or can we go straight to final?

The only change here that could trigger a new RC is the change in `src/rpc/mining.cpp`, which affects the release binary, but as that's contained to a test-only RPC i don't think it's worth rolling another rc for. The rest are test changes.

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569873452)
Thank you for the review @mzumsande,

Updated f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 -> 182945e946ac6a92daad012d6579e5441b9641cc ([blockmanDB_4](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_4) -> [blockmanDB_5](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_4..blockmanDB_5))

* Addressed @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977), added documentatio
...

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569874763)
Re https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2521245377

> I did a bit of research on how much logic and I/O in constructors are considered to be "good style", and there doesn't really seem to be a clear consensus to me.

I wish there would be better consensus on what the best practices are. If somebody has reasoning for doing things differently, do share!

🤔 sipa reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2529697189)
Concept ACK, some nits/questions:

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902163221)
Since `State::COMPLETED` is the final state, it's possible to move this condition into the loop (`it->m_txhash == txhash && it->GetState() != State::COMPLETED`); hitting COMPLETED means we're done with that hash.

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902181326)
Nit: can use `contains(peer)` instead of `count(peer) > 0`.

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902155109)
Should this attempt to deduplicate? Is it possible that we have both an txid and wtxid announcement from the same peer for the same transaction?

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902153685)
Should this attempt to recurse, and give all ancestors? Not relevant for 1p1c, but in general, in both call sites that seems desirable (an announcement for an orphan should be seen as an announcement for all its ancestors, resolving an orphan means all ancestors can be considered received).

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902202580)
This logic looks very similar to that in `TxDownloadManagerImpl::AddTxAnnouncement`. Is it possible to abstract it out?

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902179413)
I think this can be written to avoid a repeated lookup:

```c++
auto orphan_it = orphan.announcers.find(peer);
if (orphan_it != orphan.announcers.end()) {
orphan.announcers.erase(orphan_it);
if (orphan.announcers.empty()) {
nErased += EraseTx(orphan.tx->GetWitnessHash());
}
}
```

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902194407)
Is it possible that `unique_parents` is empty here? I guess that would mean the orphan can be / should have been processed already? If so, should we call `AddAnnouncer` even?

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902188752)
What if the first peer in the announcer list equals `nodeid`?

EDIT: I see this function is removed in a later commit anyway, so it probably doesn't matter much.

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902201969)
Pass `unique_parents` by reference to avoid a copy for each invocation of `add_orphan_reso_candidate`.

👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2529808309)
Code review ACK b35ed41b03578586e380cb73aece14046ec2da93. Nice changes that make the mining interface more useful and complete, and are cleanly implemented

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1902222969)
In commit "test: check testnet4 difficulty adjustment" (6b504d6cf8d0b3cf14b0df1683d080f505588e6b)

Can there be some comment about how the testnet.4 file is generated and what it contains? Maybe there could be a readme in the test/functional/data/ directory.

It seems ok to have one test like this with a large static data file, but it would be nice in the future to be able to generate block data more dynamically, and having a description of the data could help implement that.

🤔 mzumsande reviewed a pull request: "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors"
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2529778939)
Concept ACK

💬 mzumsande commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1902209558)
did you consider using the existing `hasBlocks()` function instead to adding this function? (I didn't check if it works, but at a glance it looks like it might).

The reason I looked for other functions because I wasn't sure if it's good to expose RPC function such as `GetPruneHeight()` through the chain interface, so that non-RPC code could use it too.