💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163670725)
Having a constructor here will prevent the C.I failure I think, emplace_back will forward the values to constructor which is missing now.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2163681310)
@andrewtoth, right! I will change that next time I retouch, "resolving" this.

@stratospher, thanks! Lets continue the discussion in https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163643282

👍 ismaelsadeeq approved a pull request: "mempool: use `FeeFrac` for ancestor/descendant score comparators"
(https://github.com/bitcoin/bitcoin/pull/32799#pullrequestreview-2953351336)
Code review ACK 922adf66ac7420e21ea171d3586ea84554e5d91b

💬 ismaelsadeeq commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2999938644)
cc @fjahr and @polespinasa, friendly pinged since you take a look initially.

💬 hebasto commented on pull request "depends: Build `qt` package for FreeBSD hosts":
(https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2163703635)
Accepted @vasild's patch.

> Note that `bitcoin-qt` is linked against some dynamic libraries inside `depends/` so it will not work if moved to another machine without those libraries. Is this expected?

No, it is not. This should be fixed in a follow up.

📝 PixelPil0t1 opened a pull request: "Update Docker Build-Push-Action to Latest Stable Version"
(https://github.com/bitcoin/bitcoin/pull/32801)

Updates docker/build-push-action from v5 to v6

References
docker/build-push-action@v6: https://github.com/docker/build-push-action/releases/tag/v6.18.0


This update ensures we're using the latest stable version of the Docker Build and Push Action, which includes performance improvements and bug fixes.

✅ fanquake closed a pull request: "Update Docker Build-Push-Action to Latest Stable Version"
(https://github.com/bitcoin/bitcoin/pull/32801)

🤔 hebasto reviewed a pull request: "build: add root dir to CMAKE_SYSTEM_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2953470821)
Considering the commit history of the NixOS [patch](https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/cm/cmake/001-search-path.diff), it seems it was required for some packages, but perhaps it’s no longer needed.

On the other hand, the CMake documentation [states](https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PREFIX_PATH.html):
> `CMAKE_SYSTEM_PREFIX_PATH` is not intended to be modified by the project; use [`CMAKE_PREFIX_PATH`](https://cmake.org/cmake/help/latest/variable
...

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