💬 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

💬 laanwj commented on pull request "test: Remove non-portable IPv6 test":
(https://github.com/bitcoin/bitcoin/pull/31580#issuecomment-2571265003)
i think this makes sense, relying on a "default scope" is brittle at most. Better to add the interface number explicitly.
We don't use that anywhere in our code outside the test, right? If so i think it's okay to remove this test unconditionally.