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

👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2530058120)
Code review re ACK b35ed41b03578586e380cb73aece14046ec2da93

Sync from genesis was successful (to 877742, `000000000000000000008bac930b7efd54a7ddef0b9ec5b1c0c00b6a6c8e0cfb`)

fyi, conf used:
```
[main]
connect=<sister node on LAN>
checkpoints=0
prune=53000
dbcache=24000
```

⚠️ thebignaught opened an issue: "Fake bitcoin core website at the top of duckduckgo"
(https://github.com/bitcoin/bitcoin/issues/31602)
Hello all,

If you search "bitcoin core" on duckduckgo.com the top search result is a fake bitcoin core website with the URL: btcore . cc

I haven't actually visited the site but I am assuming it is offering malware downloads to the unsuspecting. Is there anything that can be done about this?

I have crossposted this also on the bitcoin-core github on the website repo.

💬 TheCharlatan commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1902837379)
In commit 0dd386994bafd67081c4521673018ddd22e2d63c

Can we call this `ReadBlockUndo` instead?

💬 TheCharlatan commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1902903280)
In commit 3b66b7e20ddba94d251c97818700d46030b14cc5

`s/Spit/Split` in commit message.

💬 TheCharlatan commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2571248802)
Concept ACK