💬 instagibbs commented on pull request "test: Fix reorg patterns in mempool tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2135690754)
not "first" node anymore

💬 instagibbs commented on pull request "test: Fix reorg patterns in mempool tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2135693533)
if fork length is always 10 because it's sufficient and easy, just make a constant like `FORK_LENGTH` with a quick comment to describe what it's for

🤔 hodlinator reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2909638652)
Concept ACK b740b6f9479a2351cd3b66a0f6e53848452b6bf8

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2135498545)
nit: Avoid default-initialized `GenTxid`:
```suggestion
auto gtxid{peer.m_wtxid_relay ? GenTxid{wtxid} : GenTxid{txid}};
```

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2135710312)
nit: It's tempting to avoid this change in the final diff through making `unbroadcast_txids` more strongly typed:

<details><summary>Diff</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1246628b75..06b8a5e39f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1578,13 +1578,13 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service

void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler
...

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2135464192)
ea58f45b43fcabef02019c5f8d2b3c79ba3340ca:
The hash could be referring to a wtxid here, and erasing would not work for peers supporting wtxid, making the code buggier at this commit (only to be fixed in later commits)?
Suggestion:
```C++
tx_relay->m_tx_inventory_to_send.erase(peer->m_wtxid_relay ? GenTxidVariant{Wtxid::FromUint256(inv.hash)} : GenTxidVariant{Txid::FromUint256(inv.hash)});
```

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2135549808)
nit in ea58f45b43fcabef02019c5f8d2b3c79ba3340ca:

At first it seems more optimal to keep taking `bool wtxid` as a function parameter, and assume our `GenTxidVariant`s are of the expected type. The current way gives the impression that we may be mixing `Txid` with `Wtxid` for the same peer.

```suggestion
indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(std::get<Wtxid>(hashb)) : mapTx.find(std::get<Txid>(hashb));
```
A strong argument against reverting to that is
...

💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2135717500)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2135515163

In commit "validation: in invalidateblock, calculate m_best_header right away" (9a70883002e1fee76c24810808af4fb43f2c8cf5)

Thanks! It looks like the later RecalculateBestHeader() call is now unnecessary and could repeat the work this new code is doing, which doesn't seem great from perspective of duplicating code. InvalidChainFound also seems to be updating
m_best_invalid and m_best_header together while the new code
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2135787497)
In a general sense, having a generalized logging function is helpful when the loglevel isn't known in advance, as is e.g. the case here: https://github.com/bitcoin/bitcoin/blob/f3bbc746647d1fd23bf5cfe357e32f38c5f6319c/src/httpserver.cpp#L440

It seems like we currently don't really need it in our codebase, but I would keep that discussion for a separate PR because it would lead us astray a bit much here?

💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2955934513)
Only change since my previous review is removing the cast.

re-ACK d4e212e8a69ea118acb6caa1a7efe64a77bdfdd2 🚘

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4b
...

💬 fanquake commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#discussion_r2135811545)
> can be helpful on some systems.

Can we document something more specific; I'm guessing this doesn't close https://github.com/bitcoin/bitcoin/issues/32536?

💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2135820932)
> Maybe I can change `ReadRawBlock(...)` to work with `std::vector<std::byte>`: [7dd0554](https://github.com/bitcoin/bitcoin/commit/7dd0554abf62eab8f17b44d9562630cca265cf73)

Sure, but probably a separate pull request. Also to avoid the new cast in the commit, SpanReader could be extended with another constructor, so that the cast is private/internal.

```cpp
explicit SpanReader(std::span<const std::byte> data) : ...

🤔 brunoerg reviewed a pull request: "wallet: have external signer use PSBT error code EXTERNAL_SIGNER_NOT_FOUND"
(https://github.com/bitcoin/bitcoin/pull/32682#pullrequestreview-2910246695)
Concept ACK

💬 hebasto commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#discussion_r2135828587)
> > can be helpful on some systems.
>
> Can we document something more specific;

What do you have in mind?

> I'm guessing this doesn't close #32536?

Correct, it doesn't. The `Qt6_DIR` cache variable can be set by the user to specify the location of a Qt 6 package that CMake cannot discover for some reason. The user should be able to see this variable in CMake's cache.

💬 maflcko commented on pull request "test: Explain how to reproduce zmq:: upstream race":
(https://github.com/bitcoin/bitcoin/pull/32703#issuecomment-2955974196)
Added a commit that removes intermittent, and presumed fixed BDB suppressions. (explanation in the commit message)

🤔 pablomartin4btc reviewed a pull request: "rpc, doc: update `listdescriptors` RCP help"
(https://github.com/bitcoin/bitcoin/pull/32708#pullrequestreview-2910296859)
ACK b44514b876333a94ae242da8b1e4cee439c2d37e

Since you are there I think there's a dead code for "non-descriptor wallets" as those can't be loaded anymore:

https://github.com/bitcoin/bitcoin/blob/b44514b876333a94ae242da8b1e4cee439c2d37e/src/wallet/rpc/backup.cpp#L496-L498

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2135886621)
> I wonder if maybe instead of adding the new code in this commit it might make more sense to just move the InvalidateBlock call above the CheckBlockIndex call?

You probably meant to suggest moving the `InvalidChainFound` call.

I think that wouldn't be ideal:
We could put it into the loop (before `cs_main` is released) and remove all other changed to InvalidateBlock of this PR, but then we'd loop over the entire block index with each iteration, which I'd like to to avoid for performance r
...

💬 kdmukai commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2956055221)
## Summary
Since this PR now has 444 comments, this is an attempt to help people get up to speed on the discussion thus far and identify any remaining concerns or TODO items.

Note vasild's "Further extensions" section in the PR description. Throughout this discussion he has emphasized keeping this PR as simple as possible, deferring further work to subsequent PRs.


## Q&A highlights
* vasild to 1440000bytes: [Why not relay to an existing Tor peer?](https://github.com/bitcoin/bitcoin/pul
...

🤔 jonasnick reviewed a pull request: "test: update BIP340 test vectors and implementation (variable-length messages)"
(https://github.com/bitcoin/bitcoin/pull/32642#pullrequestreview-2910350191)
utACK b184f5c87c418ea49429e4ce0520c655d458306b

💬 kdmukai commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2956071169)
Concept ACK.

This seems like a significant chain analysis heuristic breaker. While only newly updated nodes would have this private broadcast option, my understanding is that any Tor- or I2P-enabled node will be able to receive these txs and then broadcast them out to all their peers.

So now the broadcaster-is-the-sender heuristic is permanently called into question _even when it's an older node_ that's being spied on. That's pretty neat.