π vasild approved a pull request: "test: refactor out same-txid-diff-wtxid tx to reuse in other tests"
(https://github.com/bitcoin/bitcoin/pull/32385#pullrequestreview-2953228936)
ACK afaaba69eddd50bc22b94ca7c0f9195773aaa111
The code seems to do what is intended and I confirm that it works. I guess it is useful as a generic tool to create a new transaction and two new children of it that have the same txid and different wtxid.
That is useful for the tests of https://github.com/bitcoin/bitcoin/pull/29415 too. However those tests already have a transaction and it would be easier for them and less code, if `malleate_tx()` could create a malleated/changed version of an
...
(https://github.com/bitcoin/bitcoin/pull/32385#pullrequestreview-2953228936)
ACK afaaba69eddd50bc22b94ca7c0f9195773aaa111
The code seems to do what is intended and I confirm that it works. I guess it is useful as a generic tool to create a new transaction and two new children of it that have the same txid and different wtxid.
That is useful for the tests of https://github.com/bitcoin/bitcoin/pull/29415 too. However those tests already have a transaction and it would be easier for them and less code, if `malleate_tx()` could create a malleated/changed version of an
...
π¬ vasild commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163608973)
The name `build_malleated_children` might be confusing since there is nothing "malleated" in those transactions. They are created as such and never [changed or malleated](https://en.bitcoin.it/wiki/Transaction_malleability) afterwards.
Also in the name of `ValidWitnessMalleatedTx`.
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163608973)
The name `build_malleated_children` might be confusing since there is nothing "malleated" in those transactions. They are created as such and never [changed or malleated](https://en.bitcoin.it/wiki/Transaction_malleability) afterwards.
Also in the name of `ValidWitnessMalleatedTx`.
π¬ vasild commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163643282)
This works! Following the hint from https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2160767733 I could integrate it into the tests of https://github.com/bitcoin/bitcoin/pull/29415 to try to send two transactions that have the same txid and different wtxid.
However using it is a bit involved - one has to create a new dedicated parent transaction, get it in the mempool and then broadcast the two children. Also, one has to handle the fees manually. Like this:
```py
self.log.info("S
...
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2163643282)
This works! Following the hint from https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2160767733 I could integrate it into the tests of https://github.com/bitcoin/bitcoin/pull/29415 to try to send two transactions that have the same txid and different wtxid.
However using it is a bit involved - one has to create a new dedicated parent transaction, get it in the mempool and then broadcast the two children. Also, one has to handle the fees manually. Like this:
```py
self.log.info("S
...
π€ ismaelsadeeq reviewed a pull request: "util: detect and warn when using exFAT on MacOS"
(https://github.com/bitcoin/bitcoin/pull/31453#pullrequestreview-2953272636)
Thanks for taking my suggestion @willcl-ark
Your modified string is even better!
Just a couple of comments and then I will re-test again.
(https://github.com/bitcoin/bitcoin/pull/31453#pullrequestreview-2953272636)
Thanks for taking my suggestion @willcl-ark
Your modified string is even better!
Just a couple of comments and then I will re-test again.
π¬ ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163635372)
nit: use `fs::quoted` here and other places, will avoid the slashes?
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163635372)
nit: use `fs::quoted` here and other places, will avoid the slashes?
π¬ ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163641986)
nit: would be nice to be just generic not specific to macOs
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2163641986)
nit: would be nice to be just generic not specific to macOs
π¬ 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.
(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
(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
(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.
(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.
(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.
(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)
(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
...
(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.
(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)
```
(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<(
...
(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).
(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
...
(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)
{
...
(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,
...
(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,
...