💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2169541692)
no strong opinion. Generally, nits are left for the author to decide on (take or leave)

💬 sipa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169647676)
Well if you're going to assume `at_size <= INT32_MAX`, why bother changing the type?

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169656553)
Yep, I think I will not :) https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3009102393:
> In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

🤔 furszy reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2963076372)
Because the `scheduler` is also used for the validation signals (see [here](https://github.com/bitcoin/bitcoin/blob/c43cc48aaaaa5c1c9fcb1175b415f7bc33e8537f/src/init.cpp#L1382)), using it for RPC as well seems likely to delay block processing — we only allow up to 10 queued tasks at a time before pausing block processing (see [here](https://github.com/bitcoin/bitcoin/blob/c43cc48aaaaa5c1c9fcb1175b415f7bc33e8537f/src/validation.cpp#L3441)).
So, in other words, adding 10 long-lived timers through
...

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3009418385)
> Whiteboarded in person. Hopefully this explanation is clear and explains it in a more permanent location. If this ends up being correct we can adapt the benchmark to be more realistic.

🤦 yes sorry, I was being dumb. I think I'm still missing why peer 0 is separate from the others? These are the numbers I get:

- announcement limit `A=24,000`
- number of peers `P=125`
- number of unique transactions `N` = A / P = 24,000 / 125 = 192
- total memory limit `M` = 404k * P = 50,500,000wu
-
...

💬 luke-jr commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3009423742)
I don't get it. Watch-only wallets are treated the same as non-watch-only, aren't they?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3009427139)
I think the peer that "triggers" to the LimitOrphan eviction should be one that is not by itself violating per-peer dos limits, otherwise it'll be the first one evicted from, immediately halting the eviction process.

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2169685817)
If `Commit()` fails and (while failing) were to close the file internally as suggested, it should probably throw an exception after that so calling code doesn't try to continue using a closed `AutoFile`.

An alternative would be to have a `AutoFile::CommitAndClose()`-function that always closes regardless of errors and clearly signals that calling code does not expect to continue using the `AutoFile`?

💬 luke-jr commented on pull request "wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members":
(https://github.com/bitcoin/bitcoin/pull/32763#issuecomment-3009454105)
Concept NACK, agree with @ryanofsky's concerns. I don't think there's any benefit to refactoring this either?

⚠️ NowYu opened an issue: "First v"
(https://github.com/bitcoin/bitcoin/issues/32820)
### Motivation

Segrhiij

### Possible solution

_No response_

### Useful Skills

* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...


### Guidance for new contributors

Want to work on this issue?

For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3009463187)
> it'll be the first one evicted from, immediately halting the eviction process

Does this require the "aggressive" approach where we trim a peer until it is within limits? Even if peer0 is one of the 50.5MWu ones, we would only need to delete 1 item to make it no longer the most DoSy, and then we round robin through all 125?

✅ willcl-ark closed an issue: "First v"
(https://github.com/bitcoin/bitcoin/issues/32820)

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3009468920)
Oh, you're right. My thinking was that as soon as the trigger-peer is selected, the bump-over-the-limit transaction announcement would be evicted, causing the global limit to no longer be exceeded. But given that we evict from old to new, that isn't possible.

🤔 mzumsande reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2959189061)
First readthrough of the new implementation, just one suggestion to add a `LimitOrphans()` call (and a few nits).

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169494578)
could also assume / break that `it_ann != m_orphans.get<ByPeer>().end()` like in other places.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2167328218)
nit: missing "from"

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169614249)
now that the oldest announcement is evicted, should the comment
"Note that, if the orphanage reaches capacity, it's possible that we immediately evict the transaction we just added." from `TxDownloadManagerImpl::MempoolRejectedTx` be adjusted - since the announcement we added there would be the newest it shouldn't be evicted right away.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169536671)
nit: could save the score in a local variable so that you don't have to call `GetDosScore()` again in the next line.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690)
Now that we have both a size and an announcement count limit, we should probably also call `LimitOrphans()` when we call `AddAnnouncer()` in `TxDownloadManager::AddTxAnnouncement()`, so that peers can't add thousands of announcements over the allowed limit using existing orphans, without `LimitOrphans()` ever being called.

💬 luke-jr commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3009606593)
Opposite thought from @l0rinc: disable them *only* for CI, but leave them enabled for normal builds (which may be the only time the tests are ever run on the end user machine)