💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2105777838)
Same here.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2105777838)
Same here.
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107326256)
Same point of using logical `and` here.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107326256)
Same point of using logical `and` here.
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107283500)
Not a bug but would prefer to have a logical `and` here instead of a bitwise one because `all_from_me` is strictly a boolean in cpp.
```
all_from_me = all_from_me && mine;
```
Ref: https://stackoverflow.com/a/24560
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107283500)
Not a bug but would prefer to have a logical `and` here instead of a bitwise one because `all_from_me` is strictly a boolean in cpp.
```
all_from_me = all_from_me && mine;
```
Ref: https://stackoverflow.com/a/24560
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107393628)
Bitwise `and` for booleans is fine in Python I guess but I find it odd to read it.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107393628)
Bitwise `and` for booleans is fine in Python I guess but I find it odd to read it.
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107371939)
```
enum isminetype : unsigned int {
ISMINE_NO = 0,
ISMINE_WATCH_ONLY = 1 << 0,
ISMINE_SPENDABLE = 1 << 1,
ISMINE_USED = 1 << 2,
ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
ISMINE_ALL_USED = ISMINE_ALL | ISMINE_USED,
ISMINE_ENUM_ELEMENTS,
};
```
`ISMINE_ALL` didn't include `ISMINE_USED`, i.e., it avoided used ones probably because it wanted to avoid reuse. Should it not be replaced with `/*avoid_reuse=*/true` otherwise it en
...
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107371939)
```
enum isminetype : unsigned int {
ISMINE_NO = 0,
ISMINE_WATCH_ONLY = 1 << 0,
ISMINE_SPENDABLE = 1 << 1,
ISMINE_USED = 1 << 2,
ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
ISMINE_ALL_USED = ISMINE_ALL | ISMINE_USED,
ISMINE_ENUM_ELEMENTS,
};
```
`ISMINE_ALL` didn't include `ISMINE_USED`, i.e., it avoided used ones probably because it wanted to avoid reuse. Should it not be replaced with `/*avoid_reuse=*/true` otherwise it en
...
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107373819)
The ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d message can include the boolean param name.
> This isminetype is not a real isminetype as it is never returned by
IsMine. This is only used for isminefilters in one function, which can
be better represented with a bool parameter.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107373819)
The ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d message can include the boolean param name.
> This isminetype is not a real isminetype as it is never returned by
IsMine. This is only used for isminefilters in one function, which can
be better represented with a bool parameter.
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107486198)
As per this earlier statement, `avoid_reuse: true` does translate to "don't use ISMINE_USED".
https://github.com/bitcoin/bitcoin/pull/32523/files#diff-69473389a98be9232528ccdef04f9fa51ce8c5558e64994e15589be924eebae3L296
```
isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
```
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107486198)
As per this earlier statement, `avoid_reuse: true` does translate to "don't use ISMINE_USED".
https://github.com/bitcoin/bitcoin/pull/32523/files#diff-69473389a98be9232528ccdef04f9fa51ce8c5558e64994e15589be924eebae3L296
```
isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
```
💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107241475)
nit in 1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: The RPC result removals should probably have a release note?
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107241475)
nit in 1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: The RPC result removals should probably have a release note?
💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107252018)
nit in https://github.com/bitcoin/bitcoin/commit/1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: Why is the backward-compat guard for `options.isObject()` needed? Seems better to remove `&& options.isObject()`?
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107252018)
nit in https://github.com/bitcoin/bitcoin/commit/1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: Why is the backward-compat guard for `options.isObject()` needed? Seems better to remove `&& options.isObject()`?
💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107489803)
1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: I don't think this is correct. Descriptor wallets can't mix watchonly and non-watchonly, but they still need this for size estimation.
At least locally when I try `fundrawtransaction "020000000001002d310100000000160014a3a5a66c36a754cb95d32f5489a6f856aa30586000000000" '{"fee_rate":3.331}'`, I get now a smaller fee result for a watch-only wallet.
I guess it would be good to add a test for this on current master or earlier in this pull, then fixup any
...
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107489803)
1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: I don't think this is correct. Descriptor wallets can't mix watchonly and non-watchonly, but they still need this for size estimation.
At least locally when I try `fundrawtransaction "020000000001002d310100000000160014a3a5a66c36a754cb95d32f5489a6f856aa30586000000000" '{"fee_rate":3.331}'`, I get now a smaller fee result for a watch-only wallet.
I guess it would be good to add a test for this on current master or earlier in this pull, then fixup any
...
💬 TheCharlatan commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2107503689)
Would it be useful if we'd have a `ReadRawBlockUndo`? Similarly to `ReadRawBlock` it could just read the data without any hash integrity checks and without an extra serialization roundtrip. Our internal serialization just skips over the coinbase, but I don't feel like that would be surfacing too much detail either.
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2107503689)
Would it be useful if we'd have a `ReadRawBlockUndo`? Similarly to `ReadRawBlock` it could just read the data without any hash integrity checks and without an extra serialization roundtrip. Our internal serialization just skips over the coinbase, but I don't feel like that would be surfacing too much detail either.
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2107515289)
I can only see 5 reported errors, however, there should be 8 in total, no?
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2107515289)
I can only see 5 reported errors, however, there should be 8 in total, no?
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2868754066)
light code review ACK 538e9ff804f8dfba6f6a808e83572fdeab145ab8
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2868754066)
light code review ACK 538e9ff804f8dfba6f6a808e83572fdeab145ab8
💬 ajtowns commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#issuecomment-2910192822)
ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5
(https://github.com/bitcoin/bitcoin/pull/32270#issuecomment-2910192822)
ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5
💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2107645746)
> I assume that if the participants are ranged, then you can't also have a range after derivation, e.g. musig(alice/\*, bob)/\* isn't allowed. Where do we check this?
> In the next commit which implements parsing. Enforcement of the descriptor constraints is usually at the parsing level as it is not possible create these classes outside of the descriptor module.
As an additional belt-and-suspenders and documentation, would a corresponding assertion (or `Assume`) in the MuSig2PubkeyProvider
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2107645746)
> I assume that if the participants are ranged, then you can't also have a range after derivation, e.g. musig(alice/\*, bob)/\* isn't allowed. Where do we check this?
> In the next commit which implements parsing. Enforcement of the descriptor constraints is usually at the parsing level as it is not possible create these classes outside of the descriptor module.
As an additional belt-and-suspenders and documentation, would a corresponding assertion (or `Assume`) in the MuSig2PubkeyProvider
...
💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2107649960)
That makes sense. I only realize now that I was talking about code paths in two different functions (`ParsePubkey` vs `ParseScript`; for the latter the `key_exp_index` argument was already passed by reference).
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2107649960)
That makes sense. I only realize now that I was talking about code paths in two different functions (`ParsePubkey` vs `ParseScript`; for the latter the `key_exp_index` argument was already passed by reference).
💬 ismaelsadeeq commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#discussion_r2107593916)
Approach NACK
This fix will just hides the failure. I believe the correct fix is to update the test to generate and mine the correct number of transactions with a fee rate that will always reliably return a fee rate.
(https://github.com/bitcoin/bitcoin/pull/32615#discussion_r2107593916)
Approach NACK
This fix will just hides the failure. I believe the correct fix is to update the test to generate and mine the correct number of transactions with a fee rate that will always reliably return a fee rate.
🤔 janb84 reviewed a pull request: "doc: Remove stale sections in dev notes"
(https://github.com/bitcoin/bitcoin/pull/32572#pullrequestreview-2868906114)
LGTM ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72
(https://github.com/bitcoin/bitcoin/pull/32572#pullrequestreview-2868906114)
LGTM ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72
💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2107711484)
Thanks @TheCharlatan!
It should indeed improve the read performance :)
Would it be OK if I would open a separate PR for adding `ReadRawBlockUndo`?
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2107711484)
Thanks @TheCharlatan!
It should indeed improve the read performance :)
Would it be OK if I would open a separate PR for adding `ReadRawBlockUndo`?
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2107728700)
Added an `Assume`.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2107728700)
Added an `Assume`.