💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2179725939)
nit on 472849c747: `isInMempool` can be changed to take a `Txid` without any further code changes and avoids the conversion and having to touch it again in the future.

<details>
<summary>git diff on 472849c747</summary>

```diff
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index f6e831624d..511ba41568 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -208,7 +208,7 @@ public:
virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;

...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2177557370)
nit in e5a1ccd5d6e83d1027e3d2516ffd36d0e88291c4: this ctor can be simplified with member initialization lists. And while these lines are changed anyway, it might be good to add some constness guarantees and modernize naming.

Complete diff would be:

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

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7a97071052..f390b0f944 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5442,18 +5442,15
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2179710076)
in 472849c747334f52cb87bf13b9ee6d23e24d3494: this looks like it still just should be using `ToGenTxid`? Using the visitor pattern here makes things quite verbose wrt thread safety annotations, but then that made me realize - perhaps `FindTxForGetData` should just take an `inv`? It's a net_processing function, it seems like an intuitive fit and avoids duplication.

<details>
<summary>git diff on 472849c747</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
inde
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2185661544)
nit in db125fed333a74999a63ac0e8d8b038cab96a3f4: multiline statements require braces as per developer notes, so would revert this change (or add braces)

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

could consider modernizing with structured bindings while touching this:

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

```diff
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index 7116c433be..32f3ae7931 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -273,9 +273,9 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
std::
...

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

I don't really see the point of using `util::Overloaded` here when we're always doing a `Wtxid` query?

This looks more straightforward to me:

<details>
<summary>git diff on 7c4eb58fb0</summary>

```diff
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index a16490ceb9..87a47b30ee 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -9,7 +9,6 @@
#include <consensus/validation.h>
#include <logging.h>
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2185683937)
These changes seem unnecessary and unrelated to any code changes made in this PR?

💬 theStack commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-3036801220)
Rebased on master (due to conflict with #29307, commit a69c4098b273b6db5d2212ba91cfc713c1634c5d).

@yancyribbens: Sorry, I must have missed your comments back then.

> Interesting - I read the link above "name pipe" and it seems the main benefit to a named pipes is just that for performance, one can skip writing to disk and the churn of creating a new file. I'm not sure I see the perf benefit here in a named pipe though..

Yes, feeding the data directly into the consuming process is obviou
...

💬 achow101 commented on issue "seeds: `seed.testnet.achownodes.xyz` not returning results":
(https://github.com/bitcoin/bitcoin/issues/32879#issuecomment-3037091569)
Oops, DNS migration got a couple IPs mixed up. Should be resolved once the new records propagate.

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3037115130)
Tested ACK https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185072353

Two remaining nits:
* PR description https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185072353
* Comment alignment if you edit again https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184813932

💬 1440000bytes commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3037209014)
Can you write a test that triggers this assert on master branch?

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2186732350)
I think the current `Commit()` is good as it is. It does what it promises - flushes the file data (and checks that the flush succeeded). However a subsequent close is not guaranteed to succeed if commit succeeded before that because close does more than flush.

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3038295873)
> Can you write a test that triggers this assert on master branch?

Sure, I'll do that.

💬 b-l-u-e commented on issue "Internal bug detected: FinalizeAndExtractPSBT(psbtx_copy, mtx)":
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3038398123)
Hi @ekrembal,

I've analyzed this issue. The "Internal bug detected" error occurs because `descriptorprocesspsbt` uses `PSBTInputSigned()` which only checks for signature **presence**, not **validity**.

### Root Cause
In `src/rpc/rawtransaction.cpp` around lines 2092-2094:
```cpp
bool complete = true;
for (const auto& input : psbtx.inputs) {
complete &= PSBTInputSigned(input); // ❌ Only checks presence, not validity
}
```

When `complete` is `true`, the code calls `CHECK_NONFATAL(FinalizeA
...

📝 maflcko opened a pull request: "ci: Avoid cd into build dir"
(https://github.com/bitcoin/bitcoin/pull/32880)
Changing into the build dir is confusing and brittle, because the following commands implicitly assume it. So they could break on unrelated changes.

The changes are required for stuff like:

* cmake presets (see https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344031208)
* meta ci tests (like https://github.com/bitcoin/bitcoin/pull/32874)

So remove the `cd` and just make the build dir explicit.

💬 ismaelsadeeq commented on pull request "fees: prevent redundant estimates flushes":
(https://github.com/bitcoin/bitcoin/pull/32748#discussion_r2187355851)
Taken, also please add an explanation.

I also note a behavior change introduced by this. It is possible that we may track some unconfirmed transactions and, in rare cases, see some unconfirmed transactions get evicted. However, since we have no confirmations yet, if we shut down in this state, we will lose that data and have to start from scratch. To mitigate this, we need a way to detect when we are in such a state.

I see two possible approaches:

1. Brute-force check : Go through all t
...

📝 maflcko opened a pull request: "Turn rpcauth.py test into functional test"
(https://github.com/bitcoin/bitcoin/pull/32881)
Currently the `rpcauth-test.py` is problematic, because:

* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. Specifically `ConfigParser`.
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476.
* Outside of ctest, this single test has to be run manually and separately, which is easy to forget.
* If the test is manually called, it runs single threaded, when it could just run
...

💬 maflcko commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2187385272)
> the correct fix would be to just remove the cmake stuff here and move this test to the python test runner, so that it is run like all other python unit/util/functional tests.

Fixed in https://github.com/bitcoin/bitcoin/pull/32881



> (Actually, while testing, a missing python is now a hard error already when `-DBUILD_GUI=ON`. So I don't think the wording "refactor" accurately represents the changes here.)

To clarify, either the changes here in this pull need to be reverted/fixed up,
...

📝 HowHsu opened a pull request: "index: remove unnecessary locater cleaning in BaseIndex::Init()"
(https://github.com/bitcoin/bitcoin/pull/32882)
BaseIndex::DB::ReadBestBlock() already cleans locater in failure path, remove duplicate code in the caller and change ReadBestBlock to return void.

✅ tdb3 closed a pull request: "rpc: add `relevant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713)