💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132906675)
Done

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132906794)
Done

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132906970)
Done

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132907093)
Done

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132907214)
Done

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-2950793651)
Decoupled part of this work inside #32694 - combining it with part of #24230.

💬 luke-jr commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-2950796348)
Any reason not to just pass a parameter to CWallet::Create, just to fix the bug in the simplest manner? Refactoring should ideally be separate from fixing.

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132909169)
Done

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132909275)
Done

💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-2950807203)
> Any reason not to just pass a parameter to CWallet::Create, just to fix the bug in the simplest manner? Refactoring should ideally be separate from fixing.

Because doing the bare minimum to fix a bug that is unreachable in production is how we end up with technical debt.

IMO, this PR is primarily a refactor to split these 2 actions that should not be combined. The fact that it fixes those bugs, which are a result of the combining, is a bonus.

🤔 sipa reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2905651896)
Made it through most of the big commit.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132631041)
In commit "[p2p] improve TxOrphanage DoS limits"

Nitty McNitface: commit title undersells it a bit. Maybe "Overhaul TxOrphanage with smarter limits" or so?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132633811)
In commit "[p2p] improve TxOrphanage DoS limits"

I see in other places `int64_t` is used for expressing usage (like `PeerDosInfo::m_total_usage`). Maybe better to pick one type for all places that involve usage? Or even introduce some type aliases for things like announcementcounts/txcounts/memusages, as that may improve readability too.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132644726)
In commit "[p2p] improve TxOrphanage DoS limits"

Nit: this function documentation may be more appropriate near its declaration inline in `TxOrphanageImpl`, rather than here with its implementation.

(and elsewhere below)

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132880562)
In commit "[p2p] improve TxOrphanage DoS limits"

`IsUnique(it_ann)` here too? (or move it inside `Erase`, see comment there).

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132893009)
In commit "[p2p] improve TxOrphanage DoS limits"

Nit: `static constexpr auto mark_reconsider_modifier = ...`

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132887033)
In commit "[p2p] improve TxOrphanage DoS limits"

Calling `lower_bound` to initialize `it`, and then calling `CountWtxid(wtxid)` which will do the same seems superfluous.

How about:
```c++
auto it = index_by_wtxid.lower_bound(ByWtxidView{wtxid, MIN_PEER});
auto it_end = index_by_wtxid.upper_bound(ByWtxidView{wtxid, MAX_PEER});
const auto num_announcers{std::distance(it, it_end)};
if (num_announcers == 0) continue;
std::advance(it, rng.randrange(num_announcers));
```

This would per
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132899931)
In commit "[p2p] improve TxOrphanage DoS limits"

This loop could also be written as:

```c++
auto it = index_by_wtxid.lower_bound(ByWtxidView{wtxid, MIN_PEER});
auto it_end = index_by_wtxid.upper_bound(ByWtxidView{wtxid, MAX_PEER});
unsigned int num_erased{0};
while (it != it_end) {
Erase<ByWtxid>(it++, num_erased == 0);
++num_erased;
}
```

(also applies to `EraseForPeer`)

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132893986)
In commit "[p2p] improve TxOrphanage DoS limits"

Nit: `static constexpr auto mark_reconsidered_modifier = ...`

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2132879588)
In commit "[p2p] improve TxOrphanage DoS limits"

I think `IsUnique(it)` would work here too, and be more efficient? Or move into `Erase`, see comment there.