💬 murchandamus commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476493286)
@ryanofsky Yeah, I was not proposing a change in behavior. It just seems cleaner to tie the intended behavior directly to the relevant wallet property than to a generic boolean like `success` that is instantiated half a code block away and much more likely to change its meaning in the course of the project.
@achow101: I would have expected that `DoMigration` sets `WALLET_FLAG_DESCRIPTOR`, but it doesn’t seem to be the case. If you wanted to set it generally, perhaps you could just set it afte
...
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476493286)
@ryanofsky Yeah, I was not proposing a change in behavior. It just seems cleaner to tie the intended behavior directly to the relevant wallet property than to a generic boolean like `success` that is instantiated half a code block away and much more likely to change its meaning in the course of the project.
@achow101: I would have expected that `DoMigration` sets `WALLET_FLAG_DESCRIPTOR`, but it doesn’t seem to be the case. If you wanted to set it generally, perhaps you could just set it afte
...
💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1924448996)
ACK a1414b3388a870aeb463e9cc293d47cbd86e7f0e
via rangediff a14b95129d3a2894b7a41ce919a426bb60f62e35..a1414b3388a870aeb463e9cc293d47cbd86e7f0e
The changes since my prior ACK appear to only pertain to improvements in the fuzz test to avoid circular packages and a typo fix on a comment in the code.
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1924448996)
ACK a1414b3388a870aeb463e9cc293d47cbd86e7f0e
via rangediff a14b95129d3a2894b7a41ce919a426bb60f62e35..a1414b3388a870aeb463e9cc293d47cbd86e7f0e
The changes since my prior ACK appear to only pertain to improvements in the fuzz test to avoid circular packages and a typo fix on a comment in the code.
📝 mzumsande opened a pull request: "fix intermittent failure in `rpc_setban.py --v2transport`, run it in CI"
(https://github.com/bitcoin/bitcoin/pull/29372)
This test failed for me on master locally:
The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later `connect_nodes` call.
Also add the test with `--v2transport` to the test runner because banning with v2 seems lik
...
(https://github.com/bitcoin/bitcoin/pull/29372)
This test failed for me on master locally:
The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later `connect_nodes` call.
Also add the test with `--v2transport` to the test runner because banning with v2 seems lik
...
💬 achow101 commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476516869)
`DoMigration` does do that, that's why this is only a problem for blank wallets.
As currently implemented, this only sets it for blank wallets, but I do also think the correct behavior is to set it for all wallets, but as I said earlier, I have not analyzed that yet.
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476516869)
`DoMigration` does do that, that's why this is only a problem for blank wallets.
As currently implemented, this only sets it for blank wallets, but I do also think the correct behavior is to set it for all wallets, but as I said earlier, I have not analyzed that yet.
💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1924479030)
> Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.
It makes sense that having backwards compatibility may not be worth it, but I don't see why test chains would set `nTx` to a real value without also setting BLOCK_VALID_TRANSACTIONS. It seems like we should be able to use BLOCK_VALID_TRANSACTIONS to know if n
...
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1924479030)
> Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.
It makes sense that having backwards compatibility may not be worth it, but I don't see why test chains would set `nTx` to a real value without also setting BLOCK_VALID_TRANSACTIONS. It seems like we should be able to use BLOCK_VALID_TRANSACTIONS to know if n
...
👋 Christewart's pull request is ready for review: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371)
(https://github.com/bitcoin/bitcoin/pull/29371)
💬 ryanofsky commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476587099)
re: https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476516869
> `DoMigration` does do that, that's why this is only a problem for blank wallets.
Seems like in a followup it might be cleaner to do this inside DoMigration, so the flag is set in one place. In the meantime it could help to change the "Make sure that descriptors flag is actually set" comment to "Make sure that descriptors flag is set if DoMigration is not called (otherwise DoMigration will set it)"
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476587099)
re: https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476516869
> `DoMigration` does do that, that's why this is only a problem for blank wallets.
Seems like in a followup it might be cleaner to do this inside DoMigration, so the flag is set in one place. In the meantime it could help to change the "Make sure that descriptors flag is actually set" comment to "Make sure that descriptors flag is set if DoMigration is not called (otherwise DoMigration will set it)"
🚀 ryanofsky merged a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367)
(https://github.com/bitcoin/bitcoin/pull/29367)
💬 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?