💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824955448)
I think I prefer letting the fuzzer pick it, so that I don't have to worry about whether there are other aspects of the transaction I'm overfitting (and thus getting worse coverage than if I let the fuzzer do all the work).
On the other hand, if there are certain transaction templates that we want to be sure are covered, then perhaps I can flip a bool and determine whether to use a pre-constructed transaction versus use one from the fuzzer.
I can't think of what that might be though, so
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824955448)
I think I prefer letting the fuzzer pick it, so that I don't have to worry about whether there are other aspects of the transaction I'm overfitting (and thus getting worse coverage than if I let the fuzzer do all the work).
On the other hand, if there are certain transaction templates that we want to be sure are covered, then perhaps I can flip a bool and determine whether to use a pre-constructed transaction versus use one from the fuzzer.
I can't think of what that might be though, so
...
💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824957314)
this is fine, feel free to resolve it
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824957314)
this is fine, feel free to resolve it
💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2450524432)
Some calls to the function have duplicated the last argument to pass the tests, but it is necessary to see how to correctly pass the ``best_header``.
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2450524432)
Some calls to the function have duplicated the last argument to pass the tests, but it is necessary to see how to correctly pass the ``best_header``.
💬 Sjors commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#issuecomment-2450525374)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31192#issuecomment-2450525374)
Concept ACK.
💬 Sjors commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824962864)
I suggest either leaving out `apt install` or also naming `brew install` and friends, to make it clear this applies to all platforms.
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824962864)
I suggest either leaving out `apt install` or also naming `brew install` and friends, to make it clear this applies to all platforms.
📝 darosior opened a pull request: "init: warn, don't error, when '-upnp' is set"
(https://github.com/bitcoin/bitcoin/pull/31198)
It prevented the GUI from starting when its settings.json had the -upnp option set. This also doesn't prevent the node from running, so this error didn't need to be fatal.
Thanks to Sjors for bringing attention to what i broke and to Maflcko for suggestion a simple short term fix.
(https://github.com/bitcoin/bitcoin/pull/31198)
It prevented the GUI from starting when its settings.json had the -upnp option set. This also doesn't prevent the node from running, so this error didn't need to be fatal.
Thanks to Sjors for bringing attention to what i broke and to Maflcko for suggestion a simple short term fix.
💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824965387)
I can't imagine it would greatly effect performance of the fuzz target as it's a single transaction, but it might matter more if more are constructed like in the rest of the harness.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824965387)
I can't imagine it would greatly effect performance of the fuzz target as it's a single transaction, but it might matter more if more are constructed like in the rest of the harness.
💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450539742)
I made a note to add `waitNewPowBlock()` or something similar to the interface, but I think it can wait for a later release. And we should probably get some performance benchmarks to see if it's even worth it. It's possible that cluster mempool updates stuff so quickly we can just wait for it.
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450539742)
I made a note to add `waitNewPowBlock()` or something similar to the interface, but I think it can wait for a later release. And we should probably get some performance benchmarks to see if it's even worth it. It's possible that cluster mempool updates stuff so quickly we can just wait for it.
💬 maflcko commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2450544709)
lgtm ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
I think the GUI will still need to be fixed either way. A warning is tolerable, but horrible UX, if the user can't do anything about it other than manually finding and editing the settings json file.
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2450544709)
lgtm ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
I think the GUI will still need to be fixed either way. A warning is tolerable, but horrible UX, if the user can't do anything about it other than manually finding and editing the settings json file.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824996488)
Now I think I may have been mistaken; I've commented out this line and started fuzzing again, and I cannot reproduce the crash. Also, I agree with your assessment after re-reading the code some more.
However, it would be nice if the SubPackageState were clearly destroyed when it goes out of scope from logic that uses it... Even though it's seemingly not necessary to call cleanup here, it makes me a little uncomfortable to leave the code in a place where it takes some investigation to figure
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1824996488)
Now I think I may have been mistaken; I've commented out this line and started fuzzing again, and I cannot reproduce the crash. Also, I agree with your assessment after re-reading the code some more.
However, it would be nice if the SubPackageState were clearly destroyed when it goes out of scope from logic that uses it... Even though it's seemingly not necessary to call cleanup here, it makes me a little uncomfortable to leave the code in a place where it takes some investigation to figure
...
💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1824994969)
Why is this clamping removed? Blocks can have a time in the future, and the changes in this pull don't change it, so it still seems possible to be hit? If not, it would be good explain why, or a unit test could be added to show that a block time in the future doesn't lead to a value larger than 1.
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1824994969)
Why is this clamping removed? Blocks can have a time in the future, and the changes in this pull don't change it, so it still seems possible to be hit? If not, it would be good explain why, or a unit test could be added to show that a block time in the future doesn't lead to a value larger than 1.
💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1824997032)
Why is this fallback reasonable? It would be good to explain why, or remove it (require the argument to be passed). An alternative would be to make the function a member method on chainman. This way the call site doesn't have to pass `hindex` every time. If `hindex` is supposed to be different for different calls, it would be good to explain why.
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1824997032)
Why is this fallback reasonable? It would be good to explain why, or remove it (require the argument to be passed). An alternative would be to make the function a member method on chainman. This way the call site doesn't have to pass `hindex` every time. If `hindex` is supposed to be different for different calls, it would be good to explain why.
💬 maflcko commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2450578651)
> I don't know how to access that, is it part of CI?
It needs to be run manually. See https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally. (`podman run --rm --privileged docker.io/multiarch/qemu-user-static --reset -p yes` may be required to setup qemu-s390x, depending on your setup). Then something like `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh` should run it.
> Does the test suite pass on it otherwise or was it just c
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2450578651)
> I don't know how to access that, is it part of CI?
It needs to be run manually. See https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally. (`podman run --rm --privileged docker.io/multiarch/qemu-user-static --reset -p yes` may be required to setup qemu-s390x, depending on your setup). Then something like `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh` should run it.
> Does the test suite pass on it otherwise or was it just c
...
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825015423)
I'm very puzzled why none of the tests appear to fail on this commit.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825015423)
I'm very puzzled why none of the tests appear to fail on this commit.
💬 TheBlueMatt commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450593825)
I think there's about four reasons to want to switch around the mining interface to be push-based (rather than the current request-based):
1) reduces (tail) latency when requesting a block. In most cases it won't matter, but immediately after finding a new block there is potentially a lot of `cs_main` contention with other peers sending us copies of the block header or peers requesting the block's transactions. Even ignoring the on-block stuff, you don't want to find yourself waiting for some
...
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450593825)
I think there's about four reasons to want to switch around the mining interface to be push-based (rather than the current request-based):
1) reduces (tail) latency when requesting a block. In most cases it won't matter, but immediately after finding a new block there is potentially a lot of `cs_main` contention with other peers sending us copies of the block header or peers requesting the block's transactions. Even ignoring the on-block stuff, you don't want to find yourself waiting for some
...
💬 sipa commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450599316)
I think it would (eventually) make sense that Bitcoin Core can decide when a new block template is generated, as it is at the source of the information that determines whether that's worth doing.
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450599316)
I think it would (eventually) make sense that Bitcoin Core can decide when a new block template is generated, as it is at the source of the information that determines whether that's worth doing.
👍 tdb3 approved a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2408783550)
Code review ACK 9173b056006fd3940675108aa3fe0d0d855af768
This seems quite useful when troubleshooting errors. Left a few nits, but nothing I feel very strong about.
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2408783550)
Code review ACK 9173b056006fd3940675108aa3fe0d0d855af768
This seems quite useful when troubleshooting errors. Left a few nits, but nothing I feel very strong about.
💬 tdb3 commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1825023128)
Maybe we can use `util.assert_raises()` or `util.assert_raises_message()` here to keep things more concise?
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1825023128)
Maybe we can use `util.assert_raises()` or `util.assert_raises_message()` here to keep things more concise?
💬 tdb3 commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1824996905)
non-blocking nit: same with these.
e.g.
```python
ignored_errors.update([errno.errorcode[e.errno]])
```
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1824996905)
non-blocking nit: same with these.
e.g.
```python
ignored_errors.update([errno.errorcode[e.errno]])
```
💬 tdb3 commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1824983303)
non-blocking nit: might be a bit more Pythonic to use Counter.update() rather than `+= 1`.
For example:
```python
ignored_errors.update([f"JSONRPCException {e.error['code']}"])
```
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1824983303)
non-blocking nit: might be a bit more Pythonic to use Counter.update() rather than `+= 1`.
For example:
```python
ignored_errors.update([f"JSONRPCException {e.error['code']}"])
```