💬 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)

💬 NowYu commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#issuecomment-3009638607)
https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1876650074

💬 luke-jr commented on pull request "doc: Add fetching single PRs from upstream to productivity.md":
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2169785058)
Why the experimental `git switch` instead of `git checkout`?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2169804538)
I don't think this is confusing because of prior context. I can understand the point about newer contributors. I've come across other places in the codebase where I've had to learn the context by combing through IRC, PRs, commit messages, blogs, etc. I think it's similar to adding the information in the commit message rather than including a comment since a user must search for the commit message info.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3009676579)
See https://github.com/sipa/bitcoin/commits/pr31829:
* Makes a few preparatory behavior changes to make it more simulation-testable (return which announcements are made reconsiderable from `AddChildrenToWorkSet, and tie-break equally-DoSy peers by picking the highest NodeId).
* Add a simulation fuzz test which uses a super dumb vector of (wtxid, nodeid) pairs (in announcement order) to represent the state of the orphanage.
* Adds a `std::set<Wtxid>` of reconsiderable wtxids to prevent having
...

💬 NowYu commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3009713243)
> How does this compare with #32685?

💬 theStack commented on pull request "test: fix catchup loop in outbound eviction functional test":
(https://github.com/bitcoin/bitcoin/pull/32742#issuecomment-3009742101)
@pablomartin4btc:

> not sure if this is an issue but `tip_header.hash` (after this change) will be only used in this file later at [`test_outbound_eviction_blocks_relay_only()`](https://github.com/bitcoin/bitcoin/blob/f27898c8bfe339e2cad09d42e133a05602b1a030/test/functional/p2p_outbound_eviction.py#L235) following the same patter as above, calling `wait_for_getheaders` with `block_hash=tip_header.hash` being `tip_header.hash=None`, should that be fixed too?

Good catch, just verified that t
...