💬 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']}"])
```
💬 hebasto commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1825031652)
I wouldn't expect that all packages have the same names across Ubuntu/Debian and Homebrew repositories.
I agree that a macOS-specific section can be added here, but this is not a goal of this PR.
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1825031652)
I wouldn't expect that all packages have the same names across Ubuntu/Debian and Homebrew repositories.
I agree that a macOS-specific section can be added here, but this is not a goal of this PR.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825032783)
Ah, it's because at this point, the `Apply()` function doesn't use the ancestors anyway. Agreed that this is pretty ugly -- I'll try to fix.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825032783)
Ah, it's because at this point, the `Apply()` function doesn't use the ancestors anyway. Agreed that this is pretty ugly -- I'll try to fix.
💬 Sjors commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2450633698)
I added commits to drop `getTransactionsUpdated()` and `testBlockValidity()` as well.
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2450633698)
I added commits to drop `getTransactionsUpdated()` and `testBlockValidity()` as well.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996376)
Thanks, great suggestion. I’ve adopted that change.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996376)
Thanks, great suggestion. I’ve adopted that change.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996009)
Thanks, great, taken!
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996009)
Thanks, great, taken!
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825015274)
Yes, this tests that BnB will fail to find the best solution when the attempts exceed the `TOTAL_TRIES` limit of 100,000. This test follows the same approach but is slightly different than the one in the original tests. I wanted to replace the old test because I found it too difficult to parse, but I came up with my own way of testing it. I have expanded the explanation of what is going on here, and how I created this artificial example that manages to exhaust the limit with only 18 UTXOs.—To be
...
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825015274)
Yes, this tests that BnB will fail to find the best solution when the attempts exceed the `TOTAL_TRIES` limit of 100,000. This test follows the same approach but is slightly different than the one in the original tests. I wanted to replace the old test because I found it too difficult to parse, but I came up with my own way of testing it. I have expanded the explanation of what is going on here, and how I created this artificial example that manages to exhaust the limit with only 18 UTXOs.—To be
...
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825044678)
I added a longer explanation to this test. Luckily 18 is not generally the limeit, it’s just the limit in this artificially crafted case where all of our UTXOs have very similar same amounts and we have to pick eight of them to meet the target. Please let me know if it makes sense now! :)
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825044678)
I added a longer explanation to this test. Luckily 18 is not generally the limeit, it’s just the limit in this artificially crafted case where all of our UTXOs have very similar same amounts and we have to pick eight of them to meet the target. Please let me know if it makes sense now! :)
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825000480)
Yeah, as explained, the intention is to rewrite all coinselection tests with transactions that could be encountered in the wild rather than the artificial zero-fee transactions the old tests are using.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825000480)
Yeah, as explained, the intention is to rewrite all coinselection tests with transactions that could be encountered in the wild rather than the artificial zero-fee transactions the old tests are using.
🤔 murchandamus reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2408805041)
Thanks @furszy, @nymius, @jasonribble, and @vicariousdrama for the review!
I updated the commit message names as suggested, adopted all of your proposed changes as indicated, and added more explanation to the test covering the attempt limit.
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2408805041)
Thanks @furszy, @nymius, @jasonribble, and @vicariousdrama for the review!
I updated the commit message names as suggested, adopted all of your proposed changes as indicated, and added more explanation to the test covering the attempt limit.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825047813)
Thanks, I’ll make a note to update the benchmark as well.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825047813)
Thanks, I’ll make a note to update the benchmark as well.