💬 ismaelsadeeq commented on issue "RFC: Should node Wallet Startup Options Apply to Individual Wallets?":
(https://github.com/bitcoin/bitcoin/issues/32462#issuecomment-2877356452)
I share the same opinion. I also noticed @murchandamus gave a thumbs up, which I interpret as agreement. Therefore, I’ll resolve the comment in https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274 and close this issue.
As mentioned, if there’s a use case for wanting different wallets to have different values, support for that can be added in another PR, and the rationale can be documented in this issue for better tracking.
(https://github.com/bitcoin/bitcoin/issues/32462#issuecomment-2877356452)
I share the same opinion. I also noticed @murchandamus gave a thumbs up, which I interpret as agreement. Therefore, I’ll resolve the comment in https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274 and close this issue.
As mentioned, if there’s a use case for wanting different wallets to have different values, support for that can be added in another PR, and the rationale can be documented in this issue for better tracking.
💬 andrewtoth commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2877371202)
> `generateblock` is thought to be used always with a given set of txs and that may not always be the use case. Maybe would be worth to implement it on both RPC?
> you may want to mine txs from the mempool and not care about the exact amount of sats to distribute. Maybe implement this on both RPC is the solution to it.
> What do you think?
I think you are making the case to add a feature to `generateblock` to mine the mempool and collect rewards the same as `generateto` commands. Perhap
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2877371202)
> `generateblock` is thought to be used always with a given set of txs and that may not always be the use case. Maybe would be worth to implement it on both RPC?
> you may want to mine txs from the mempool and not care about the exact amount of sats to distribute. Maybe implement this on both RPC is the solution to it.
> What do you think?
I think you are making the case to add a feature to `generateblock` to mine the mempool and collect rewards the same as `generateto` commands. Perhap
...
💬 vicjuma commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877401817)
This PR improves the usability of the `listdescriptors true` RPC, especially for descriptor-based (modern) wallets where some private keys may be missing.
However, it's unclear why someone would create a watch-only wallet and still choose to set `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors` instead of `bitcoin-cli -named listdescriptors private=true`. The former already shows public keys? Unless the user is not certain about the nature of the descriptors
...
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877401817)
This PR improves the usability of the `listdescriptors true` RPC, especially for descriptor-based (modern) wallets where some private keys may be missing.
However, it's unclear why someone would create a watch-only wallet and still choose to set `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors` instead of `bitcoin-cli -named listdescriptors private=true`. The former already shows public keys? Unless the user is not certain about the nature of the descriptors
...
💬 achow101 commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877413827)
Concept NACK
The purpose of the `private` argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877413827)
Concept NACK
The purpose of the `private` argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.
💬 achow101 commented on pull request "doc: update CWallet::SignTransaction doc to mention SIGHASH_DEFAULT":
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877418091)
DEFAULT and ALL are effectively the same for Taproot inputs, and DEFAULT is an alias for ALL for non-taproot inputs. I don't think that this change is meaningfully useful as ALL and DEFAULT are essentially aliases for each other, and mean basically the same thing.
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877418091)
DEFAULT and ALL are effectively the same for Taproot inputs, and DEFAULT is an alias for ALL for non-taproot inputs. I don't think that this change is meaningfully useful as ALL and DEFAULT are essentially aliases for each other, and mean basically the same thing.
💬 vicjuma commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877434573)
Concept NACK
It's unclear why someone should receive public keys with `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors`, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877434573)
Concept NACK
It's unclear why someone should receive public keys with `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors`, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.
👍 pinheadmz approved a pull request: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2837564813)
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
Reviewed all commits and tested on macos/arm64. Compared `test_runner --usecli` on master to branch. Master failed 17 tests and skipped 58. This branch skipped 36 and failed none. Some of the skipped tests are due to my platform.
This PR touches test code only but also by adding CI coverage requires all future changes to the API to be bitcoin-cli compatible (or otherwise pass new tests with cli)
<details><summary>Show Signature</summary>
...
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2837564813)
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
Reviewed all commits and tested on macos/arm64. Compared `test_runner --usecli` on master to branch. Master failed 17 tests and skipped 58. This branch skipped 36 and failed none. Some of the skipped tests are due to my platform.
This PR touches test code only but also by adding CI coverage requires all future changes to the API to be bitcoin-cli compatible (or otherwise pass new tests with cli)
<details><summary>Show Signature</summary>
...
💬 pinheadmz commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087318320)
Is this only ever needed for passing a hash as a hash-or-height argument?
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087318320)
Is this only ever needed for passing a hash as a hash-or-height argument?
💬 pinheadmz commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087299133)
959dc33283be3c465476f604263f7a2de4007da0
Why not add this method to `BitcoinTestFramework` then you won't need to pass `self.options.usecli` every call?
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087299133)
959dc33283be3c465476f604263f7a2de4007da0
Why not add this method to `BitcoinTestFramework` then you won't need to pass `self.options.usecli` every call?
💬 rkrux commented on pull request "doc: update CWallet::SignTransaction doc to mention SIGHASH_DEFAULT":
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877457566)
True. The main reason for this change was that I didn't see any benefit in leaving ALL around in the function documentation when the implementation uses DEFAULT.
I am open to closing this PR later if this doesn't seem like a strong enough reason.
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877457566)
True. The main reason for this change was that I didn't see any benefit in leaving ALL around in the function documentation when the implementation uses DEFAULT.
I am open to closing this PR later if this doesn't seem like a strong enough reason.
💬 achow101 commented on pull request "test: fix two intermittent failures in wallet_basic.py":
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2877470966)
ACK 4290a86c6022c8f63610b9c1b185ac115cefbd2c
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2877470966)
ACK 4290a86c6022c8f63610b9c1b185ac115cefbd2c
💬 theuni commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2087368590)
`HashWriter ss{}` in the if() branch below now shadows this one. Rename to avoid confusion (and warnings) ?
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2087368590)
`HashWriter ss{}` in the if() branch below now shadows this one. Rename to avoid confusion (and warnings) ?
🤔 naiyoma reviewed a pull request: "test: Fix feature_pruning test after nTime typo fix"
(https://github.com/bitcoin/bitcoin/pull/32312#pullrequestreview-2837686646)
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
I don't think this is a reorg issue. I believe the test fails because node 1 invalidates blocks in `reorg_test `-> https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_pruning.py#L179.
So basically, when node 2 tries to getblockfrompeer from node 1, node 1 has already invalidated the block we're trying to fetch.
Node 2 and Node 5 are on the same chain tip even after Node 2 has been reorged. The test passes based on whether or no
...
(https://github.com/bitcoin/bitcoin/pull/32312#pullrequestreview-2837686646)
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
I don't think this is a reorg issue. I believe the test fails because node 1 invalidates blocks in `reorg_test `-> https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_pruning.py#L179.
So basically, when node 2 tries to getblockfrompeer from node 1, node 1 has already invalidated the block we're trying to fetch.
Node 2 and Node 5 are on the same chain tip even after Node 2 has been reorged. The test passes based on whether or no
...
💬 achow101 commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087371964)
In e66e9ee9f19413c93d8523afdb8f9f3e07b818ec "wallet, refactor: Remove Legacy warnings and errors"
I'd prefer to at least preserve enforcement that the wallet being created is a descriptor wallet. Perhaps we could add an `Assert()` or `Assume()` for the flag.
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2087371964)
In e66e9ee9f19413c93d8523afdb8f9f3e07b818ec "wallet, refactor: Remove Legacy warnings and errors"
I'd prefer to at least preserve enforcement that the wallet being created is a descriptor wallet. Perhaps we could add an `Assert()` or `Assume()` for the flag.
💬 maflcko commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2877505784)
lgtm ACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2877505784)
lgtm ACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
💬 rkrux commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877510396)
> If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors.
I have not checked this PR yet but the original intention came from this issue: https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475
The idea was to show results for descriptors that don't have _all_ the private keys but do have at least one. This is not for the case when the descriptor doesn't have _any_ private key.
I do like how th
...
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877510396)
> If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors.
I have not checked this PR yet but the original intention came from this issue: https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475
The idea was to show results for descriptors that don't have _all_ the private keys but do have at least one. This is not for the case when the descriptor doesn't have _any_ private key.
I do like how th
...
💬 instagibbs commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877519744)
@darosior just noting the tests are specifically written to not worry about standardness, so at least these would have to be augmented to cover the cases this PR is ostensibly supporting
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877519744)
@darosior just noting the tests are specifically written to not worry about standardness, so at least these would have to be augmented to cover the cases this PR is ostensibly supporting
🤔 stratospher reviewed a pull request: "test: Fix feature_pruning test after nTime typo fix"
(https://github.com/bitcoin/bitcoin/pull/32312#pullrequestreview-2837725653)
tested ACK 2aa63d5. verified that `nTime` is being incremented now.
agree with @naiyoma - this appears a "block being requested is valid/invalid in node1 problem" and not a "which node to connect to problem".
- node2 and node5 have identical chain tips (aside from differing prune heights: node2 at 1246, node5 at 774).
- on master: node2 requests block 1245 from node1. but in node1, that block is marked BLOCK_FAILED_VALID, so the request is ignored. I see this log in node1:
`ignoring requ
...
(https://github.com/bitcoin/bitcoin/pull/32312#pullrequestreview-2837725653)
tested ACK 2aa63d5. verified that `nTime` is being incremented now.
agree with @naiyoma - this appears a "block being requested is valid/invalid in node1 problem" and not a "which node to connect to problem".
- node2 and node5 have identical chain tips (aside from differing prune heights: node2 at 1246, node5 at 774).
- on master: node2 requests block 1245 from node1. but in node1, that block is marked BLOCK_FAILED_VALID, so the request is ignored. I see this log in node1:
`ignoring requ
...
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877536457)
@instagibbs Yes and no, depending on the meaning of standardness I guess. The worrisome transactions *are* non-standard (in the sense that they are not relayed/accepted), but are standard in the `IsStandard()` sense, and thus not immediately rejected before script execution even begins.
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2877536457)
@instagibbs Yes and no, depending on the meaning of standardness I guess. The worrisome transactions *are* non-standard (in the sense that they are not relayed/accepted), but are standard in the `IsStandard()` sense, and thus not immediately rejected before script execution even begins.
💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087404190)
yea I tried doing that but then I was getting less coverage for some reason not sure if I just need to let the fuzzer run longer
Without `ConsumeTransaction`
```
#657589 REDUCE cov: 813 ft: 3922 corp: 309/188Kb lim: 4096 exec/s: 8323 rss: 359Mb L: 941/3853 MS: 1 EraseBytes-
```
With `ConsumeTransaction`
```
#14901 REDUCE cov: 1461 ft: 8651 corp: 536/449Kb lim: 4096 exec/s: 3725 rss: 350Mb L: 677/4093 MS: 1 EraseBytes-
```
I can try running the fuzz test for a while and see if it
...
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087404190)
yea I tried doing that but then I was getting less coverage for some reason not sure if I just need to let the fuzzer run longer
Without `ConsumeTransaction`
```
#657589 REDUCE cov: 813 ft: 3922 corp: 309/188Kb lim: 4096 exec/s: 8323 rss: 359Mb L: 941/3853 MS: 1 EraseBytes-
```
With `ConsumeTransaction`
```
#14901 REDUCE cov: 1461 ft: 8651 corp: 536/449Kb lim: 4096 exec/s: 3725 rss: 350Mb L: 677/4093 MS: 1 EraseBytes-
```
I can try running the fuzz test for a while and see if it
...