💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112731246)
In commit "feature: Add TxOrphanageImpl"

Would it be possible to use `std::map<COutPoint, std::set<Iter>>` here as type? That would be faster (avoiding a lookup to resolve `Wtxid` -> `Iter`) and use less memory. `boost::multi_index` iterator objects are stable (remain valid as long as the object they point to exists), unlike `std::unordered_map`.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112783352)
In commit "feature: Add TxOrphanageImpl"

This could be avoided by checking the `ret.first` result of `auto ret = m_orphans.get<ByWtxid>().emplace(tx, peer, m_current_sequence);` below. The `ByWtxid` index is unique, so emplacement will fail if the same the wtxid/peer combination already exist, I think.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112772043)
In commit "feature: Add TxOrphanageImpl"

Nit: ` ` after `int64_t`.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112777005)
In commit "feature: Add TxOrphanageImpl"

Style: `MIN_PEER`? It wasn't immediately clear when reading the code that this referred to a constant.

💬 achow101 commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2112803011)
In 6d9bf36b2c061088abb0d839fee90683ff1525e1 "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"

Instead of having this output parameter, perhaps we could check if `local_wallet` has any SPKMs after `DoMigration`e.g. `if (local_wallet->GetAllScriptPubKeyMans().empty())`. These should be equivalent checks.

🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2871976352)
reviewed through 455b4b817884f860cd4467f0a9be4a459e89891c

apologies if later commits clear up my confusion

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112259957)
f2dcdbf700b3b20a315f5a6eec57c7463955fe43

if this goes away later anyways, ignore, but `CheckInvariants` could just directly use `node::DEFAULT_MAX_ORPHAN_TRANSACTIONS`

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2109879316)
455b4b817884f860cd4467f0a9be4a459e89891c

Take or leave suggestion to reduce churn in the heap, did no benchmarking but may reduce work for most DoSy peers.
```
diff --git a/src/node/txorphanage_impl.h b/src/node/txorphanage_impl.h
index 89974ec506..6fe7422054 100644
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -623,20 +623,27 @@ public:
heap_peer_dos.pop_back();

+ auto it_worst_peer = m_peer_orphanage_info.find(worst_peer);
+

...

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112159931)
aed51fe7d5cbcc43eba2be3cd5af666fe1d95dd7

give motivation in commit message for the change?

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2109790109)
extra fn doesn't seem worth it
```
diff --git a/src/node/txorphanage_impl.h b/src/node/txorphanage_impl.h
index 89974ec506..b6137845b4 100644
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -205,11 +205,11 @@ class TxOrphanageImpl
}

- /** Return number of announcements with the same wtxid as it. */
- unsigned int CountSameWtxid(Iter<ByWtxid> it) const
+ /** Return number of announcements with this wtxid. */
+ unsigned int CountWtxid(const Wtx
...

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112585455)
PeerMap is unused circa 455b4b817884f860cd4467f0a9be4a459e89891c

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112167760)
737c5127df841e9c8037b1885284f80b0aba17dd

might be good to note in commit message the getorphantxs is experimental, so breaking is considered ok

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112547493)
455b4b817884f860cd4467f0a9be4a459e89891c

nit: unsure if "uses" section adds a lot to clarity vs reading the code

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112572392)
not sure what result_type is doing here or in ByPeerViewExtractor

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112597837)
nit: s/PeerInfo/PeerDoSInfo/

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2112813722)
In 0341e021fe6edfa21252f4bd93b9e1a209383891 "Implement rescan stop with timestamp as never for import descriptors"

Please use `snake_case` for new variables. Additionally, when the type is not annoying to type out, please give the full type rather than use `auto`.

Same for `requestTimestamp` below.

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2112814780)
In 0341e021fe6edfa21252f4bd93b9e1a209383891 "Implement rescan stop with timestamp as never for import descriptors"

nit: space between `now` and `:`

```suggestion
const int64_t timestamp = requestTimestamp < 0 ? now : requestTimestamp;
```

💬 achow101 commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2917732757)
fe0432b1c4a10b74844c2dedefccfe340c0d3b10

💬 achow101 commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2112856923)
```suggestion
"Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only.\n"
```

💬 achow101 commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2112856236)
Comma should be a period.

```suggestion
"if the transaction has unconfirmed inputs. This is because the wallet will attempt to make the\n"
```