💬 fanquake commented on issue "ci: tidy job is producing output":
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3223652850)
cc @Sjors @ryanofsky

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620446)
Yeah, encoding seems like another possible source for inconsistencies. I don't think we want to deal with encoding here, because the value is small enough for it to not matter.

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620527)
thx, reworded the comment a bit

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620620)
It is mostly a regression test, checking that on master a single arg with size of `CLI_MAX_ARG_SIZE = 131071` fails the test (with the reproducer shared in the pull description).

Force pushed to reduce to that (and added a comment).

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620722)
> This test also passes on master (MacOS environment with ARG_MAX = 1048576).

Thanks for adding a macos data point for reference. This further supports that the hard-coded value may be different on different platforms.

> However, the following test only passes on this PR, not on my master:

Nice, thanks for adapting the test and checking the bugfix on macos.

💬 maflcko commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3223742148)
Seems fine to pick this up now, if you want.

💬 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

💬 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

💬 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

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

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

💬 cedwies commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(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
...

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

📝 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
...

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

🤔 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.

💬 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

💬 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) {
```

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