💬 davidgumberg commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2917613764)
crACK 96a341d4064132292621af404de908f01fbe3e2f
`src/node/interfaces.cpp`'s `FillBlock()` is incorrectly annotated, and no warning is emitted, cherry-picking the commits that fix `reverse_lock`'s annotations, the thread safety analyzer complains as it should about `FillBlock()`:
```bash
$ git checkout --detach master && git cherry-pick 0065b9673db5da2994b0b07c1d50ebfb19af39d0^..96a341d4064132292621af404de908f01fbe3e2f
$ CC=clang CXX=clang++ cmake -B build && cmake --build build -j $(nproc
...
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2917613764)
crACK 96a341d4064132292621af404de908f01fbe3e2f
`src/node/interfaces.cpp`'s `FillBlock()` is incorrectly annotated, and no warning is emitted, cherry-picking the commits that fix `reverse_lock`'s annotations, the thread safety analyzer complains as it should about `FillBlock()`:
```bash
$ git checkout --detach master && git cherry-pick 0065b9673db5da2994b0b07c1d50ebfb19af39d0^..96a341d4064132292621af404de908f01fbe3e2f
$ CC=clang CXX=clang++ cmake -B build && cmake --build build -j $(nproc
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112592805)
In commit "[prep/test] modify test to not access TxOrphanage internals"
It seems all invocations of `Random()` assume that the returned value won't be `nullptr` anyway, so I think they can just be replaced with:
```c++
CTransactionRef txPrev = orphans_added[m_rng.randrange(orphans_added.size())];
```
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112592805)
In commit "[prep/test] modify test to not access TxOrphanage internals"
It seems all invocations of `Random()` assume that the returned value won't be `nullptr` anyway, so I think they can just be replaced with:
```c++
CTransactionRef txPrev = orphans_added[m_rng.randrange(orphans_added.size())];
```
🤔 sipa reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2876188083)
Some comments already.
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2876188083)
Some comments already.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112726648)
In commit "feature: Add TxOrphanageImpl"
Many of the includes here seem unused:
```diff
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -5,33 +5,20 @@
#ifndef BITCOIN_NODE_TXORPHANAGE_IMPL_H
#define BITCOIN_NODE_TXORPHANAGE_IMPL_H
-#include <coins.h>
-#include <consensus/amount.h>
-#include <indirectmap.h>
#include <logging.h>
-#include <net.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
-#include <sync.h>
-#include <util/epoc
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112726648)
In commit "feature: Add TxOrphanageImpl"
Many of the includes here seem unused:
```diff
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -5,33 +5,20 @@
#ifndef BITCOIN_NODE_TXORPHANAGE_IMPL_H
#define BITCOIN_NODE_TXORPHANAGE_IMPL_H
-#include <coins.h>
-#include <consensus/amount.h>
-#include <indirectmap.h>
#include <logging.h>
-#include <net.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
-#include <sync.h>
-#include <util/epoc
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112750662)
In commit "feature: Add TxOrphanageImpl"
This has an unstated assumption that `it` is the iterator the first entry in the `ByWtxid` index for a given wtxid.
Given that there is only one call site (`CountWtxid` below), maybe it is better to inline this function there?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112750662)
In commit "feature: Add TxOrphanageImpl"
This has an unstated assumption that `it` is the iterator the first entry in the `ByWtxid` index for a given wtxid.
Given that there is only one call site (`CountWtxid` below), maybe it is better to inline this function there?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112773426)
In commit "feature: Add TxOrphanageImpl"
Nit: maybe `auto& peer_info = reconstructed_peer_info[it->m_announcer];` ?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112773426)
In commit "feature: Add TxOrphanageImpl"
Nit: maybe `auto& peer_info = reconstructed_peer_info[it->m_announcer];` ?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112744241)
In commit "feature: Add TxOrphanageImpl"
`Assume(peer_it != m_peer_orphanage_info.end());` ?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112744241)
In commit "feature: Add TxOrphanageImpl"
`Assume(peer_it != m_peer_orphanage_info.end());` ?
💬 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`.
(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.
(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`.
(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.
(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.
(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
(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`
(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);
+
...
(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?
(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
...
(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
(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
(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
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112547493)
455b4b817884f860cd4467f0a9be4a459e89891c
nit: unsure if "uses" section adds a lot to clarity vs reading the code