🤔 stickies-v reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2949718938)
I think the changes here can be simplified quite a bit by first replacing `get_iter_from_wtxid` with a `GetIter(const wtxid&)` overload, see my suggestions here and here (or full branch https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-06/overload-getiter).

There are also quite a few clang-tidy violations that would be helpful to address here, left per-commit comments.

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2161772934)
clang-tidy nit:

```suggestion
explicit CompareInvMempoolOrder(CTxMemPool* _mempool)
```

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2163691934)
clang-tidy nit for 5b5abf88de7975babe2264456076f85f1c9f11a0:

<details>
<summary>git diff on a6f1af5f55</summary>

```diff
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index 54c2fb578f..adadb2a3e7 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -438,17 +438,18 @@ public:
friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }
friend bool operator<(
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2161385282)
nit: this docstring imo doesn't add anything useful. Would either remove or beef up (e.g. explaining when (not) to use this).

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2163696012)
clang-tidy nit for da9927139b0f4583c9245aa3b29185612f94cdc7:

<details>
<summary>git diff on 07822f5386</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index df66d4c93d..a0ed1e2620 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2440,8 +2440,8 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
}

CTransactionRef tx{inv.IsMsgWtx() ?
- FindTxForGetDa
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2163699389)
clang-tidy nit in 151ab0e50bf37dae4631ba0b7e6bdccf788cbc11:

<details>
<summary>git diff on 44c6c811ba</summary>

```diff
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index c169c3bf41..054c8fb7dd 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -126,21 +126,21 @@ void TxDownloadManagerImpl::BlockDisconnected()
bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable)
{

...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2163701736)
clang-tidy nit in 6c2ba583d5dde911b4cf32edf5baf5acdb9824fb:

<details>
<summary>git diff on 50c04bce87</summary>

```diff
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index 8db30d21d1..a514ac8143 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -275,12 +275,12 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
auto requestable = m_txrequest.GetRequestable(nodeid, current_time,
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2163728201)
In 3e378fd6594d1811773153e14b9e34cb538ac66e:

These visitors can be cleaned up quite a bit by first replacing `get_iter_from_wtxid` with a new `GetIter(const Wtxid&)` overload (see https://github.com/stickies-v/bitcoin/commit/2585f12836bd5ef599c75e338066e2a13d6e58f7 and https://github.com/stickies-v/bitcoin/commit/589357b193b98d15b6db8db48f521ab193fc0136). This code block then becomes quite a bit more concise. It increases the diff a bit in other places, but part of that is from increased type
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2163747520)
In da9927139b0f4583c9245aa3b29185612f94cdc7:

With my other suggestion to implement `GetIter(const Wtxid&)`, the `info` and `info_for_relay` duplication can be avoided, because they can now be trivially templated (moving them into `txmempool.h`). It does also require moving `GetInfo` to the header (see https://github.com/stickies-v/bitcoin/commit/8d62f6eb0c279cda82dcb5fa867ba8ae5d39fa97), but I think that's okay (ideally that function would just be replaced with a `TxMempoolInfo(txiter)` ctor
...

💬 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_r2163778896)
The choice of a hashing algorithm has many dimensions (distribution, performance, collision resistance, ...). Capturing that rationale in a variable name seems an unrealistic expectation. I'm all for removing dead comments but this seems not the place to me.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163787286)
It's possible that this isn't the place, but it doesn't currently explain performance or collision resistance either - but if those are importance, maybe we could explain them in the commit message instead.

💬 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_r2163788183)
Both are frequently used throughout the codebase. `RATELIMIT_MAX_BYTES` and `DEFAULT_MAX_LOG_BUFFER` are not related to eachother beyond both being used in logging. I don't see any footgun potential between using one or the other, so this seems like an either-is-fine option that can be left to the author to decide, and very quickly becomes bikeshedding otherwise.

💬 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_r2163792560)
This variable is not used or changed in this PR.

💬 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_r2163798705)
The latter. "Ensure tests don't use each other's rate limiting budget" could be an alternative?

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163814997)
This argument may not be, but the test is validating the changed code, dead parameter names don't increase my confidence in the correctness of the PR. It may have an obvious explanation, but it seemed related to me, given that we're touching surrounding 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_r2163818101)
Semantically, I agree. But practically, I think it's fine as is. Conditional vs unconditional are at this point fairly established concepts in our logging code in that they indicate whether or not logging categories need to be enabled or not. Rate limiting should in practice never occur, so for all intents and purposes, it is indeed unconditional.

How would you suggest to improve this? I can't think of an improvement.

💬 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_r2163825735)
That would mean the rate limiting warnings are no longer logged to console and callbacks (`NeedsRateLimiting` has side effects), so I think keeping this as-is is better.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163837664)
I don't consider "established" a relevant argument in this case - it just prevents new contributors from understanding the codebase. But if you think this isn't confusing, I don't mind resolving the comment, I did eventually understand it, just got sidetracked by the contradiction and wanted to simplify it to the next person. If we do correct it, we could be more specific instead of overly generalizing it (ie specify the restrictions instead of claiming there are none).

💬 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_r2163843028)
What do you mean with "dead parameter names"? This variable defines the default log level, which indeed is debug level. If you run `bitcoind -debug=net`, you'll start seeing net debug logs, you don't have to set `-loglevel=debug`.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163843866)
(I'm on a phone now, can't check the source)
Ah, so there's a side-effect here that wasn't obvious to me, thanks for clarifying.