💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301723616)
Good catch! You're right - since `create_self_transfer()` defaults to `fee_rate=0.003`, explicitly setting it here is redundant and potentially confusing. I've removed the parameter to use the default.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301723616)
Good catch! You're right - since `create_self_transfer()` defaults to `fee_rate=0.003`, explicitly setting it here is redundant and potentially confusing. I've removed the parameter to use the default.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301724711)
Good idea, but `send_self_transfer()` only creates self-transfers (sending back to the wallet's own address). For this test, we need external outputs to properly test `getblockstats` UTXO statistics. The `send_to()` method is necessary here to create transactions with external recipients.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301724711)
Good idea, but `send_self_transfer()` only creates self-transfers (sending back to the wallet's own address). For this test, we need external outputs to properly test `getblockstats` UTXO statistics. The `send_to()` method is necessary here to create transactions with external recipients.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301728062)
This mining pattern is intentional - it creates a specific block structure for testing:
- Block 1: 1 transaction (first 10 BTC send)
- Block 2: 4 transactions (remaining sends + OP_RETURN)
This allows testing `getblockstats` across different block scenarios (single vs multi-transaction blocks) rather than having all transactions in one block.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301728062)
This mining pattern is intentional - it creates a specific block structure for testing:
- Block 1: 1 transaction (first 10 BTC send)
- Block 2: 4 transactions (remaining sends + OP_RETURN)
This allows testing `getblockstats` across different block scenarios (single vs multi-transaction blocks) rather than having all transactions in one block.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301731981)
You're right - we need to re-serialize after modifying the transaction. The `tx["hex"]` contains the original transaction, but we're modifying `tx["tx"]` by adding the OP_RETURN output. Without re-serializing, the OP_RETURN wouldn't actually be included in the sent transaction.
I've applied your suggested fix.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2301731981)
You're right - we need to re-serialize after modifying the transaction. The `tx["hex"]` contains the original transaction, but we're modifying `tx["tx"]` by adding the OP_RETURN output. Without re-serializing, the OP_RETURN wouldn't actually be included in the sent transaction.
I've applied your suggested fix.
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301743800)
* `listsinceblock`'s `blockhash` is included in this list even though block hashes cannot include `=`. There are also a ton of other RPCs that take a block hash in the same way but are not included in this list.
* `descriptorprocesspsbt`'s `sighashtype` is a parameter which has a list of fixed string values and none of them have an `=`. There are multiple RPCs that take a sighashtype but they are not included in this list.
* `sendmany`'s `dummy` must be an empty string so it is reasonable to e
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301743800)
* `listsinceblock`'s `blockhash` is included in this list even though block hashes cannot include `=`. There are also a ton of other RPCs that take a block hash in the same way but are not included in this list.
* `descriptorprocesspsbt`'s `sighashtype` is a parameter which has a list of fixed string values and none of them have an `=`. There are multiple RPCs that take a sighashtype but they are not included in this list.
* `sendmany`'s `dummy` must be an empty string so it is reasonable to e
...
💬 frankomosh commented on pull request "fuzz: add target for `DifferenceFormatter` and p2p invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2301744219)
@brunoerg The PR title was already updated yesterday, to "fuzz: add target for DifferenceFormatter and p2p invariant check" (changed from the previous version that mentioned net_processing.cpp). Please let me know if you’d still like to see any wording tweaks?
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2301744219)
@brunoerg The PR title was already updated yesterday, to "fuzz: add target for DifferenceFormatter and p2p invariant check" (changed from the previous version that mentioned net_processing.cpp). Please let me know if you’d still like to see any wording tweaks?
💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3225201197)
Rebased on top of master 6001029dfd21a7ddd8ecb906961ed16c8f074298
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3225201197)
Rebased on top of master 6001029dfd21a7ddd8ecb906961ed16c8f074298
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301769238)
`internal` is only used for setting the labels in the address book within `AddWalletDescriptor`, and only if the descriptor is not ranged and that there is a label provided. Since we copy the labels from the address book right after this, there's no need for us to provide `label` or `internal` to this call.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301769238)
`internal` is only used for setting the labels in the address book within `AddWalletDescriptor`, and only if the descriptor is not ranged and that there is a label provided. Since we copy the labels from the address book right after this, there's no need for us to provide `label` or `internal` to this call.
💬 janb84 commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225229760)
I have done a review on the code and tried to understand what to look for, to improve my PR reviewing in the future.
I'm missing some experience to give "judgement" on this PR.
The PR restores code to undo some "optimisations" that had a negative impact on the performance (restores caching ) and adds some extra benchmarks.
results of benchmarking this PR (both reverted):
| ns/op | op/s | err% | total | benchmark
|--------------------:|---------------
...
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225229760)
I have done a review on the code and tried to understand what to look for, to improve my PR reviewing in the future.
I'm missing some experience to give "judgement" on this PR.
The PR restores code to undo some "optimisations" that had a negative impact on the performance (restores caching ) and adds some extra benchmarks.
results of benchmarking this PR (both reverted):
| ns/op | op/s | err% | total | benchmark
|--------------------:|---------------
...
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301786086)
I think now I got what you are trying to say. So the things like `blockhash`, `dummy`, `sighashtype` etc are included because we are not mainly targeting these params; instead, we are targeting methods which might contain a `=` character. For example, `listsinceblock` contains a param named `label` which might contain an `=` char but according to the rule for consistency, we need to include all other string params related to the same method `(listsinceblock)`. That's why I included `blockhash` h
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301786086)
I think now I got what you are trying to say. So the things like `blockhash`, `dummy`, `sighashtype` etc are included because we are not mainly targeting these params; instead, we are targeting methods which might contain a `=` character. For example, `listsinceblock` contains a param named `label` which might contain an `=` char but according to the rule for consistency, we need to include all other string params related to the same method `(listsinceblock)`. That's why I included `blockhash` h
...
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301789533)
I think it conceptually makes more sense to be passing paths as ` fs::Path`, rather than strings. Ideally, `BackupWallet` should be taking a `fs::Path` instead of a string, and the other conversions are only necessary for logging.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301789533)
I think it conceptually makes more sense to be passing paths as ` fs::Path`, rather than strings. Ideally, `BackupWallet` should be taking a `fs::Path` instead of a string, and the other conversions are only necessary for logging.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791098)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791098)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791322)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791322)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791661)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791661)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792083)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792083)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792295)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792295)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792510)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792510)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792754)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792754)
Done
🤔 l0rinc reviewed a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3156667375)
I like this simpler version more, left a few more comments
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3156667375)
I like this simpler version more, left a few more comments
💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301687712)
nit: This seems to be used a single time, we might as well inline it
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301687712)
nit: This seems to be used a single time, we might as well inline it