💬 kevkevinpal commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2300759162)
Just an fyi, this test passes even without the changes in `src/policy/fees.h` I also agree with @polespinasa that if another test already covers this, then I'd rather not have redundant tests added
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2300759162)
Just an fyi, this test passes even without the changes in `src/policy/fees.h` I also agree with @polespinasa that if another test already covers this, then I'd rather not have redundant tests added
💬 kevkevinpal commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2300768342)
I am not sure if we need a release note, since according to this note, it will invalidate old estimates files? Maybe someone else can chime in on this
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2300768342)
I am not sure if we need a release note, since according to this note, it will invalidate old estimates files? Maybe someone else can chime in on this
💬 kevkevinpal commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3223886285)
Concept ACK [413c81b](https://github.com/bitcoin/bitcoin/pull/33199/commits/413c81b134a16403cc0f64e21c3c0967c211b318)
It makes sense to update `MIN_BUCKET_FEERATE` with the recent changes to be sub 1 sat/vbyte
I do think the additional test is a bit redundant but I am okay in adding if others think it makes sense
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3223886285)
Concept ACK [413c81b](https://github.com/bitcoin/bitcoin/pull/33199/commits/413c81b134a16403cc0f64e21c3c0967c211b318)
It makes sense to update `MIN_BUCKET_FEERATE` with the recent changes to be sub 1 sat/vbyte
I do think the additional test is a bit redundant but I am okay in adding if others think it makes sense
💬 151henry151 commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#discussion_r2300772336)
Thanks for the feedback, I must have misunderstood the TODO. I thought this would be an improvement for the reasons outlined in the pull request, and thought it would address the TODO item.
(https://github.com/bitcoin/bitcoin/pull/33254#discussion_r2300772336)
Thanks for the feedback, I must have misunderstood the TODO. I thought this would be an improvement for the reasons outlined in the pull request, and thought it would address the TODO item.
💬 151henry151 commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#issuecomment-3223930711)
> Thanks, but the value of purely LLM generated "vibe" pull requests is limited, because:
>
> * The "author" does not understand the changes and can not reply to code review feedback.
> * There are more than 300 pull requests (most written by real humans) waiting for review.
> * Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.
>
> M
...
(https://github.com/bitcoin/bitcoin/pull/33254#issuecomment-3223930711)
> Thanks, but the value of purely LLM generated "vibe" pull requests is limited, because:
>
> * The "author" does not understand the changes and can not reply to code review feedback.
> * There are more than 300 pull requests (most written by real humans) waiting for review.
> * Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.
>
> M
...
💬 cedwies commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3224114903)
ACK fa96a4a
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3224114903)
ACK fa96a4a
👍 stickies-v approved a pull request: "[29.x] backport #33212"
(https://github.com/bitcoin/bitcoin/pull/33251#pullrequestreview-3155597914)
ACK fcac8022d8
There have been quite substantial changes to `BaseIndex::Rewind()` in https://github.com/bitcoin/bitcoin/commit/6f1392cc42cde638773f2b697d7d2c58abcdc860 that might warrant extra careful review, even though I couldn't find any issues.
The rationale and mechanics from the original PR all still hold here, and the changes look correct to me. The backport commits are clean, and the added test in fcac8022d839572f5d8781096eec14ca7ea2e0dd still fails without the fix in 16b1710d97464
...
(https://github.com/bitcoin/bitcoin/pull/33251#pullrequestreview-3155597914)
ACK fcac8022d8
There have been quite substantial changes to `BaseIndex::Rewind()` in https://github.com/bitcoin/bitcoin/commit/6f1392cc42cde638773f2b697d7d2c58abcdc860 that might warrant extra careful review, even though I couldn't find any issues.
The rationale and mechanics from the original PR all still hold here, and the changes look correct to me. The backport commits are clean, and the added test in fcac8022d839572f5d8781096eec14ca7ea2e0dd still fails without the fix in 16b1710d97464
...
💬 kevkevinpal commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3224154673)
ACK 111938c
This change makes sense. I've definitely run into this issue, and being able to use a regular string is less confusing
I checked that with the change to `src/rpc/client.cpp`, the old tests using `convert_to_json_for_cli` still passed.
I was wondering if it makes sense to keep one test using `convert_to_json_for_cli` to ensure that these parameters can still be used as JSON for backward compatibility.
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3224154673)
ACK 111938c
This change makes sense. I've definitely run into this issue, and being able to use a regular string is less confusing
I checked that with the change to `src/rpc/client.cpp`, the old tests using `convert_to_json_for_cli` still passed.
I was wondering if it makes sense to keep one test using `convert_to_json_for_cli` to ensure that these parameters can still be used as JSON for backward compatibility.
📝 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.