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

🤔 NowYu reviewed a pull request: "Add read-only mode to sqlite db and use in `bitcoin-wallet`"
(https://github.com/bitcoin/bitcoin/pull/32818#pullrequestreview-2963387333)
Before I get off

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

Ah, I didn't know (or had perhaps forgotten) about that PR. I came via #15608 and did not check what was open. Shame on me :(

I will mark this as draft and take a look over that PR.

📝 willcl-ark converted_to_draft a pull request: "Add read-only mode to sqlite db and use in `bitcoin-wallet`"
(https://github.com/bitcoin/bitcoin/pull/32818)
Currently our `wallet-tool` performs a few "read-only" operations (`info`, `dump`) on sqlite databases. However, currently this involves opening the wallet in RW mode, with the side-effects of modifying the wallet file, and failing to open a "read-only" wallet file.

See #15608 for more context.

Since we have dropped the BDB wallet, this change got a lot simpler; the BDB parser is now custom and only operates in read-only mode anyway.

This change adds a `read_only` bool to `DatabaseOptio
...

💬 luke-jr commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3009784226)
Concept NACK. Stop trying to enable spam.

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169883496)
Is exposing LimitOrphans even worth it vs calling it internally when we add a tx/announcement?

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169961484)
Calling it just internally makes sense to me.