📝 tomt1664 opened a pull request: "Bug Fix: invalid address error message and test"
(https://github.com/bitcoin/bitcoin/pull/33257)
Error message returned for an invalid HRP string for a correctly encoded bech32 addresses was incorrect. It was returning `'Not a valid Bech32 or Base58 encoding'.` but it should return "Invalid or unsupported prefix for Segwit (Bech32) address (expected ..., got ...)."
The check in the `DecodeDestination` function that returns `"Invalid or unsupported prefix for Segwit (Bech32) address (expected ..., got ...)."` was not reachable for any address, as the HRP was checked at the start of the `D
...
(https://github.com/bitcoin/bitcoin/pull/33257)
Error message returned for an invalid HRP string for a correctly encoded bech32 addresses was incorrect. It was returning `'Not a valid Bech32 or Base58 encoding'.` but it should return "Invalid or unsupported prefix for Segwit (Bech32) address (expected ..., got ...)."
The check in the `DecodeDestination` function that returns `"Invalid or unsupported prefix for Segwit (Bech32) address (expected ..., got ...)."` was not reachable for any address, as the HRP was checked at the start of the `D
...
💬 ryanofsky commented on issue "ci: tidy job is producing output":
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3224259026)
This warning "warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]" is a clang-analzyer false positive that we'd seen before and is fixed in
https://github.com/capnproto/capnproto/pull/2334/commits/4c011373fe209a8b045b5aca0de043de271ad931 (https://github.com/capnproto/capnproto/pull/2334) upstream.
As described in that commit message it's possible to turn off that specific warning globally but not really to suppress it locally with compiler op
...
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3224259026)
This warning "warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]" is a clang-analzyer false positive that we'd seen before and is fixed in
https://github.com/capnproto/capnproto/pull/2334/commits/4c011373fe209a8b045b5aca0de043de271ad931 (https://github.com/capnproto/capnproto/pull/2334) upstream.
As described in that commit message it's possible to turn off that specific warning globally but not really to suppress it locally with compiler op
...
🤔 rkrux reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3155276288)
Code review c9eb6c853dbc96f47c89ab5ca26dde028cd5e108
Looking good, mentioned few points.
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3155276288)
Code review c9eb6c853dbc96f47c89ab5ca26dde028cd5e108
Looking good, mentioned few points.
💬 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_r2300744906)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Nit:
s/watchonly_wallet/watchonly wallet
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300744906)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Nit:
s/watchonly_wallet/watchonly 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_r2300812988)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Nit:
```diff
// Add to the watchonly wallet
- if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
+ if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label*/ "", /*internal*/ false); !spkm_res) {
```
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300812988)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Nit:
```diff
// Add to the watchonly wallet
- if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
+ if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label*/ "", /*internal*/ false); !spkm_res) {
```
💬 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_r2300852509)
In c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 "test: Test for exportwatchonlywallet"
Otherwise assertion for online wallet is missed.
```diff
--- a/test/functional/wallet_exported_watchonly.py
+++ b/test/functional/wallet_exported_watchonly.py
@@ -104,7 +104,7 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
label = f"addrbook_{purpose}"
assert_equal(wallet.listlabels(purpose), [label])
addr = send_addr if purpose == "send" else r
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300852509)
In c9eb6c853dbc96f47c89ab5ca26dde028cd5e108 "test: Test for exportwatchonlywallet"
Otherwise assertion for online wallet is missed.
```diff
--- a/test/functional/wallet_exported_watchonly.py
+++ b/test/functional/wallet_exported_watchonly.py
@@ -104,7 +104,7 @@ class WalletExportedWatchOnly(BitcoinTestFramework):
label = f"addrbook_{purpose}"
assert_equal(wallet.listlabels(purpose), [label])
addr = send_addr if purpose == "send" else r
...
💬 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_r2300814679)
In https://github.com/bitcoin/bitcoin/commit/3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
All the inactive descriptors are hardcoded to not being internal, is this correct? I can't seem to come up with a test case that fails this assumption.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300814679)
In https://github.com/bitcoin/bitcoin/commit/3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
All the inactive descriptors are hardcoded to not being internal, is this correct? I can't seem to come up with a test case that fails this assumption.
💬 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_r2301113117)
In https://github.com/bitcoin/bitcoin/commit/3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Logs while exporting:
```bash
2025-08-26T13:55:18Z [test_watchonly] Setting spkMan to active: id = c21a5fab36193920bcc2f13558b436ac4b1866c56a97bf03bb4c128610279a21, type = bech32m, internal = true
```
Perhaps we can also add a `temp` in the name to emphasise in the logs that this is a temporary wallet.
Maybe `<name>_watchonly_temp`?
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301113117)
In https://github.com/bitcoin/bitcoin/commit/3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Logs while exporting:
```bash
2025-08-26T13:55:18Z [test_watchonly] Setting spkMan to active: id = c21a5fab36193920bcc2f13558b436ac4b1866c56a97bf03bb4c128610279a21, type = bech32m, internal = true
```
Perhaps we can also add a `temp` in the name to emphasise in the logs that this is a temporary wallet.
Maybe `<name>_watchonly_temp`?
💬 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.
(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
...
(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
...
(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
...
(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.
(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)
(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
...
(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
(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:
(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.
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3156543444)
ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM.