💬 jonatack commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476465530)
Same suggestion here as https://github.com/bitcoin/bitcoin/pull/28950/files#r1476464485.
```suggestion
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
```
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476465530)
Same suggestion here as https://github.com/bitcoin/bitcoin/pull/28950/files#r1476464485.
```suggestion
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
```
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924395126)
> In that case, I think it's totally fine for you to maintain a separate client in case miners would like to mine your potential future nonstandard transactions, and I wish you the best of luck in this endeavor.
> As you correctly point out, if this PR is merged users and miners have other options to coordinate for transactions which the community writ large doesn't want to have polluting their local mempool. It seems like your objective in the comments section of this PR isn't really to NACK
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924395126)
> In that case, I think it's totally fine for you to maintain a separate client in case miners would like to mine your potential future nonstandard transactions, and I wish you the best of luck in this endeavor.
> As you correctly point out, if this PR is merged users and miners have other options to coordinate for transactions which the community writ large doesn't want to have polluting their local mempool. It seems like your objective in the comments section of this PR isn't really to NACK
...
🤔 murchandamus reviewed a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1860014271)
cod review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
Nit: The code behavior seems correct to me, but I would prefer if either the commit message were adapted to state that the `WALLET_FLAG_DESCRIPTOR` is being set for all successfully migrated wallets, or if the code were adapted to more explicitly implement the behavior described in the commit as described in my comment above.
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1860014271)
cod review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
Nit: The code behavior seems correct to me, but I would prefer if either the commit message were adapted to state that the `WALLET_FLAG_DESCRIPTOR` is being set for all successfully migrated wallets, or if the code were adapted to more explicitly implement the behavior described in the commit as described in my comment above.
💬 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