💬 Sjors commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3070380062)
Sorry, #32948 probably caused some conflicts.

💬 marcofleon commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205405038)
nit: I believe this should be DoS score > 1.

🤔 marcofleon reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3017110217)
Code review ACK 790f6e7a72d31988c5be9f2d520a04d0c0edfa09

Focused mainly on the orphanage reimplementation d5097d6eef85eab971352cf78fb94a30c6e6f127 and fuzzing. Fuzzed the three txorphan targets for ~1000 cpu hours each. A couple comments I had initially are addressed in the followup.

💬 marcofleon commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205431437)
Thinking out loud, it seems like using `m_outpoint_to_orphan_it` for lookups here instead of going through all orphans could be better performance-wise (if there's a lot of unrelated orphans from this peer). Ultimately, I think this implementation is fine, as it's simpler and automatically returns the children sorted from newest to oldest.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2205459748)
Agree, see https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166251339

💬 marcofleon commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205468802)
Do you mean if we end up constructing txs with more than 9 inputs in the sim target? It seems to me that `txorphan` is more of a stress test than the simulation target, which is a differential test, no?

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205473289)
> It seems to me that txorphan is more of a stress test than the simulation target, which is a differential test, no?

fair enough

💬 adamjonas commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-3070432431)
Any chance this was reproduced on a newer version? 26 hit EOL on 2025-04-14.

💬 rkrux commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3070441050)
Re: [Merge branch 'master' into wallet-readonly-access](https://github.com/bitcoin/bitcoin/pull/32685/commits/fa0699eaffee914aded42fce21beffdebf41b1b6)

I don't think merging master in the PR branch is preferred, instead rebasing is done.
https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

🤔 Crypt-iQ reviewed a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3017290197)
I tracked down the `m_best_header` non-determinism I [posted above](https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-2950384616) to `RecalculateBestHeader`. It seems that since `m_block_index` is a `std::unordered_map`, the iteration order in `RecalculateBestHeader` is not guaranteed.

I narrowed down my corpus to two files that, when run in separate processes (the first step of the `deterministic-fuzz-coverage` script), are deterministic. When they are run in the same process (
...

💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2205513966)
`blocks` is reset each iteration, so can this be removed?

👋 waketraindev's pull request is ready for review: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972)

✅ glozow closed a pull request: "chore: rename misspelling variable"
(https://github.com/bitcoin/bitcoin/pull/32971)

💬 glozow commented on pull request "chore: rename misspelling variable":
(https://github.com/bitcoin/bitcoin/pull/32971#issuecomment-3070566405)
Thanks for your interest in contributing! However, as we have hundreds of pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).

🤔 glozow reviewed a pull request: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972#pullrequestreview-3017372888)
Is anything here not already provided by getpeerinfo?

💬 waketraindev commented on pull request "rpc,net: Add getpeerbyid and listpeersbyids RPCs":
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3070587105)
> Is anything here not already provided by getpeerinfo?

yes, the option to retrieve result by peer id.

🤔 theStack reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3017348305)
Left some nits I found over the last days, mostly about naming. I think it would be worth it to update the commit message of main overhaul commit d5097d6eef85eab971352cf78fb94a30c6e6f127 to use the new "latency score" terminology. Planning to look more at functional and fuzz tests soon.

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205553042)
:detective:
```suggestion
// Delete this tx, clearing the orphanage.
```

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205543956)
nitty nit: for consistency, this should probably be done after every successful `AddTx` call (e.g. also in the next loop below), though it doesn't seem to be necessary for now

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205568179)
in d5097d6eef85eab971352cf78fb94a30c6e6f127: nit: could use a structured binding for getting rid of many `.first/.second` accesses (that I often find confusing) and enhancing the readability, e.g.:
```suggestion
auto [new_announcement_it, inserted] = m_orphans.get<ByWtxid>().emplace(tx, peer, m_current_sequence);
```
(here and in many other return values of `.emplace` calls)

💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613)
in d5097d6eef85eab971352cf78fb94a30c6e6f127: nit: the naming seems outdated, considering that (in contrast to master) the map's value type is not an iterator anymore?
```suggestion
std::unordered_map<COutPoint, std::set<Wtxid>, SaltedOutpointHasher> m_outpoint_to_orphan_wtxids;
```