🤔 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
...

📝 achow101 opened a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721)
`getwalletinfo` result fields `balance`, `immature_balance`, and `unconfirmed_balance`, and the `getunconfirmedbalance` RPC have all been deprecated since 0.19.0. It's been long enough that they should either be removed or undeprecated. The functionality provided by these RPCs is provided by `getbalances`.

💬 bigshiny90 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960344612)
> > I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
> > This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
>
> It's on Core to have sane d
...

💬 glozow commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#issuecomment-2960356836)
> The developer notes could point to the incantation to run all the linters in a container.

Assuming I understand you correctly, that does exist here https://github.com/bitcoin/bitcoin/tree/master/test/lint#test-runner

🚀 glozow merged a pull request: "doc: Remove stale sections in dev notes"
(https://github.com/bitcoin/bitcoin/pull/32572)

🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2910938872)
reviewed through 128ad62cd68038641ac7c3308ceb40c6c84d325e

CI failure seems unrelated

I'm going to think more about how evicting peers completely interacts with the behavior.

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

if this line is hit, the `rit` iterator is never incremented; infinite loop?

alternative formulation to consider for the entire conditional starting at https://github.com/bitcoin/bitcoin/pull/31829/commits/498f1c019197a8e4105490cdc4a0605594ca97d5#diff-e6100361fa0e9e25478f808ca084e5f681d4dddbbee7b3bea0f9d5bcd29db3aaR596
```
for (auto rit = std::make_reverse_iterator(it_upper);
rit != std::make_reverse_iterator(it_lower); ++rit)
{
if
...

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

why signed int?

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

and helps determine the total memory limits based on number of entries

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

was this logic change necessary for this commit?

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

exactly one *announcement* for this wtxid I presume

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138563428)
nit(?): is it more of an AnnouncementMap?

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138586549)
"copying one that exists" I assume means grabbing the CTransactionRef?