💬 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

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138571084)
`MaxPeerAnnouncements()` is affected by the number of peers, since it's the global limit / num peers.

However, I just realized this comment is also wrong because `m_peer_orphanage_info` size *can* change during the call. I think it's only possible when the per-peer reservation is below 400k, so we allow adding a tx, but immediately remove it because the peer is using "too much" space. Weird, but possible.

💬 PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2960288134)
> > I cannot run some CIs and tests on GitHub.
>
> Approved, they should be run automatically now.

Seems that I still could not run all ci tests without approval. Can you fix it?

💬 davidgumberg commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2138583239)
Looks good to me

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138610576)
Oh, duh, it's the per-peer announcement reservation, not the global announcement limit. :man_facepalming:.

But I don't think it matters really that this is a constant. We'd be ok with using `global_announcement_limit / total_peers` as per-peer reservation, but we are - as a resource optimization on top - using `global_announcement_limit / peers_with_at_least_one_orphan` instead. Not updating the constant here when the last orphan of a peer disappears means using something in between temporari
...