💬 maflcko commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960168808)
> all existing tests pass.
no they do not
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960168808)
> all existing tests pass.
no they do not
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960169750)
> 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 defau
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960169750)
> 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 defau
...
✅ Muniru0 closed a pull request: "docs: add guidance on initialism capitalisation in PascalCase identifiers."
(https://github.com/bitcoin/bitcoin/pull/32718)
(https://github.com/bitcoin/bitcoin/pull/32718)
💬 achow101 commented on pull request "ci: Rewrite test-each-commit as py script":
(https://github.com/bitcoin/bitcoin/pull/32680#issuecomment-2960210974)
ACK fa9cfdf3be754b01e6ce73a0cc2f998b81e12970
(https://github.com/bitcoin/bitcoin/pull/32680#issuecomment-2960210974)
ACK fa9cfdf3be754b01e6ce73a0cc2f998b81e12970
💬 Muniru0 commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960212133)
> > all existing tests pass.
>
> no they do not
>
> (There is no point in trying to claim the tests pass. Most tests are run in the CI, so anyone can see for themselves or run them themselves. If there is a test that isn't run in CI, it would be good to mention it. However, this is just a doc change.)
I first push it to my branch and had no conflicts. So that is why I said the test passed before I push them here.
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960212133)
> > all existing tests pass.
>
> no they do not
>
> (There is no point in trying to claim the tests pass. Most tests are run in the CI, so anyone can see for themselves or run them themselves. If there is a test that isn't run in CI, it would be good to mention it. However, this is just a doc change.)
I first push it to my branch and had no conflicts. So that is why I said the test passed before I push them here.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138025794)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Since these types are used in the return types of (the implementation of) `TxOrphanage` interface functions, maybe they belong in `TxOrphanage` directly, so that e.g. both `TxOrphanage::UsageFromPeer` and `TxOrphanageImpl::UsageFromPeer` can use `Usage` as return type?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138025794)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Since these types are used in the return types of (the implementation of) `TxOrphanage` interface functions, maybe they belong in `TxOrphanage` directly, so that e.g. both `TxOrphanage::UsageFromPeer` and `TxOrphanageImpl::UsageFromPeer` can use `Usage` as return type?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138105074)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Nit: maybe point out that it isn't strictly necessary to use 1 as a lower limit, because when the last peer goes below ratio 1, necessarily `NeedsTrim()` will be false, and we'd stop anyway. But it's more convenient to have some fallback number to use anyway.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138105074)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Nit: maybe point out that it isn't strictly necessary to use 1 as a lower limit, because when the last peer goes below ratio 1, necessarily `NeedsTrim()` will be false, and we'd stop anyway. But it's more convenient to have some fallback number to use anyway.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138070999)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
I'm confused by this comment. Neither `MaxPeerAnnouncements()` or `ReservedPeerUsage()` are affected by the size of `m_peer_orphanage_info`, so even if that map's size were to change (which can happen in this function...), nothing would change? In fact, they're both just returning `const` class member variables.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138070999)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
I'm confused by this comment. Neither `MaxPeerAnnouncements()` or `ReservedPeerUsage()` are affected by the size of `m_peer_orphanage_info`, so even if that map's size were to change (which can happen in this function...), nothing would change? In fact, they're both just returning `const` class member variables.
💬 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
...
(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)
...
(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.
(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.
(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.
(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.
(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` ?
(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
(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
(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
(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.
(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:
(https://github.com/bitcoin/bitcoin/pull/32719#issuecomment-2960238196)
Tooltip shown when hovering mouse cursor above bitcoin-qt.exe:
Before:
After:
