💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416225122)
> Seems like `NO_KEY_TIME` ought to be `0` so it triggers a full scan
That was the bug I solved 0a1107d0.
By default, the legacy spkm is empty. It has no keys nor scripts, thus it should not trigger a full scan.
Once the first key is generated or the first script is imported,
the legacy spkm will update the `nTimeFirstKey` time automatically and start scanning blocks.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416225122)
> Seems like `NO_KEY_TIME` ought to be `0` so it triggers a full scan
That was the bug I solved 0a1107d0.
By default, the legacy spkm is empty. It has no keys nor scripts, thus it should not trigger a full scan.
Once the first key is generated or the first script is imported,
the legacy spkm will update the `nTimeFirstKey` time automatically and start scanning blocks.
🤔 murchandamus requested changes to a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1765984823)
I realized that we need to add the input fee to each UTXO rather than deducting it from the target for the two input set options to have the same waste score.
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1765984823)
I realized that we need to add the input fee to each UTXO rather than deducting it from the target for the two input set options to have the same waste score.
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416203316)
> Looks good! I am uncomfortable with adding the log message in [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) for now and r
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416203316)
> Looks good! I am uncomfortable with adding the log message in [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) for now and r
...
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416221619)
… drop the deducted fee:
```suggestion
add_coin(9 * CENT, /*nInput=*/0, expected_result);
add_coin(1 * CENT, /*nInput=*/0, expected_result);
const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb);
```
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416221619)
… drop the deducted fee:
```suggestion
add_coin(9 * CENT, /*nInput=*/0, expected_result);
add_coin(1 * CENT, /*nInput=*/0, expected_result);
const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb);
```
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416220620)
I just realized that this breaks the test. Deducting the fee of a specific count of inputs from the target breaks the equivalence of the input sets. The two inputs 9+1 now have exactly the target amount, whereas the single input of 10 will lead to an excess of one input fee. This would mean that whatever the feerate and long-term feerate, the solution with 9 and 1 would be preferred.
Instead of deducting the fee of two inputs from the target, this test should be fixed by adding one input fee
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416220620)
I just realized that this breaks the test. Deducting the fee of a specific count of inputs from the target breaks the equivalence of the input sets. The two inputs 9+1 now have exactly the target amount, whereas the single input of 10 will lead to an excess of one input fee. This would mean that whatever the feerate and long-term feerate, the solution with 9 and 1 would be preferred.
Instead of deducting the fee of two inputs from the target, this test should be fixed by adding one input fee
...
🤔 ryanofsky reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1765995922)
Concept ACK, and started reviewing. But I don't think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it's actually needed or just desired for some other reason.
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1765995922)
Concept ACK, and started reviewing. But I don't think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it's actually needed or just desired for some other reason.
💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416209707)
In commit "walletdb: Refactor privkey checksum calc" (0a59e878006763817d53017e50d268b32868bd8d)
> No, the point is so that when we are writing an HD key, this checksum covers the entire xpub too, not just the pubkey part.
I don't understand this, since the following change compiles:
```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -69,8 +69,7 @@ const std::unordered_set<std::string> LEGACY_TYPES{CRYPTED_KEY, CSCRIPT, DEFAULT
// WalletBatch
//
-template <
...
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416209707)
In commit "walletdb: Refactor privkey checksum calc" (0a59e878006763817d53017e50d268b32868bd8d)
> No, the point is so that when we are writing an HD key, this checksum covers the entire xpub too, not just the pubkey part.
I don't understand this, since the following change compiles:
```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -69,8 +69,7 @@ const std::unordered_set<std::string> LEGACY_TYPES{CRYPTED_KEY, CSCRIPT, DEFAULT
// WalletBatch
//
-template <
...
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841551086)
> But I don't think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it's actually needed or just desired for some other reason.
The description currently states:
> This loading is backwards compatible and uses a new non-required flag `WALLET_F
...
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841551086)
> But I don't think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it's actually needed or just desired for some other reason.
The description currently states:
> This loading is backwards compatible and uses a new non-required flag `WALLET_F
...
🤔 furszy reviewed a pull request: "index: block filters sync, reduce disk read operations by caching last header"
(https://github.com/bitcoin/bitcoin/pull/28955#pullrequestreview-1766044359)
> > Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.
>
> Is it, though? I would think the OS should have this cached regardless?
We will know it soon. I'm working on a way to test it inside https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1761271269.
But, it seems to be an orthogonal topic. Regardless of the result, which could vary depending on the OS, the changes in this PR s
...
(https://github.com/bitcoin/bitcoin/pull/28955#pullrequestreview-1766044359)
> > Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.
>
> Is it, though? I would think the OS should have this cached regardless?
We will know it soon. I'm working on a way to test it inside https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1761271269.
But, it seems to be an orthogonal topic. Regardless of the result, which could vary depending on the OS, the changes in this PR s
...
🤔 jonatack reviewed a pull request: "build: Enable -Wunreachable-code"
(https://github.com/bitcoin/bitcoin/pull/28999#pullrequestreview-1766061715)
ACK fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876 tested with arm64 clang 17.0.6
Could use [-Wunreachable-code-aggressive](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-aggressive) -- it catches a few additional unreachable `break` statements in the current code.
(https://github.com/bitcoin/bitcoin/pull/28999#pullrequestreview-1766061715)
ACK fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876 tested with arm64 clang 17.0.6
Could use [-Wunreachable-code-aggressive](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-aggressive) -- it catches a few additional unreachable `break` statements in the current code.
💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841580968)
> An important point is that `ACTIVEHDKEY` does not need to exist after the upgrade was done.
Oh, that explains why you would want a flag. But I'm not sure when this would happen except when the key can't be determined, which seems like an unusual case. But I need to look into the PR more in any case, so thanks for the correction.
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841580968)
> An important point is that `ACTIVEHDKEY` does not need to exist after the upgrade was done.
Oh, that explains why you would want a flag. But I'm not sure when this would happen except when the key can't be determined, which seems like an unusual case. But I need to look into the PR more in any case, so thanks for the correction.
🤔 jonatack reviewed a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766097325)
ACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
This change would result in the following help:
```
$ ./src/bitcoin-cli -signet help encryptwallet
encryptwallet "passphrase"
Encrypts the wallet with 'passphrase'. This is for first time encryption.
After this, any calls that interact with private keys such as sending or signing
will require the passphrase to be set prior the making these calls.
Use the walletpassphrase call for this, and then walletlock call.
If the wallet is already
...
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766097325)
ACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
This change would result in the following help:
```
$ ./src/bitcoin-cli -signet help encryptwallet
encryptwallet "passphrase"
Encrypts the wallet with 'passphrase'. This is for first time encryption.
After this, any calls that interact with private keys such as sending or signing
will require the passphrase to be set prior the making these calls.
Use the walletpassphrase call for this, and then walletlock call.
If the wallet is already
...
💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841606774)
> Could alternatively use [-Wunreachable-code-aggressive](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-aggressive) -- it catches a few additional unreachable `break` statements in the current code.
They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841606774)
> Could alternatively use [-Wunreachable-code-aggressive](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-aggressive) -- it catches a few additional unreachable `break` statements in the current code.
They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841609784)
> But I'm not sure when this would happen except when the key can't be determined, which seems like an unusual case.
It would happen for all watch-only wallets, for example, although we do check for that flag separately.
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841609784)
> But I'm not sure when this would happen except when the key can't be determined, which seems like an unusual case.
It would happen for all watch-only wallets, for example, although we do check for that flag separately.
💬 jonatack commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841613979)
> They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841613979)
> They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.
💬 achow101 commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841622415)
Perhaps a test for this case?
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841622415)
Perhaps a test for this case?
💬 jonatack commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416294937)
```suggestion
if table_name == "tried" and result["error"] == "failed-adding-to-tried":
```
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416294937)
```suggestion
if table_name == "tried" and result["error"] == "failed-adding-to-tried":
```
🤔 jonatack reviewed a pull request: "rpc: addpeeraddress tried return error on failure"
(https://github.com/bitcoin/bitcoin/pull/28998#pullrequestreview-1766137449)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28998#pullrequestreview-1766137449)
Concept ACK
💬 jonatack commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416295706)
Not sure, maybe test explicitly for `True` rather than just truthy
```suggestion
if result["success"] is True:
```
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416295706)
Not sure, maybe test explicitly for `True` rather than just truthy
```suggestion
if result["success"] is True:
```
💬 techy2 commented on issue "getrawtransaction xxxxxx.... 2 causes a segfault":
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1841638292)
Over the years I have found a lot of obscure bugs in all kinds of code. I think karma pushes them towards me LOL
I'm 80+ and still finding them. I seem to have a knack for it... purely by accident.
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1841638292)
Over the years I have found a lot of obscure bugs in all kinds of code. I think karma pushes them towards me LOL
I'm 80+ and still finding them. I seem to have a knack for it... purely by accident.