💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745889618)
I think it's clearer to name this `p2p_headers_presync` as well as the file.
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745889618)
I think it's clearer to name this `p2p_headers_presync` as well as the file.
💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745881799)
won't this always build off genesis?
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745881799)
won't this always build off genesis?
💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745887683)
I modified this harness to use `REGTEST` chainparams with a minimum chainwork setting of `0x0` and the harness has yet to fail, fwiw.
Might need to make sure this harness is getting deep enough to actually check what's being intended?
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745887683)
I modified this harness to use `REGTEST` chainparams with a minimum chainwork setting of `0x0` and the harness has yet to fail, fwiw.
Might need to make sure this harness is getting deep enough to actually check what's being intended?
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745913307)
Thanks, took this.
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745913307)
Thanks, took this.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-2332274663)
Thanks @glozow for the test improvements. I've also made a change to an RBF limit that I think is worth mentioning. Rather than limit the number of direct conflicts that a transaction can have, I've now implemented the limit to be on the number of distinct clusters that the conflicts of a transaction belong to.
The idea is still to create some bound on the amount of CPU we might spend linearizing clusters and doing feerate diagram checks when processing a single transaction. Limiting the n
...
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-2332274663)
Thanks @glozow for the test improvements. I've also made a change to an RBF limit that I think is worth mentioning. Rather than limit the number of direct conflicts that a transaction can have, I've now implemented the limit to be on the number of distinct clusters that the conflicts of a transaction belong to.
The idea is still to create some bound on the amount of CPU we might spend linearizing clusters and doing feerate diagram checks when processing a single transaction. Limiting the n
...
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745941441)
is f94c546ac02ad11ba73174cb8d25220515cee893 the first commit that you can assert optimality? I'm not sure having the benchmark in prior to that is so meaningful
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745941441)
is f94c546ac02ad11ba73174cb8d25220515cee893 the first commit that you can assert optimality? I'm not sure having the benchmark in prior to that is so meaningful
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745945489)
also seems like the assertion fails for me at that commit
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745945489)
also seems like the assertion fails for me at that commit
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745962506)
Nice catch. Apparently the example graphs were obtained by finding 100k-1M iterations-optimal clusters *with LIMO* (so while passing in an already-good linearization as input), while the benchmarks didn't pass in an existing linearization.
@sdaftuar is going to construct new ones.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745962506)
Nice catch. Apparently the example graphs were obtained by finding 100k-1M iterations-optimal clusters *with LIMO* (so while passing in an already-good linearization as input), while the benchmarks didn't pass in an existing linearization.
@sdaftuar is going to construct new ones.
👍 ryanofsky approved a pull request: "test: Pin and document TEST_DIR_PATH_ELEMENT, SeedRand::FIXED_SEED"
(https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
IMO, `TEST_DIR_PATH_ELEMENT` would be clearer as `TESTS_DIR_NAME` and `"test_common bitcoin"` would be clearer as `"bitcoin tests"` but current names seem fine too.
(https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
IMO, `TEST_DIR_PATH_ELEMENT` would be clearer as `TESTS_DIR_NAME` and `"test_common bitcoin"` would be clearer as `"bitcoin tests"` but current names seem fine too.
💬 ryanofsky commented on pull request "refactor: Avoid wallet code writing node settings file":
(https://github.com/bitcoin/bitcoin/pull/22217#issuecomment-2332366356)
This PR caused an unintended change in behavior. Before this PR, passing `-nowallet=0`would cause a wallet "1" to be loaded. After this PR, it triggers a vague "JSON value of type bool is not of expected type string" error and uncaught exception. #30684 improves this by avoiding the exception and making the error clearer.
(https://github.com/bitcoin/bitcoin/pull/22217#issuecomment-2332366356)
This PR caused an unintended change in behavior. Before this PR, passing `-nowallet=0`would cause a wallet "1" to be loaded. After this PR, it triggers a vague "JSON value of type bool is not of expected type string" error and uncaught exception. #30684 improves this by avoiding the exception and making the error clearer.
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745986533)
These alternatives work like a simple state machine, close to how we would look at the code: are we inside a format (i.e. started with %) or outside, switching states between the two. The second suggestion can even be used to determine is the number of params is more or if it's less than the desired one.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745986533)
These alternatives work like a simple state machine, close to how we would look at the code: are we inside a format (i.e. started with %) or outside, switching states between the two. The second suggestion can even be used to determine is the number of params is more or if it's less than the desired one.
📝 hebasto converted_to_draft a pull request: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.
Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.
Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745999190)
While the code is slightly simpler (-4 lines, indentation), the error message in case of failure is also simpler:
<details>
<summary>with static assert</summary>
```
bitcoin/src/test/util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~
bitcoin/src/util/string.h:39:34: note: subexpression not valid in a constant expression
if (num_params != count) throw "Format specifier count must
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745999190)
While the code is slightly simpler (-4 lines, indentation), the error message in case of failure is also simpler:
<details>
<summary>with static assert</summary>
```
bitcoin/src/test/util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~
bitcoin/src/util/string.h:39:34: note: subexpression not valid in a constant expression
if (num_params != count) throw "Format specifier count must
...
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1746006077)
It is not "extra", but required. Consider this output:
```
C:\Users\hebasto>where echo
INFO: Could not find files for the given pattern(s).
C:\Users\hebasto>where cmd
C:\Windows\System32\cmd.exe
```
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1746006077)
It is not "extra", but required. Consider this output:
```
C:\Users\hebasto>where echo
INFO: Could not find files for the given pattern(s).
C:\Users\hebasto>where cmd
C:\Windows\System32\cmd.exe
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332421427)
> I'm also pretty hesitant to guess what people will find useful in general, e.g., if they're worried about third party replacement cycling, they can be more cautious. The can also include a tapscript branch for anyonecanspend cleanup, just like LN does today!
Seems reasonable to want keyed. Mostly wanted the rationale to be written up here.
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332421427)
> I'm also pretty hesitant to guess what people will find useful in general, e.g., if they're worried about third party replacement cycling, they can be more cautious. The can also include a tapscript branch for anyonecanspend cleanup, just like LN does today!
Seems reasonable to want keyed. Mostly wanted the rationale to be written up here.
🤔 ryanofsky reviewed a pull request: "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2283809237)
Just leaving some notes in case there are more settings to the chain api later.
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2283809237)
Just leaving some notes in case there are more settings to the chain api later.
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1745999348)
IMO it's not great that this new API allows setting null values when previous API did not. Allowing null values to be set could encourage setting null values, which could encourage using null values, and treating them differently than missing values, which is probably not a good outcome. (Or even if it is good, is something that should be thought about more clearly, not just introduced in a bugfix.)
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1745999348)
IMO it's not great that this new API allows setting null values when previous API did not. Allowing null values to be set could encourage setting null values, which could encourage using null values, and treating them differently than missing values, which is probably not a good outcome. (Or even if it is good, is something that should be thought about more clearly, not just introduced in a bugfix.)
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746016362)
Agree this would be an improvement. But I think it would be better to change behavior and avoid writing null settings (for reasons in other comment). Would probably write as:
```c++
if (auto* value = common::FindKey(settings.rw_settings, name)) {
action = update_settings_func(*value);
if (value->isNull()) settings.rw_settings.erase(name);
} else {
UniValue value;
action = update_settings_func(*value);
if (!value.isNull()) settings.rw_settings[name] = std::move(value
...
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746016362)
Agree this would be an improvement. But I think it would be better to change behavior and avoid writing null settings (for reasons in other comment). Would probably write as:
```c++
if (auto* value = common::FindKey(settings.rw_settings, name)) {
action = update_settings_func(*value);
if (value->isNull()) settings.rw_settings.erase(name);
} else {
UniValue value;
action = update_settings_func(*value);
if (!value.isNull()) settings.rw_settings[name] = std::move(value
...
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806)
Agree rvalue reference would be an improvement, less for the reason that it would make the function safer to use, and more for the reason that it would make the function more convenient to use, because lvalue references cannot bind to temporary or literal values.
But even better than an rvalue reference would just be a plain value like `common::SettingsValue value` because this would allow the caller to decide whether they want to move or copy, instead of forcing the caller to always move.
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806)
Agree rvalue reference would be an improvement, less for the reason that it would make the function safer to use, and more for the reason that it would make the function more convenient to use, because lvalue references cannot bind to temporary or literal values.
But even better than an rvalue reference would just be a plain value like `common::SettingsValue value` because this would allow the caller to decide whether they want to move or copy, instead of forcing the caller to always move.
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746018681)
Seems like as long as `SettingsAction` enum exists this function and deleteRwSettings below should probably take `SettingsAction action` parameters instead of `bool write` parameters to be more consistent and explicit.
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746018681)
Seems like as long as `SettingsAction` enum exists this function and deleteRwSettings below should probably take `SettingsAction action` parameters instead of `bool write` parameters to be more consistent and explicit.