💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301080188)
In 34137b327085b8e1d18f4dbf30f773fb507505dc "wallet, rpc: Add exportwatchonlywallet RPC"

Any particular reason to convert from string to path before passing it to `ExportWatchOnlyWallet`? Asking because I noticed 4 `fs::PathToString(destination)` calls inside `ExportWatchOnlyWallet` that could be avoided if the string is accepted and this conversion is done over there.

💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300928844)
In https://github.com/bitcoin/bitcoin/commit/c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 "test: Test for exportwatchonlywallet"

Nit: don't think this call is necessary here.

```diff
@@ -211,7 +215,6 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
# Spend funds in order to mark addr as previously spent
offline_wallet.sendall([self.funder.getnewaddress()])
self.funder.sendtoaddress(addr, 1)
- self.sync_mempools()
self.generate(self.online
...

💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300951675)
In https://github.com/bitcoin/bitcoin/commit/c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 "test: Test for exportwatchonlywallet"

Nit to make it explicit that why the unspent is marked as reused.
```diff
- assert_equal(offline_wallet.listunspent()[0]["reused"], True)
+ assert_equal(offline_wallet.listunspent(addresses=[addr])[0]["reused"], True)
self.disconnect_nodes(self.offline.index ,self.online.index)

# Export the watchonly wallet file and load onto onli
...

💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300740989)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"

Because there is no actual use of keys and error here in this flow.

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 4bfa11c0bf..95061559c7 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4562,10 +4562,11 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
// Parse the descriptors and add them to the new wallet

...

💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301117939)
Fair point.
I can imagine one of the use cases being that the user creates a offline wallet and immediately exports the watchonly version that's to be imported on a connected node.

✅ tomt1664 closed a pull request: "Bug Fix: invalid address error message and test"
(https://github.com/bitcoin/bitcoin/pull/33257)

💬 cedwies commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301123345)
Nice and clear! One small UX suggestion: since we already know which container the duplicate came from, it could be helpful to mention the categories in the error (e.g. “-bind vs -whitebind”). That said, even without categories, I think this should at least be translatable for consistency with nearby init errors (e.g. `i2psam`). Something like:
```cpp
return InitError(strprintf(
_("Duplicate binding configuration for address %s. "
"Please check your -bind, -whitebind, and onion bindin
...

💬 fanquake commented on pull request "[29.x] backport #33212":
(https://github.com/bitcoin/bitcoin/pull/33251#issuecomment-3224597646)
cc @mzumsande

💬 instagibbs commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3224827005)
Benchmarks now match expectation: each commit separately improves the benchmarks :heavy_check_mark:

💬 brunoerg commented on pull request "fuzz: add target for `DifferenceFormatter` and p2p invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2301545461)
Stiil needs to update the PR title.

📝 fanquake opened a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258)
Use LLVM 21 in the *san & fuzz CIs.

💬 enirox001 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2301586217)
That’s reasonable. Since the test uses ASCII 'a' and the sizes are large, encoding won’t change the outcome in practice.

🤔 enirox001 reviewed a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3156543444)
ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM.

✅ w0xlt closed an issue: "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added"
(https://github.com/bitcoin/bitcoin/issues/33250)

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301652903)
My point was more that the criteria appears to be applied inconsistently.

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301654129)
I don't think the meaning is different enough to warrant a whole new class.

💬 w0xlt commented on issue "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added":
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3225070972)
I haven’t fully reviewed this code yet, but at first glance it seems that if the issue were related to the P2P port, the assert condition in `test/functional/test_framework/util.py` should have failed.

```
def p2p_port(n):
assert n <= MAX_NODES
return PORT_MIN + n + (MAX_NODES * PortSeed.n) % (PORT_RANGE - 1 - MAX_NODES)
```

Anyway, it doesn’t seem like much of a concern, and since the PR doesn’t touch the test further, I’ll close this for now.

💬 fanquake commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3225097225)
Yea I'm not sure. For master, the binaries still have a `libxcb-cursor` dependency, that may need to be installed. It's not clear if #33238 will work for `29.x`, so that will still have a `libxcb-xinerama` dependency, that may need to be installed, (same for `28.x`). There's a note about `libxcb-cursor0` in build-unix, but that isn't shipped with release binaries, or likely read by anyone who isn't compiling from source, so it seems like any of this could benefit from additional documentation.


...

💬 maflcko commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3225106768)
> There's a note about `libxcb-cursor0` in build-unix, but that isn't shipped with release binaries, or likely read by anyone who isn't compiling from source, so it seems like any of this could benefit from additional documentation.

I see, so it seems a bit related to https://github.com/bitcoin/bitcoin/issues/32565

💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2301680659)
Added an assert and comment.