🚀 achow101 merged a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361)
(https://github.com/bitcoin/bitcoin/pull/29361)
💬 Empact commented on pull request "test: Fix CPartialMerkleTree.nTransactions signedness":
(https://github.com/bitcoin/bitcoin/pull/29363#issuecomment-1924312018)
ACK
(https://github.com/bitcoin/bitcoin/pull/29363#issuecomment-1924312018)
ACK
💬 jonatack commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1476415646)
Adding a new configuration option would need a release note.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1476415646)
Adding a new configuration option would need a release note.
💬 knst commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1924351566)
@Empact
In the original 2011 implementation, a regular std::string was used for the passphrase:
```
commit b7bcaf940d27fa8cfe89422943fbeaab7a350930
Author: Wladimir J. van der Laan <laanwj@gmail.com>
Date: Wed Aug 24 22:07:26 2011 +0200
Wallet encryption part 2: ask passphrase when needed, add menu options
diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp
new file mode 100644
index 0000000000..a297513a62
--- /dev/null
+++ b/src/qt/askpassphrasedia
...
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1924351566)
@Empact
In the original 2011 implementation, a regular std::string was used for the passphrase:
```
commit b7bcaf940d27fa8cfe89422943fbeaab7a350930
Author: Wladimir J. van der Laan <laanwj@gmail.com>
Date: Wed Aug 24 22:07:26 2011 +0200
Wallet encryption part 2: ask passphrase when needed, add menu options
diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp
new file mode 100644
index 0000000000..a297513a62
--- /dev/null
+++ b/src/qt/askpassphrasedia
...
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1924360021)
Added two commits to stay in sync with SRI: https://github.com/stratum-mining/stratum/pull/742
Next week I'll try to rebase this on the latest #29346, which has diverged a bit.
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1924360021)
Added two commits to stay in sync with SRI: https://github.com/stratum-mining/stratum/pull/742
Next week I'll try to rebase this on the latest #29346, which has diverged a bit.
💬 jonatack commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476464485)
Default value provided twice, can omit second one.
`3. maxburnamount (numeric or string, optional, default="0.00") Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value(default of 0), expressed in BTC.`
```suggestion
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + C
...
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476464485)
Default value provided twice, can omit second one.
`3. maxburnamount (numeric or string, optional, default="0.00") Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value(default of 0), expressed in BTC.`
```suggestion
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + C
...
🤔 stratospher reviewed a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1859682894)
tested ACK d8165db.
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1859682894)
tested ACK d8165db.
💬 stratospher commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1476463624)
d065b33: maybe say only for `LARGE_REORG_SIZE` tests? because the last 2 tests have v2 peers after [reconnection](https://github.com/bitcoin/bitcoin/blob/d8165db8e889f423351ae35fd375c982e793d136/test/functional/feature_block.py#L1312-L1320) due to block height mismatch/invalid block header version.
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1476463624)
d065b33: maybe say only for `LARGE_REORG_SIZE` tests? because the last 2 tests have v2 peers after [reconnection](https://github.com/bitcoin/bitcoin/blob/d8165db8e889f423351ae35fd375c982e793d136/test/functional/feature_block.py#L1312-L1320) due to block height mismatch/invalid block header version.
💬 stratospher commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1476295813)
d065b33: nit: ChaCha20 typo here and in feature_block.py
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1476295813)
d065b33: nit: ChaCha20 typo here and in feature_block.py
💬 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)