🚀 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...

🤔 janb84 reviewed a pull request: "contrib: Count entry differences in asmap-tool diff summary"
(https://github.com/bitcoin/bitcoin/pull/33939#pullrequestreview-3511446986)
utACK fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8

changes since last ACK:
- changes to use net.num_addresses

(thanks for using my suggestion! )

📝 fanquake opened a pull request: "guix: reduce allowed exported symbols"
(https://github.com/bitcoin/bitcoin/pull/33950)
Need to double-check, but pretty sure this is atl east partly from #33181.

💬 fanquake commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3581825335)
Guix Build (aarch64):
```bash
5e1eb207d68450278951fe45b8c06465beeb49af6a84ef12d037430dcd6519ad guix-build-2984690b20f7/output/aarch64-linux-gnu/SHA256SUMS.part
dd93dd1b964f0629eae5db5106ad1afc307f4317fdecd5f44223aa1942f56348 guix-build-2984690b20f7/output/aarch64-linux-gnu/bitcoin-2984690b20f7-aarch64-linux-gnu-debug.tar.gz
af0c96a7f3f1eb528e21af366d4fd6f585dd5b6620d147822f028bb4c14023f0 guix-build-2984690b20f7/output/aarch64-linux-gnu/bitcoin-2984690b20f7-aarch64-linux-gnu.tar.gz
f3653b
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565434827)
very cool, this also helps with retaining the if-happy-path-else-nullopt scenarios!

Nits: we can likely use `auto&` in the comparison as well and use the `it` inside the if to narrow its scope (and use `emplace_back` to avoid copy and `Now<NodeSeconds>()` for easily comparable picked time)

```suggestion
if (const auto it{std::ranges::max_element(
m_transactions,
[](const auto& a, const auto& b) { return a < b; },
[](const auto& el) { return DerivePriorit
...