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

💬 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_r1902211186)
similar here: could you use the existing `hasAssumedValidChain`?

💬 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_r1902202946)
this went into the wrong commit (2nd instead of 1st)

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2569993729)
> Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don't read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees)

Fair enough.

I've pushed an update [diff](https://github.com/bitcoin/bitcoin/compare/f7a2f125c7b23c4c3c2e5253a6d3e2dcd8d3fe97..cd48e2ccaf24c9594f5987a29e02b924573134ed):

1. Set the default to 8000, based on [this suggestion](https://github.com/bitcoin/bitcoin/pull/31384#issuec
...

💬 achow101 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#issuecomment-2570023831)
> q: why not store the id belonging to the pubkey with the correct parity bit inside the `SigningProvider` arg at the point where such a structure is loaded? (which most likely happens inside `ParsePubkeyInner()`).

The pubkey in the descriptor has no parity bit information at all, and parsing is context independent.

💬 i-am-yuvi commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2570079166)
Tested ACK f1b40b4de460173e927ebe93cdb5f4ad2ac02859

I tried testing using `./src/bitcoind -signetchallenge=512102ee856c56a5aaadd1656f849bafa4c9dacc86a2878fe546c6189185f842ae2c1851ae`