💬 sipa commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631134852)
This is clearly an issue; we should not be inferring descriptors that the code itself doesn't accept back.
On the other hand, maybe we should just permit `addr()` and `raw()` inside sh/wsh/tr. There is discussion about that in #24114 too.
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631134852)
This is clearly an issue; we should not be inferring descriptors that the code itself doesn't accept back.
On the other hand, maybe we should just permit `addr()` and `raw()` inside sh/wsh/tr. There is discussion about that in #24114 too.
🚀 ryanofsky merged a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044)
(https://github.com/bitcoin/bitcoin/pull/28044)
💬 sipa commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1260003229)
Coding style nit: if you have more than a single line `if` statement, you must use braces and indentation.
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1260003229)
Coding style nit: if you have more than a single line `if` statement, you must use braces and indentation.
💬 furszy commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631222345)
> On the other hand, maybe we should just permit `addr()` and `raw()` inside sh/wsh/tr. There is discussion about that in #24114 too.
Nice, I was asking myself the same question.
I actually started implementing this as a custom, sort of dummy, pubkey provider which had no knowledge of its keys, only containing the key ids, inside the `PKHDescriptor`. So it mapped to the original `sh(pkh(key_id))`. But.. I ended up preferring this approach for the "controversy" that the other one could had.
...
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631222345)
> On the other hand, maybe we should just permit `addr()` and `raw()` inside sh/wsh/tr. There is discussion about that in #24114 too.
Nice, I was asking myself the same question.
I actually started implementing this as a custom, sort of dummy, pubkey provider which had no knowledge of its keys, only containing the key ids, inside the `PKHDescriptor`. So it mapped to the original `sh(pkh(key_id))`. But.. I ended up preferring this approach for the "controversy" that the other one could had.
...
💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1260062800)
thx, fixed
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1260062800)
thx, fixed
🤔 mzumsande reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1524806674)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1524806674)
Concept ACK
💬 mzumsande commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260065112)
Just wanted to note that this proposal has some history, see Issue #13477 and the two closed PRs: #13490 and #13713.
In particular, comment https://github.com/bitcoin/bitcoin/pull/13490#issuecomment-398069237 still seems relevant:
The expectation that -stopatheight stops exactly at the specified height relies on `ActivateBestChainStep()` only connecting a single block in each invocation, which is currently the case, but more
of a coincidence due to not wanting to lock `cs_main` for too lon
...
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260065112)
Just wanted to note that this proposal has some history, see Issue #13477 and the two closed PRs: #13490 and #13713.
In particular, comment https://github.com/bitcoin/bitcoin/pull/13490#issuecomment-398069237 still seems relevant:
The expectation that -stopatheight stops exactly at the specified height relies on `ActivateBestChainStep()` only connecting a single block in each invocation, which is currently the case, but more
of a coincidence due to not wanting to lock `cs_main` for too lon
...
🤔 MarcoFalke reviewed a pull request: "Add support for "partial" fuzzers that indicate usefulness"
(https://github.com/bitcoin/bitcoin/pull/27552#pullrequestreview-1524835460)
lgtm, but would be good to test this before merge
(https://github.com/bitcoin/bitcoin/pull/27552#pullrequestreview-1524835460)
lgtm, but would be good to test this before merge
💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260091062)
I think this is intentional to collect fuzz inputs that fail `SanityCheckASMap` into the qa-assets directory.
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260091062)
I think this is intentional to collect fuzz inputs that fail `SanityCheckASMap` into the qa-assets directory.
💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260083660)
This will discard all cases where `miniscript::FromScript` fails? This seems undesirable, because then someone can change the code to add undefined behavior or a crash in code paths that return an error.
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260083660)
This will discard all cases where `miniscript::FromScript` fails? This seems undesirable, because then someone can change the code to add undefined behavior or a crash in code paths that return an error.
💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260084752)
Same?
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260084752)
Same?
💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260089383)
style nit: May be better to use a switch-case to avoid missing a case, when a new value is added (unlikely)?
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260089383)
style nit: May be better to use a switch-case to avoid missing a case, when a new value is added (unlikely)?
💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260087559)
```suggestion
UNINTERESTING,
```
Style nit: Missing comma to avoid having to touch this line again if a new value is added (unlikely).
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260087559)
```suggestion
UNINTERESTING,
```
Style nit: Missing comma to avoid having to touch this line again if a new value is added (unlikely).
💬 murchandamus commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1631267324)
> 1. But yes, the larger the spend the more would be revealed, so it's not ideal.
Not only that, but if you spend a UTXO that’s the change from the previous transaction with the output of the next, that’s one thing. But if you spend an output with the change from fifteen transactions prior, you may heavily imply that all payments in between were also self-sends.
> * Another option would be to provide a user parameter for "likely spendable size", which we use in the chunking decision and ma
...
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1631267324)
> 1. But yes, the larger the spend the more would be revealed, so it's not ideal.
Not only that, but if you spend a UTXO that’s the change from the previous transaction with the output of the next, that’s one thing. But if you spend an output with the change from fifteen transactions prior, you may heavily imply that all payments in between were also self-sends.
> * Another option would be to provide a user parameter for "likely spendable size", which we use in the chunking decision and ma
...
👍 brunoerg approved a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1524893172)
ACK bbbb996c15aef7786c3a215ed16e1913b12b0f8c
lgtm! running `./test/fuzz/test_runner.py corpus process_message -g` on this PR,`target` becomes:
```sh
[('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'version'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'verack'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addr'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'sendaddrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'inv'
...
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1524893172)
ACK bbbb996c15aef7786c3a215ed16e1913b12b0f8c
lgtm! running `./test/fuzz/test_runner.py corpus process_message -g` on this PR,`target` becomes:
```sh
[('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'version'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'verack'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addr'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'sendaddrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'inv'
...
💬 MarcoFalke commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631312519)
> See https://github.com/theuni/bitcoin-tidy-experiments/pull/1 for an example of the kind of synchronization problems we'll have otherwise.
Wouldn't it be easier to (have an option to) self-sanity-check the match? For example, assert that more than one result was found?
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631312519)
> See https://github.com/theuni/bitcoin-tidy-experiments/pull/1 for an example of the kind of synchronization problems we'll have otherwise.
Wouldn't it be easier to (have an option to) self-sanity-check the match? For example, assert that more than one result was found?
💬 MarcoFalke commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1631316656)
PR description needs update?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1631316656)
PR description needs update?
💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260132300)
I don't actually want to make any new guarantees here. I'm just trying to make a simplification to the `ActivateBestChain` function and document the resulting behavior in release notes. Maybe the following release note would be better?
- The `-stopatheight` option now stops earlier and no longer tries to validate and connect all the downloaded blocks above the specified height on the most-work chain. This means the resulting chain is more likely to be exactly the specified height instead of l
...
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260132300)
I don't actually want to make any new guarantees here. I'm just trying to make a simplification to the `ActivateBestChain` function and document the resulting behavior in release notes. Maybe the following release note would be better?
- The `-stopatheight` option now stops earlier and no longer tries to validate and connect all the downloaded blocks above the specified height on the most-work chain. This means the resulting chain is more likely to be exactly the specified height instead of l
...
💬 murchandamus commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260141297)
How about something along these lines?
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The transaction’s outputs specified as key-value pairs.\n"
"Payment outputs are specified in the form of \"address\":\"amount [BTC]\", e.g. \"bc1p…xyz\":\"0.001\".\n"
"A single data output may be specified in the form of \"data\":\"hex\", e.g. \"data\":\"ff00ff00ff\".\n"
"Each key may only appear once. This means that each ad
...
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260141297)
How about something along these lines?
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The transaction’s outputs specified as key-value pairs.\n"
"Payment outputs are specified in the form of \"address\":\"amount [BTC]\", e.g. \"bc1p…xyz\":\"0.001\".\n"
"A single data output may be specified in the form of \"data\":\"hex\", e.g. \"data\":\"ff00ff00ff\".\n"
"Each key may only appear once. This means that each ad
...
💬 murchandamus commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260158337)
How about:
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
```
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260158337)
How about:
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
```