💬 fanquake commented on pull request "policy: Remove individual transaction <minrelay restriction":
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2565262574)
LLM: s/includeing/including/

💬 maflcko commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3581633296)
Not sure. Is this caused by removing `-g`? If yes, I am not sure this is something meaningful to support when running with sanitizers.

💬 maflcko commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3581643069)
Going further, we could require `-fno-inline` for ubsan. But if you want to go the route in this pull, might as well just wildcard-suppress `InsecureRandomContext`?

💬 fjahr commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#discussion_r2565291756)
Ah, neat, I am sure I knew `num_addresses` existed. Updated the code to use it now and confirmed the result is the same as before for a diff between two different asmap files.

🚀 fanquake merged a pull request: "interfaces: remove redundant mempool lock in ChainImpl::isInMempool()"
(https://github.com/bitcoin/bitcoin/pull/33946)

🤔 l0rinc reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3502433168)
Thanks for being so responsive and open to suggestions!
Publishing my intermediary review to speed up the response cycle.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564625280)
I think a `try_emplace` is more fitting here to avoid creation in case it already exists - it also allows us to avoid the default-constructed send-status list:
```suggestion
const bool inserted{m_transactions.try_emplace(tx).second};
```
or
```C++
LOCK(m_mutex);
return m_transactions.try_emplace(tx).second;
```
or even
```C++
return WITH_LOCK(m_mutex, return m_transactions.try_emplace(tx).second);
```

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564486363)
nit: I don't think it's fair to call it a list. Resolve without comment if irrelevant.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564656833)
we're storing this redundantly, it should be already available to us via `most_urgent_it->first`

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2557883037)
nit: this non-purity (`NodeClock::now()`) can make testing and benchmarking a bit tougher - consider passing it as a param. Just resolve if you disagree.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564687076)
what should happen when a tx was never confirmed, are we deliberately classifying those as stale?

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564662338)
instead of checking whether `most_urgent` is assigned at every iteration, we could init `most_urgent_it` with the first value and do the loop starting from `auto it{std::next(m_transactions.begin())}.
If it's possible to call this with an empty tx list, we could guard it at the beginning.
```C++
LOCK(m_mutex);

if (m_transactions.empty()) return std::nullopt;

auto most_urgent_it{m_transactions.begin()};
Priority most_urgent_prio{DerivePriority(most_urgent_it->second)};
for (auto it{std
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564673247)
is this still accurate (or it's just meant to be a rough overview?)
```suggestion
* Pick the transaction with the fewest send attempts, and confirmations, and oldest send/confirm times.
```

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564860482)
nit: we could prepare the vector for worst-case:

```suggestion
std::vector<CTransactionRef> stale;
stale.reserve(m_transactions.size());
```

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564719339)
if `this` is smaller than `other`, return `greater`?
hmmm, isn't that inverted? Though https://en.cppreference.com/w/cpp/container/priority_queue.html also uses the largest value having priority, so conceptually it makes sense.
To simplify it slightly, I would either invest it to check for actually use the "greater than" operation:
```C++
if (other_tie > this_tie) {
return std::strong_ordering::greater;
```
or the [CI](https://github.com/l0rinc/bitcoin/actions/runs/19703586332
...

👍 dergoegge approved a pull request: "ubsan: add another suppression for InsecureRandomContext"
(https://github.com/bitcoin/bitcoin/pull/33949#pullrequestreview-3511377420)
ACK d35b1755e0fe7f4760ea76dd2ba054e568bb8395

Don't feel strongly about adding this, also happy to just keep adding this to my own suppressions.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3581723693)
`ade7fcb6a0...12dfb1cb7c`: use `std::ranges::max_element()` instead of manual find, inspired by https://github.com/l0rinc/bitcoin/commit/d42c922e9256625b166ab388ff234ba9bc272233#diff-ad1d5d83183a5facaf06e2b44c42a3e942a261b7506576720588bf6b158e4c3dR37

🤔 janb84 reviewed a pull request: "ci: clear out space on CentOS, depends, gui GHA job"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3511396583)
ACK e07e57368e9fab8ecfc140d44aef7db9b23c7ce0

Clever to run the removal in the background ! I was wondering where the output goes but that goes no-where (A thing to keep in mind for the future when we are debugging this)

We could even go a bit more aggressive (if needed) and delete `/usr/local/.ghcup`(6gb) and [others](https://www.geraldonit.com/mastering-disk-space-on-github-actions-runners-a-deep-dive-into-cleanup-strategies-for-x64-and-arm64-runners/)

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565361540)
This is redundant, no? A tx with the same wtxid must have the same txid.
```suggestion
return a->GetWitnessHash() == b->GetWitnessHash();
```

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565372938)
Maybe, I would still keep both so that we don't have to investigate if this opens up any attack surface...