💬 ryanofsky commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#issuecomment-1924582691)
I went ahead and merged this in its current form since it is a minimal bugfix for a bug introduced yesterday in #28976. Without #28976, blank wallets would refuse to migrate. But with #28976 and without this fix, blank wallets would appear to successfully migrate but be in an unsupported legacy mode + sqlite backend state, so it seemed better to merge sooner rather than later.
It should be possible to followup on comments here and in the previous PR:
- https://github.com/bitcoin/bitcoin/pu
...
(https://github.com/bitcoin/bitcoin/pull/29367#issuecomment-1924582691)
I went ahead and merged this in its current form since it is a minimal bugfix for a bug introduced yesterday in #28976. Without #28976, blank wallets would refuse to migrate. But with #28976 and without this fix, blank wallets would appear to successfully migrate but be in an unsupported legacy mode + sqlite backend state, so it seemed better to merge sooner rather than later.
It should be possible to followup on comments here and in the previous PR:
- https://github.com/bitcoin/bitcoin/pu
...
🤔 furszy reviewed a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1860238551)
post-merge ACK 3904123
In the test, would be nice to decouple the `migratewallet()` call into another function, to consistently check that all migrated wallet contain this flag.
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1860238551)
post-merge ACK 3904123
In the test, would be nice to decouple the `migratewallet()` call into another function, to consistently check that all migrated wallet contain this flag.
🤔 furszy reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1860288188)
Now that #28170 has been merged, it would be good to include a test case for it.
The node should only connect to `NODE_NETWORK_LIMITED` peers when the local chain is within 24h window from the tip.
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1860288188)
Now that #28170 has been merged, it would be good to include a test case for it.
The node should only connect to `NODE_NETWORK_LIMITED` peers when the local chain is within 24h window from the tip.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476692557)
It was being used for migration as `LegacyScriptPubKeyMan::MigrateToDescriptor()` creates `DescriptorScriptPubKeyMan` which needs it. However, these can also just take a no-op function, and I've done that as well as your suggestion.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476692557)
It was being used for migration as `LegacyScriptPubKeyMan::MigrateToDescriptor()` creates `DescriptorScriptPubKeyMan` which needs it. However, these can also just take a no-op function, and I've done that as well as your suggestion.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476693351)
It was necessary since the cache was being refilled by `LegacyScriptPubKeyMan::MigrateToDescriptor` when it creates the `DescriptorScriptPubKeyMan` replacements. But since those are now made with a no-op callback, this is unnecessary, so I've removed this and replaced it with an `Assume(m_cached_spks.empty())`.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476693351)
It was necessary since the cache was being refilled by `LegacyScriptPubKeyMan::MigrateToDescriptor` when it creates the `DescriptorScriptPubKeyMan` replacements. But since those are now made with a no-op callback, this is unnecessary, so I've removed this and replaced it with an `Assume(m_cached_spks.empty())`.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476693441)
Done
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476693441)
Done
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476694495)
The cache is intended to be somewhat generic, so if we had another type of ScriptPubKeyMan in the future, it would still be used, so leaving this comment as is.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1476694495)
The cache is intended to be somewhat generic, so if we had another type of ScriptPubKeyMan in the future, it would still be used, so leaving this comment as is.
💬 brunoerg commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1924688906)
@naumenkogs and @amitiuttarwar I just updated the description with more infos. Thanks!
--------
> Or, what i think is even more likely, we may rather change Addrman architecture to facilitate this better in the first place.
@naumenkogs: What do you mean by changing addrman architecture?
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1924688906)
@naumenkogs and @amitiuttarwar I just updated the description with more infos. Thanks!
--------
> Or, what i think is even more likely, we may rather change Addrman architecture to facilitate this better in the first place.
@naumenkogs: What do you mean by changing addrman architecture?
👍 josibake approved a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1860220702)
crACK https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39
Looks good! Code reviewed, didn't verify the bench numbers. Left some nits, feel free to ignore
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1860220702)
crACK https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39
Looks good! Code reviewed, didn't verify the bench numbers. Left some nits, feel free to ignore
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476632077)
in "refactor: SetAddressBookWithDB, minimize number of map lookups" (https://github.com/bitcoin/bitcoin/pull/26836/commits/10f9639fd6332cdd440f540c2202082044aab98c):
nit: unnecessary formatting change
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476632077)
in "refactor: SetAddressBookWithDB, minimize number of map lookups" (https://github.com/bitcoin/bitcoin/pull/26836/commits/10f9639fd6332cdd440f540c2202082044aab98c):
nit: unnecessary formatting change
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476711835)
in "refactor: wallet, simplify addressbook migration" (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):
This is a pretty thorny block of code with all of the booleans, let me summarize just to make sure I'm following:
* If the destination requires_transfer but isn't ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail
* If it requires transfer and does belong to the current wallet in the loop
...
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476711835)
in "refactor: wallet, simplify addressbook migration" (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):
This is a pretty thorny block of code with all of the booleans, let me summarize just to make sure I'm following:
* If the destination requires_transfer but isn't ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail
* If it requires transfer and does belong to the current wallet in the loop
...
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476626408)
in "refactor: SetAddrBookWithDB, signal only if write succeeded" (https://github.com/bitcoin/bitcoin/pull/26836/commits/0b83c8691b277d407e61a8ffca807e65866f4ded):
Maybe do `encoded_dest = EncodeDestination(address)` and then use that to avoid calling `EncodeDestination` twice?
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476626408)
in "refactor: SetAddrBookWithDB, signal only if write succeeded" (https://github.com/bitcoin/bitcoin/pull/26836/commits/0b83c8691b277d407e61a8ffca807e65866f4ded):
Maybe do `encoded_dest = EncodeDestination(address)` and then use that to avoid calling `EncodeDestination` twice?
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476718674)
in "wallet: migration, batch addressbook records removal" (https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39):
nit: most other places in the codebase we use `DB` in functions names (as opposed to `Db`)
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476718674)
in "wallet: migration, batch addressbook records removal" (https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39):
nit: most other places in the codebase we use `DB` in functions names (as opposed to `Db`)
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476616953)
in "wallet: clean redundancies in DelAddressBook" (https://github.com/bitcoin/bitcoin/pull/26836/commits/da3b8f927c2da5b7761e36fe548eb8ade29171d7):
nit: your commit message mentions "Call IsMine only once (instead of two)" but I don't see any IsMine related changes in the commit?
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476616953)
in "wallet: clean redundancies in DelAddressBook" (https://github.com/bitcoin/bitcoin/pull/26836/commits/da3b8f927c2da5b7761e36fe548eb8ade29171d7):
nit: your commit message mentions "Call IsMine only once (instead of two)" but I don't see any IsMine related changes in the commit?
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476724914)
in "refactor: wallet, simplify addressbook migration" (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):
nit: C++20 has `.contains(arg)` for sets (yay!). No performance difference, but semantically I think it makes a lot more sense to use when testing for existence.
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476724914)
in "refactor: wallet, simplify addressbook migration" (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):
nit: C++20 has `.contains(arg)` for sets (yay!). No performance difference, but semantically I think it makes a lot more sense to use when testing for existence.
💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1924710289)
ACK https://github.com/bitcoin/bitcoin/commit/46b3c569cfd01a45e5d9cf9428a07b06652e3df9
Looks good, thanks for taking the suggestions! Having the callbacks seems like a really nice feature: based on one of your review comments, I took a stab at using the callbacks with fast wallet rescans and using them simplifies that code quite a bit.
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1924710289)
ACK https://github.com/bitcoin/bitcoin/commit/46b3c569cfd01a45e5d9cf9428a07b06652e3df9
Looks good, thanks for taking the suggestions! Having the callbacks seems like a really nice feature: based on one of your review comments, I took a stab at using the callbacks with fast wallet rescans and using them simplifies that code quite a bit.
💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1924738424)
> Code looks good. I have been testing this with the asmap.dat file from https://github.com/fjahr/asmap-data/pull/6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?
Just checked it, @fjahr. I think `assert_debug_log` is slowing it down. Can you please check with the following diff?
```diff
diff --git a/contrib/asmap/test_bucketing.py b/contr
...
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1924738424)
> Code looks good. I have been testing this with the asmap.dat file from https://github.com/fjahr/asmap-data/pull/6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?
Just checked it, @fjahr. I think `assert_debug_log` is slowing it down. Can you please check with the following diff?
```diff
diff --git a/contrib/asmap/test_bucketing.py b/contr
...
💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476750977)
In commit "wallet: Reload the wallet if migration exited early" (78ba0e6748d2a519a96c41dea851e7c43b82f251)
Not related to this PR, but I think the `fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()` expression repeated throughout this function is unnecessarily verbose and also potentially dangerous if it is used in another context, or if the surrounding context changes.
It happens to be safe currently, because we know this code is only called after MigrateToSQLite is call
...
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476750977)
In commit "wallet: Reload the wallet if migration exited early" (78ba0e6748d2a519a96c41dea851e7c43b82f251)
Not related to this PR, but I think the `fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()` expression repeated throughout this function is unnecessarily verbose and also potentially dangerous if it is used in another context, or if the surrounding context changes.
It happens to be safe currently, because we know this code is only called after MigrateToSQLite is call
...
👍 ryanofsky approved a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1860423051)
Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.
I left a review comment about improving derivation of wallet paths in the MigrateLegacyToDescriptor function, but that isn't really about this PR and would be better addressed in a separate followup.
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1860423051)
Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.
I left a review comment about improving derivation of wallet paths in the MigrateLegacyToDescriptor function, but that isn't really about this PR and would be better addressed in a separate followup.
💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1924743815)
If @furszy wants to re-ack, or if someone else adds a review, I think this could be merged
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1924743815)
If @furszy wants to re-ack, or if someone else adds a review, I think this could be merged