💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185480196)
nit in 8b03122ed8f6ef6955532d4499aeb66bdb12c24b: Why add an unused include in a commit that should be mostly move-only ?

💬 fanquake commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#issuecomment-3036726862)
Backported to 29.x in #32863.

💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#issuecomment-3036730579)



> You're correct that the current docstring is incorrect. I personally don't find your suggestion very clear either, as the two cases talk about different parts of the return statement.
>
> An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:
>
> ```c++
> const CBlockIndex* pindex = chain.Next(pindex_prev);
> if (pindex || pindex_prev == chain.Tip()) {
> return pindex;
> }
> ```
>
> git diff on [d6bf3b1
...

🤔 ryanofsky reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2987635116)
Updated 03f585d58ccf4d5c02d621c5b6046d45807b3201 -> 6a6b6f9093c1f82b8ea05ad87f421eabc8849738 ([`pr/libexec.9`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.9) -> [`pr/libexec.10`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.9..pr/libexec.10)) with various suggestions updating signing & CI scripts and documentation

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

> That's because if a user starts with "bitcoin" then "this help message" implies there's nothing additional to see.

To be sure, the "show this help message" does not appear when you just run `bitcoin` with no arguments since I wanted "show this help message" to mean "show this exact help message". Happy to apply any changes, but I think this should already avoid the concern you described.

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

Good catch! Updated these as well

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

Thanks! Updated

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

Thanks! I implemented maflcko's suggestion:

```diff
- DIR_UNIT_TEST_DATA="${DIR_UNIT_TEST_DATA}" LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" "${BASE_OUTDIR}"/libexec/test_bitcoin --catch_system_errors=no -l test_suite
+ DIR_UNIT_TEST_DATA="${DIR_UNIT_TEST_DATA}" LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" "${BASE_BUILD_DIR}"/bin/test_bitcoin --catch_system_errors=no -l test_suite
```

But I am wondering if ma
...

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

> Q: slightly unrelated, but if testing should be available for the installed version, how come fuzzing isn't? Seems it should be mentioned with a 🛠 at least...

The table doesn't mention the fuzz binary just because the table is a list of installed files, and the fuzz binary is not installed before or after this change.

Installing the fuzz binary would seem to make sense for consistency. But one problem with insta
...

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

> nit: if you're already editing this file consider fixing line 86:
>
> > ... double-spends due to _the_ lack of synchronization

Thanks, added commit to fix

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

...