🤔 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"
```

🤔 w0xlt reviewed a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2876603197)
ACK https://github.com/bitcoin/bitcoin/pull/32449/commits/86e1111239cdb39dd32cfb5178653c608fa30515

nit: Perhaps the comment in `HandleWalletError` could also explain why `DatabaseStatus::FAILED_LEGACY_DISABLED` is also being mapped to `RPC_WALLET_NOT_FOUND`. While it seems clear from looking at this PR, it might be a useful for future reference.

⚠️ harryvik990 opened an issue: "My phone f"
(https://github.com/bitcoin/bitcoin/issues/32635)

✅ fanquake closed an issue: "My phone f"
(https://github.com/bitcoin/bitcoin/issues/32635)

💬 achow101 commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#issuecomment-2917784928)
ACK f6517df210f5e940d87823c86358976743de2641

💬 achow101 commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2917799485)
> OP_RETURN fallback if no address or descriptor set is provided.

Why? I think it would be simpler to just require that at least one output address/descriptor is specified.