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

📝 theStack opened a pull request: "test: check for output to stdout in `TestShell` test"
(https://github.com/bitcoin/bitcoin/pull/33951)
This is a small follow-up PR to the recently added `TestShell` test (#33546), verifying the stdout message "TestShell is already running!" when trying to instantiate a second instance.

💬 m3dwards commented on pull request "ci: clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3582022147)
ACK e07e57368e9fab8ecfc140d44aef7db9b23c7ce0

Like that it runs in the background, dislike that it hides any errors. It's simple enough that I don't think it's worth being more complex to catch any problems.

As an [experiment](https://github.com/m3dwards/bitcoin/commit/129850417f09cd9bbc819794b2626d82c4de39b8) I removed CMakeFiles directories after cmake ran which deletes a few GBs in under a second which was just enough to get my fork building again. I prefer the solution in this PR howeve
...

💬 Eunovo commented on pull request "fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3582057691)
Thanks for the reviews.

@l0rinc I added your test as an automated test in https://github.com/bitcoin/bitcoin/pull/33854/commits/729bf4d627f3bc3414af22990fcf67af2d50d4cf
@hodlinator I added a useful log message in https://github.com/bitcoin/bitcoin/pull/33854/commits/729bf4d627f3bc3414af22990fcf67af2d50d4cf

👍 hodlinator approved a pull request: "contrib: Count entry differences in asmap-tool diff summary"
(https://github.com/bitcoin/bitcoin/pull/33939#pullrequestreview-3511709141)
ACK fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8

Tested by diffing 2 previously recorded[^1] and encoded[^2] Kartograf outputs:

```shell
./contrib/asmap/asmap-tool.py diff 1762865730.dat 1762878275.dat
```

#### master

```
...
# 96634298 (2^26.53) IPv4 addresses changed; 2116571614109896006051667873103669 (2^110.71) IPv6 addresses changed
```

#### fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8

```
...
# Summary
IPv4: 35383 entries with 96634298 (2^26.53) addresses changed
IPv6: 37688 entries with 211
...

💬 hodlinator commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#discussion_r2565613005)
suggestion: Would be nice to output how many entries there are in total, something like
```
Total entries - {args.infile1.name}: {len(state1.to_entries(overlapping=False))}, {args.infile2.name}: {len(state2.to_entries(overlapping=False))}
->
Total entries - 1762865730.dat: 706503, 1762878275.dat: 698844
```
But adding that makes it run 7 seconds slower on top of the ~29 seconds I got otherwise. Maybe there's some other way to get an approximate value.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565643372)
This code is outdated. Anyway, there are still calls `NodeClock::now()`. I think passing the present time as parameter would be too cumbersome and unnecessary at the call sites. There is mock time to use from tests. I prefer to keep the `now()` call internal to the methods. The world outside of the class does not even know it deals with time internally.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2565667203)
Done

💬 brunoerg commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2565667562)
Done

💬 brunoerg commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#issuecomment-3582150488)
Force-pushed 2df9f10755ea7de94ecc9a17e6dd6dc89de0a637..52101d8809062d524393ebfde20c926d36fe5bf8:

- Rebased - and now that we have cluster mempool, I simplified the target by mocking `HasDescendants`.
- Addressed https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2564296509 and https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2564297647