💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138376457)
In commit "[p2p] overhaul TxOrphanage with smarter limits"

Use `std::move(this_orphan_announcers)` to avoid a copy of the set.

Also nit: invoking the `OrphanTxBase` constructor negates the advantage of using `emplace_back` (which can construct it in place, but instead it's being constructed by the caller, and then copied in place):

```c++
result.emplace_back(it->m_tx, std::move(this_orphan_announcers));
```

(to be clear, this doesn't matter for performance or anything at all he
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138149816)
In commit "[p2p] overhaul TxOrphanage with smarter limits"

Nit: the `!heap_peer_dos.empty()` is redundant I think, because that would imply there are no peers left (or no peers with score >= 1 left, if above suggestion is followed), so `NeedsTrim()` is definitely false.

This could arguably also be moved above the `if (it_worst_peer != ...)` check:

```c++
if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_ann, max_mem) < dos_threshold)
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138131119)
In commit "[p2p] overhaul TxOrphanage with smarter limits"

Nit: this could skip inserting any peer with DoS score < 1? In the typical (?) case where there are just a few spammy peers, this avoids an $\mathcal{O}(\log n)$ factor in cost per announcement removal in heap maintenance.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138395335)
In commit "[unit test] basic TxOrphanageImpl eviction and protection"

Style nit: not a constant, shouldn't be ALLCAPS.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138395712)
In commit "[unit test] basic TxOrphanageImpl eviction and protection"

Style nit: not a constant, shouldn't be ALLCAPS.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138396884)


In commit "[unit test] basic TxOrphanageImpl eviction and protection"

Style nit: not a (compile-time) constant, shouldn't be ALLCAPS.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138477486)
In commit "[unit test] strengthen GetChildrenFromSamePeer tests: results are in"

Isn't this just `expected == vec_txns` ?

🤔 glozow reviewed a pull request: "doc: Remove stale sections in dev notes"
(https://github.com/bitcoin/bitcoin/pull/32572#pullrequestreview-2914665505)
ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72, all lgtm

💬 glozow commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2138531677)
Marking this resolved, lmk if this is incorrect @davidgumberg

📝 hodlinator opened a pull request: "doc, windows: CompanyName "Bitcoin" => "Bitcoin Core project""
(https://github.com/bitcoin/bitcoin/pull/32719)
Brings Windows executables in line with */share/setup.nsi.in:14* used by the installer.

Discovered while reviewing tangential PR: https://github.com/bitcoin/bitcoin/pull/32634#discussion_r2112641918

💬 achow101 commented on pull request "[28.x] Backport #31407":
(https://github.com/bitcoin/bitcoin/pull/32563#issuecomment-2960237938)
Looks like 3656b828dc2204418974e94928cc8d915b10ed95 wasn't cherry-picked? I think that's necessary for 744b1c8581a88cdb2c3a1f7730b5c7caae86a702 to work.

💬 hodlinator commented on pull request "doc, windows: CompanyName "Bitcoin" => "Bitcoin Core project"":
(https://github.com/bitcoin/bitcoin/pull/32719#issuecomment-2960238196)
Tooltip shown when hovering mouse cursor above bitcoin-qt.exe:

Before:

After:

💬 instagibbs commented on pull request "init, doc: Replace datacarrier(size) deprecation with non-recommendation text":
(https://github.com/bitcoin/bitcoin/pull/32714#issuecomment-2960239724)
concept NACK, seems needless churn to me to backtrack on something a number of original reviewers(and author himself) disagree with

As per @ajtowns if miners end up over-riding the value a lot, we can rethink deprecation. I don't think removing deprecation now makes anything clearer.

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138543837)
Yeah, my bad, also de error description was clear. Just found another case, I'll fix this soon. Thanks!

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138547040)
Sorry, having said that, the error description needs to be updated mentioning that external signer should be enabled instead of descriptor wallet.

📝 Muniru0 opened a pull request: "docs: add guidance on initialism capitalisation in PascalCase identifiers."
(https://github.com/bitcoin/bitcoin/pull/32720)
Fixes #32698

**Problem**
Inconsistent initialization of class,function, method identifiers and structs. Spread throughout the codebase is [screaming / acronym caps](https://gist.github.com/adashrod/66564c690906c9b774e77ddacbd06e1b).

**Solution**
* Add a rule in the developer notes to use consistent camelcase naming convention.
* A class names like `JSONRPCRequest` should be `JsonRpcRequest`.

🤔 glozow reviewed a pull request: "init, doc: Replace datacarrier(size) deprecation with non-recommendation text"
(https://github.com/bitcoin/bitcoin/pull/32714#pullrequestreview-2914680152)
I'm all for explaining more clearly what the status of these config options it. But it seems like we're removing the "vegetarian" label from a menu item because people misinterpreted it and are upset that it contains eggs. We should keep the vegetarian label and just elaborate "this has no meat, but does contain eggs."

Also, I think it'd be fine to leave the option for more than 1-2 release cycles before removal.

💬 glozow commented on pull request "init, doc: Replace datacarrier(size) deprecation with non-recommendation text":
(https://github.com/bitcoin/bitcoin/pull/32714#discussion_r2138541059)
I do prefer "are not recommended to be used" more than "have been marked as deprecated" as it explains it more clearly to the user. However, discouragement *is* deprecation, it's weird to say "after a deprecation warning."

✅ achow101 closed a pull request: "init, doc: Replace datacarrier(size) deprecation with non-recommendation text"
(https://github.com/bitcoin/bitcoin/pull/32714)

👍 TheCharlatan approved a pull request: "index: move disk read lookups to base class"
(https://github.com/bitcoin/bitcoin/pull/32694#pullrequestreview-2914713303)
ACK 4c5ec3a70a47798d8451da65c9c0059955cde937