💬 Sjors commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620591164)
If anyone on master is affected by this, we could perhaps add a command to wallet-tool that resets the descriptor ID and cache.
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620591164)
If anyone on master is affected by this, we could perhaps add a command to wallet-tool that resets the descriptor ID and cache.
👍 brunoerg approved a pull request: "fuzz: Generate rpc fuzz targets individually"
(https://github.com/bitcoin/bitcoin/pull/28015#pullrequestreview-1513276185)
ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a
Running only with `rpc`, `targets` in `generate_corpus` becomes:
```
[('rpc', {'LIMIT_TO_RPC_COMMAND': 'analyzepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'clearbanned'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinerawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'converttopsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createmultisig'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createpsbt'}), ('rpc', {'LIMIT_
...
(https://github.com/bitcoin/bitcoin/pull/28015#pullrequestreview-1513276185)
ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a
Running only with `rpc`, `targets` in `generate_corpus` becomes:
```
[('rpc', {'LIMIT_TO_RPC_COMMAND': 'analyzepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'clearbanned'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinerawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'converttopsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createmultisig'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createpsbt'}), ('rpc', {'LIMIT_
...
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252291878)
> Are you proposing to limit the annex size including any tags to 257 bytes? I see the conceptual difference, but there wouldn't be a functional difference for now?
Yes, for now it doesn’t make a functional difference. The issue I have in mind is the following, assuming the unstructured annex format is deployed and we see applications building on top of it, the 256 (or 257 now) annex size limit will have to become a max-size data payload, which is going to leak in the all application toolchai
...
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252291878)
> Are you proposing to limit the annex size including any tags to 257 bytes? I see the conceptual difference, but there wouldn't be a functional difference for now?
Yes, for now it doesn’t make a functional difference. The issue I have in mind is the following, assuming the unstructured annex format is deployed and we see applications building on top of it, the 256 (or 257 now) annex size limit will have to become a max-size data payload, which is going to leak in the all application toolchai
...
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252299556)
> Maybe you don't want to use it for multi-party protocols at all because of that.
I think that’s a good question, there is an annex inflation risk concerning the multi-party protocol users and there is a new CPU DoS risk that is faced by node operators due to the increased in annex data size. I think this CPU DoS risk is coming from the fact that with BIP341, there is a `sha_annex` in the transaction digest if the annex is present and therefore you have a new SHA-256 ops and data bytes to ha
...
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252299556)
> Maybe you don't want to use it for multi-party protocols at all because of that.
I think that’s a good question, there is an annex inflation risk concerning the multi-party protocol users and there is a new CPU DoS risk that is faced by node operators due to the increased in annex data size. I think this CPU DoS risk is coming from the fact that with BIP341, there is a `sha_annex` in the transaction digest if the annex is present and therefore you have a new SHA-256 ops and data bytes to ha
...
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252300001)
See the CPU DoS concerned mentioned above about BIP341’s `sha_annex`, if annexes start to be relayed.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252300001)
See the CPU DoS concerned mentioned above about BIP341’s `sha_annex`, if annexes start to be relayed.
💬 luke-jr commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#issuecomment-1620640272)
The current PR doesn't make sense to me. It's treating a height as a time? But no block would have a time before a height... So this just adds calculations that do nothing?
(https://github.com/bitcoin/bitcoin/pull/27427#issuecomment-1620640272)
The current PR doesn't make sense to me. It's treating a height as a time? But no block would have a time before a height... So this just adds calculations that do nothing?
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252303541)
> Especially if there are no concrete plans for multi-party protocols that require the unique properties of the annex.
If you’re taking the use-case of unstructured data discussed on the mailing list, I think there is still a leak in term of fee-bumping reserves than a user must provision for, in case of the worst annex inflation attacks being done, under known limits of `PER_INPUT_MAX_ANNEX_SIZE` (and eventual `MAX_ANNEX_BUDGET`).
Of course, as you’re raising you can always have an advers
...
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252303541)
> Especially if there are no concrete plans for multi-party protocols that require the unique properties of the annex.
If you’re taking the use-case of unstructured data discussed on the mailing list, I think there is still a leak in term of fee-bumping reserves than a user must provision for, in case of the worst annex inflation attacks being done, under known limits of `PER_INPUT_MAX_ANNEX_SIZE` (and eventual `MAX_ANNEX_BUDGET`).
Of course, as you’re raising you can always have an advers
...
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1620650118)
Left a new round of reviews, I think the following conceptual concerns are pending:
- padding of the signaling byte to preserve evolvability for future consensus validation of the annex data
- adding a `MAX_ANNEX_BUDGET` or not to mitigate potential CPU DoS concerns for full-nodes, and potential impact for fee-bumping reserves
- opted-in of `annexrelay` for node operators
- annex policy limits versioned on the transaction’s `nVersion` field
I still have to reply on the latest mail post ab
...
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1620650118)
Left a new round of reviews, I think the following conceptual concerns are pending:
- padding of the signaling byte to preserve evolvability for future consensus validation of the annex data
- adding a `MAX_ANNEX_BUDGET` or not to mitigate potential CPU DoS concerns for full-nodes, and potential impact for fee-bumping reserves
- opted-in of `annexrelay` for node operators
- annex policy limits versioned on the transaction’s `nVersion` field
I still have to reply on the latest mail post ab
...
💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1252319116)
ACK, please resolve.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1252319116)
ACK, please resolve.
💬 dimitaracev commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1620693065)
ACK `5cf4427`
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1620693065)
ACK `5cf4427`
💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1252331248)
> I must either be missing something about this line or it might be unnecessary. If I understand correctly this is updating `m_undo_height_in_last_blockfile` when the next block is connected after the flush happened. However, why not update it right away when the flush happens? I tried that by removing this line it seems like all tests are still passing without it.
It's not surprising that there's no test coverage for this. But to explain: the goal of the code is to flush the undo file when t
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1252331248)
> I must either be missing something about this line or it might be unnecessary. If I understand correctly this is updating `m_undo_height_in_last_blockfile` when the next block is connected after the flush happened. However, why not update it right away when the flush happens? I tried that by removing this line it seems like all tests are still passing without it.
It's not surprising that there's no test coverage for this. But to explain: the goal of the code is to flush the undo file when t
...
💬 JayBitron commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252364554)
`compare` throws an exception if the input string is lower than 4 character, but since this a conflict PR, please close it.
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252364554)
`compare` throws an exception if the input string is lower than 4 character, but since this a conflict PR, please close it.
💬 ismaelsadeeq commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252358645)
```c++
if (parent_feerate < individual_feerate) _removeTx(parent.GetTx().GetHash(), /* inBlock = */true);
```
This means parents that are confirmed from previous blocks are going to be removed as well right? not just parents from this block we are processing?
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252358645)
```c++
if (parent_feerate < individual_feerate) _removeTx(parent.GetTx().GetHash(), /* inBlock = */true);
```
This means parents that are confirmed from previous blocks are going to be removed as well right? not just parents from this block we are processing?
💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1620768425)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1620768425)
Concept ACK
👍 jarolrod approved a pull request: "Add menu option to migrate a wallet"
(https://github.com/bitcoin-core/gui/pull/738#pullrequestreview-1513684449)
ACK 48aae2cffeb91add75a70ac4d5075c38054452fa
This introduces the functionality as promised, works well, and didn't have any issues with legacy wallets + watch only addrs.
Improving the text and it's translatability to can occur in a follow-up.
(https://github.com/bitcoin-core/gui/pull/738#pullrequestreview-1513684449)
ACK 48aae2cffeb91add75a70ac4d5075c38054452fa
This introduces the functionality as promised, works well, and didn't have any issues with legacy wallets + watch only addrs.
Improving the text and it's translatability to can occur in a follow-up.
🤔 jarolrod reviewed a pull request: "Disable and uncheck blank when private keys are disabled"
(https://github.com/bitcoin-core/gui/pull/739#pullrequestreview-1513716076)
ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177
This is correct, and an improvement.
Visual and UX improvements on wallet creation can be handled for a follow-up here if someone wants to take it. The issue around UX of wallet creation will be tackled in https://github.com/bitcoin-core/gui-qml
(https://github.com/bitcoin-core/gui/pull/739#pullrequestreview-1513716076)
ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177
This is correct, and an improvement.
Visual and UX improvements on wallet creation can be handled for a follow-up here if someone wants to take it. The issue around UX of wallet creation will be tackled in https://github.com/bitcoin-core/gui-qml
🤔 jarolrod reviewed a pull request: "Show own outputs on PSBT signing window"
(https://github.com/bitcoin-core/gui/pull/740#pullrequestreview-1513819941)
ACK 4da243ba023f2987e97fc62886c6ebc70d6ee50a
Tested for functionality, and confirmed new translation added here shows up when building translations.
(https://github.com/bitcoin-core/gui/pull/740#pullrequestreview-1513819941)
ACK 4da243ba023f2987e97fc62886c6ebc70d6ee50a
Tested for functionality, and confirmed new translation added here shows up when building translations.
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252663074)
I am not sure if I fully get what you mean. Can you give a concrete example of a future extended format that might interfere with the 0x00 + unstructured proposal? My thought was to simply reserve all non-0x00 first bytes for future use.
When adding 32 bytes of padding, do you mean that initially (in a world with only unstructured annex data), 31 of those bytes are wasted but paid for?
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252663074)
I am not sure if I fully get what you mean. Can you give a concrete example of a future extended format that might interfere with the 0x00 + unstructured proposal? My thought was to simply reserve all non-0x00 first bytes for future use.
When adding 32 bytes of padding, do you mean that initially (in a world with only unstructured annex data), 31 of those bytes are wasted but paid for?
💬 jarolrod commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1621182702)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1621182702)
Concept ACK
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252685430)
>I don’t think this CPU DoS risk, if correct, is that much an increase in DoS surface, though a MAX_ANNEX_BUDGET can be a good conservative “belt-and-suspender” DoS limits, like we have a lot in Bitcoin Core.
If it isn't that much of an increase, is it worth adding extra code for it? More code, more things to go wrong. In the end, they are also paying for this tx in sats and the computation cost only increases linearly with the number of inputs.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252685430)
>I don’t think this CPU DoS risk, if correct, is that much an increase in DoS surface, though a MAX_ANNEX_BUDGET can be a good conservative “belt-and-suspender” DoS limits, like we have a lot in Bitcoin Core.
If it isn't that much of an increase, is it worth adding extra code for it? More code, more things to go wrong. In the end, they are also paying for this tx in sats and the computation cost only increases linearly with the number of inputs.