💬 achow101 commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2825129551)
> 1. The `listreceivedbyaddress` RPC doesn't show any entry for `addr` to which the 1 BTC was sent, but `listunspent` RPC does show.
This is expected since we didn't get `addr` through normal means, and it therefore is missing expected items for receiving addresses like an address book entry.
> 2. The `desc` field in the `bad_deriv_wallet.listunspent` and `bad_deriv_wallet_master.listunspent` calls for the same `addr` was different slightly as I see `'` was used in the former & `h` in the
...
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2825129551)
> 1. The `listreceivedbyaddress` RPC doesn't show any entry for `addr` to which the 1 BTC was sent, but `listunspent` RPC does show.
This is expected since we didn't get `addr` through normal means, and it therefore is missing expected items for receiving addresses like an address book entry.
> 2. The `desc` field in the `bad_deriv_wallet.listunspent` and `bad_deriv_wallet_master.listunspent` calls for the same `addr` was different slightly as I see `'` was used in the former & `h` in the
...
💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056630902)
AFAICT the workaround is correct, and this test is already using `importdescriptors` on master.
This test succeeds as-is on this branch with this commit (https://github.com/bitcoin/bitcoin/pull/31250/commits/e6c900f9c0e1b143098663e1c5c80e1811946b2d) dropped.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056630902)
AFAICT the workaround is correct, and this test is already using `importdescriptors` on master.
This test succeeds as-is on this branch with this commit (https://github.com/bitcoin/bitcoin/pull/31250/commits/e6c900f9c0e1b143098663e1c5c80e1811946b2d) dropped.
💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056635528)
Nit: Extra "the".
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056635528)
Nit: Extra "the".
💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2054565214)
For other reviewers: Unlike for legacy wallets, watch only[^1] descriptor wallets do not need/have their ScriptPubKeyMans set up during wallet [creation](https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3003).
https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3073-L3080
---
Tangentially, it might be nice to refactor `SetupDescriptorsWallet()` to reuse `CWallet::Create()` rather
...
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2054565214)
For other reviewers: Unlike for legacy wallets, watch only[^1] descriptor wallets do not need/have their ScriptPubKeyMans set up during wallet [creation](https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3003).
https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3073-L3080
---
Tangentially, it might be nice to refactor `SetupDescriptorsWallet()` to reuse `CWallet::Create()` rather
...
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056733702)
I certain that this commit was required when writing this PR originally, however, I no longer recall the error that it fixed. Dropping this commit.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056733702)
I certain that this commit was required when writing this PR originally, however, I no longer recall the error that it fixed. Dropping this commit.
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056734208)
I think the `%` is your command line prompt?
This was missing a newline, fixed.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056734208)
I think the `%` is your command line prompt?
This was missing a newline, fixed.
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056734975)
Fixed
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056734975)
Fixed
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2056768817)
We're continually updating this doc and for now we're adding more and more to the mac section - it's not getting any simpler for now, seems we still need the mac section for now. And when that changes, we'll update the doc.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2056768817)
We're continually updating this doc and for now we're adding more and more to the mac section - it's not getting any simpler for now, seems we still need the mac section for now. And when that changes, we'll update the doc.
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2825349060)
> it would be good to mention [Wiki: AddressSanitizerContainerOverflow (false positives) (google/sanitizers)](https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives)
Added the links to the doc and added another workaround of setting the target architecture an Apple Silicon for errors like:
```bash
usr/include/malloc/_malloc_type.h:66:126: error: unknown type name 'size_t'; did you mean 'std::size_t'?
```
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2825349060)
> it would be good to mention [Wiki: AddressSanitizerContainerOverflow (false positives) (google/sanitizers)](https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives)
Added the links to the doc and added another workaround of setting the target architecture an Apple Silicon for errors like:
```bash
usr/include/malloc/_malloc_type.h:66:126: error: unknown type name 'size_t'; did you mean 'std::size_t'?
```
🤔 janb84 reviewed a pull request: "guix: Remove unused `file` package"
(https://github.com/bitcoin/bitcoin/pull/32242#pullrequestreview-2788550078)
Code review and Tested ACK [513e202](https://github.com/bitcoin/bitcoin/commit/513e2020a9acdd366d4933780b331f97bac85597)
- Did a code review
- Full GUIX build under nixos on macos via UTM, works !
(https://github.com/bitcoin/bitcoin/pull/32242#pullrequestreview-2788550078)
Code review and Tested ACK [513e202](https://github.com/bitcoin/bitcoin/commit/513e2020a9acdd366d4933780b331f97bac85597)
- Did a code review
- Full GUIX build under nixos on macos via UTM, works !
💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2825373348)
I've pushed an update to check more broadly. The new issues (for just Polish) include some false positives around (change/reszta). Though, the others largely apply as far as I can see:
#### Erroneous translation:
<source>Warning: If you encrypt your wallet and lose your passphrase, you will <b>LOSE ALL OF YOUR BITCOINS</b>!</source>
<translation>hasłoOstrzeżenie: Jeśli zaszyfrujesz swój portfel i zgubisz hasło - <b>STRACISZ WSZYSTKIE SWOJE BITCONY</b>!</t
...
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2825373348)
I've pushed an update to check more broadly. The new issues (for just Polish) include some false positives around (change/reszta). Though, the others largely apply as far as I can see:
#### Erroneous translation:
<source>Warning: If you encrypt your wallet and lose your passphrase, you will <b>LOSE ALL OF YOUR BITCOINS</b>!</source>
<translation>hasłoOstrzeżenie: Jeśli zaszyfrujesz swój portfel i zgubisz hasło - <b>STRACISZ WSZYSTKIE SWOJE BITCONY</b>!</t
...
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056790435)
Rather than explaining the line below, I added this comment to add context that I think isn't immediately available from the test, namely what the (obscure in both use and name) `listwalletdir` rpc does, I think your suggested comment would repeat what the test is asserting.
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056790435)
Rather than explaining the line below, I added this comment to add context that I think isn't immediately available from the test, namely what the (obscure in both use and name) `listwalletdir` rpc does, I think your suggested comment would repeat what the test is asserting.
🤔 achow101 reviewed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-2788566889)
There should also be a test where multiple things are imported, one with a timestamp, and one with `never`. There should be a rescan in this case.
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-2788566889)
There should also be a test where multiple things are imported, one with a timestamp, and one with `never`. There should be a rescan in this case.
💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2056798497)
While this verifies that the address was imported, what we care about here is whether a rescan occurred. It would be better to send to the address for the descriptor being imported, then check to see that after importing, the transaction is not detected.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2056798497)
While this verifies that the address was imported, what we care about here is whether a rescan occurred. It would be better to send to the address for the descriptor being imported, then check to see that after importing, the transaction is not detected.
🤔 mzumsande reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2788360332)
Looks good, except for an issue with commit 00fe040eca4b77d05ec5652b4d494b0ded183653
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2788360332)
Looks good, except for an issue with commit 00fe040eca4b77d05ec5652b4d494b0ded183653
💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2056670803)
This function is declared here, but not implemented or used anywhere. Is this meant to be called somewhere in `UnloadWallets`? Or does UnloadWallets already close the db somewhere (where?) and this can be deleted?
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2056670803)
This function is declared here, but not implemented or used anywhere. Is this meant to be called somewhere in `UnloadWallets`? Or does UnloadWallets already close the db somewhere (where?) and this can be deleted?
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056813556)
Unlike, [`assert_not_equal`](https://github.com/bitcoin/bitcoin/blob/6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97/test/functional/test_framework/util.py#L79-L81), `assert_equal` [does not support](https://github.com/bitcoin/bitcoin/blob/6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97/test/functional/test_framework/util.py#L72-L77) passing an error message. Re: the conceptual change; while it might have some advantages over the style currently used, as far as I can tell it is almost never the case that we l
...
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056813556)
Unlike, [`assert_not_equal`](https://github.com/bitcoin/bitcoin/blob/6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97/test/functional/test_framework/util.py#L79-L81), `assert_equal` [does not support](https://github.com/bitcoin/bitcoin/blob/6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97/test/functional/test_framework/util.py#L72-L77) passing an error message. Re: the conceptual change; while it might have some advantages over the style currently used, as far as I can tell it is almost never the case that we l
...
💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2056818503)
Just forgot to remove it, done now.
The db is closed when a `CWallet` is destroyed.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2056818503)
Just forgot to remove it, done now.
The db is closed when a `CWallet` is destroyed.
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056837989)
We are already asserting the transaction id exists in the migrated wallet, and that the wallet has the correct balance, I'm not totally opposed to checking the tx amount as opposed to the wallet balance, but I'm not sure that this change really does add any guarantees, or help with debugging when the test fails, so I will err on the side of keeping this as-is so it [parallels](https://github.com/bitcoin/bitcoin/blob/9a4c92eb9ac29204df3d826f5ae526ab16b8ad65/test/functional/wallet_migration.py#L53
...
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056837989)
We are already asserting the transaction id exists in the migrated wallet, and that the wallet has the correct balance, I'm not totally opposed to checking the tx amount as opposed to the wallet balance, but I'm not sure that this change really does add any guarantees, or help with debugging when the test fails, so I will err on the side of keeping this as-is so it [parallels](https://github.com/bitcoin/bitcoin/blob/9a4c92eb9ac29204df3d826f5ae526ab16b8ad65/test/functional/wallet_migration.py#L53
...
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056839286)
I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056839286)
I'll leave this as-is for now.