💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185644085)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184944417

> nit: not everyone views the rendered view, we might want to align the source view as well:

Thanks! Updated

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185647984)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185089423

> `bitcoin-wallet` should still be in `bin` as far as I can tell, not `libexec` (nit: + double space):

Good catch! I did mean to write `bitcoin-gui` not `bitcoin-wallet`

📝 HowHsu opened a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878)
In BaseIndex::Sync(), pindex in `Rewind(pindex, pindex_next->pprev)` isn't always equal to m_best_block_index since m_best_block_index is updated every SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could happen. Thus the assert here is wrong.

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3036773787)
I expanded #32876 with `SignOptions`. This reduces e2a151c3dc35997ac0f515feb292fc4543093593 to a very small ... but very ugly commit: casting `BaseSignatureCreator` to `MutableTransactionSignatureCreator` to access its `avoid_script_path` option.

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#issuecomment-3036784505)
Some jobs get confused with this:

```
diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
index 54f3d2d199..db8f223c7a 100644
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -312,7 +312,8 @@ void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
// Script verification errors
std::map<int, bilingual_str> input_errors;

- bool complete = SignTransaction(mtx, keystore, coins, {.sighash_type =
...

⚠️ fanquake opened an issue: "seeds: `seed.testnet.achownodes.xyz` not returning results"
(https://github.com/bitcoin/bitcoin/issues/32879)
```bash
./check-dnsseeds.py
<snip>
* testnet
OK testnet-seed.bitcoin.jonasschnelli.ch (23 results)
OK seed.tbtc.petertodd.net (37 results)
OK testnet-seed.bluematt.me (3 results)
OK seed.testnet.bitcoin.sprovoost.nl (34 results)
FAIL seed.testnet.achownodes.xyz
```

cc @achow101

🤔 stickies-v reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2975363560)
Approach ACK 700819f87b019f854340886dc8d35e66e813b53f

I did a full code review and while the changes are numerous, type-safety or pre-existing usage of `GenTxid::{Txid,Wtxid}` makes many of them trivially obvious to be a refactor, which in my opinion makes this PR reviewable even without in-depth knowledge of the p2p code this PR touches.

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

I think keeping this (see https://github.com/stickies-v/bitcoin/commit/8d62f6eb0c279cda82dcb5fa867ba8ae5d39fa97) in a separate move-only commit is a bit easier for review.

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2178043768)
nit: `TransactionIdentifier` could be interpreted as the unabbreviated `Txid`. Perhaps `TxidOrWtxid` is both more clear and shorter? I realize all naming approaches here are probably going to kinda suck in some way, especially as we already have `GenTxid` as a variant too. Don't want to bikeshed, disregard if you don't think it's an improvement.

💬 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?