🤔 maflcko reviewed a pull request: "ci: run test-each-commit on merge to master"
(https://github.com/bitcoin/bitcoin/pull/32103#pullrequestreview-2702829350)
Not sure I understand the goal nor the changes here.
It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.
However, you check out the previous commit, so overall this change is a no-op , unless I am missing something?
Maybe you wanted to instead *rebase* the changes onto master? However, this is not possible in general, as some commits are not possible to rebase. For example, a subtree merge commit is not possib
...
(https://github.com/bitcoin/bitcoin/pull/32103#pullrequestreview-2702829350)
Not sure I understand the goal nor the changes here.
It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.
However, you check out the previous commit, so overall this change is a no-op , unless I am missing something?
Maybe you wanted to instead *rebase* the changes onto master? However, this is not possible in general, as some commits are not possible to rebase. For example, a subtree merge commit is not possib
...
💬 maflcko commented on pull request "ci: run test-each-commit on merge to master":
(https://github.com/bitcoin/bitcoin/pull/32103#discussion_r2005694622)
wouldn't this also need to increase fetch depth?
(https://github.com/bitcoin/bitcoin/pull/32103#discussion_r2005694622)
wouldn't this also need to increase fetch depth?
💬 vasild commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2740570834)
`48a636cdde...859c259092`: fix CI
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2740570834)
`48a636cdde...859c259092`: fix CI
💬 maflcko commented on issue "test: intermittent issue in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740571721)
Sorry for missing the ping. The goal of the `test-each-commit` is to check for bisect errors on the original commits. Thus, it will be susceptible to intermittent issues that were fixed in current master.
Maybe the docs could be improved to mention this?
Other than than, I'd recommend to just rebase on master when a known/fixed issue is hit in the `test-each-commit` task, or to re-run the task.
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740571721)
Sorry for missing the ping. The goal of the `test-each-commit` is to check for bisect errors on the original commits. Thus, it will be susceptible to intermittent issues that were fixed in current master.
Maybe the docs could be improved to mention this?
Other than than, I'd recommend to just rebase on master when a known/fixed issue is hit in the `test-each-commit` task, or to re-run the task.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005731535)
> semi-related: I wonder if Windows can be bumped to Windows 10 either way.
Addressed in https://github.com/bitcoin/bitcoin/pull/31172.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005731535)
> semi-related: I wonder if Windows can be bumped to Windows 10 either way.
Addressed in https://github.com/bitcoin/bitcoin/pull/31172.
💬 maflcko commented on pull request "fuzz: Use immediate task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2740599566)
> The only downside to this is that we diverge from the production setup and e.g. bugs that only occur when fuzzing and using the `SerialTaskRunner` would be masked by this change.
Right, but I wonder what type of bug this could possibly uncover, given that the fuzz targets are mostly single threaded and require some kind of sync (like `SyncWithValidationInterfaceQueue`) when they use the task runner.
Obviously, bugs in `CScheduler` itself could be missed. Though, if we are worried about t
...
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2740599566)
> The only downside to this is that we diverge from the production setup and e.g. bugs that only occur when fuzzing and using the `SerialTaskRunner` would be masked by this change.
Right, but I wonder what type of bug this could possibly uncover, given that the fuzz targets are mostly single threaded and require some kind of sync (like `SyncWithValidationInterfaceQueue`) when they use the task runner.
Obviously, bugs in `CScheduler` itself could be missed. Though, if we are worried about t
...
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2740599986)
@ismaelsadeeq I think this just preserves existing RPC behavior. What to return during shutdown is a bit arbitrary anyway, because a connected client will have to handle shutdown more generally, potentially by also shutting down.
I also haven't checked if any of the other tests rely on the existing behavior.
So maybe better for a followup?
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2740599986)
@ismaelsadeeq I think this just preserves existing RPC behavior. What to return during shutdown is a bit arbitrary anyway, because a connected client will have to handle shutdown more generally, potentially by also shutting down.
I also haven't checked if any of the other tests rely on the existing behavior.
So maybe better for a followup?
🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2702602071)
Thanks for the reviews
Rebased 467528960689c2913c101ef75bc833e8f04bd0f3 -> 00c3e4db49ebd2c3bf2f3c7f2a8c9ba5dfb3c8f5 ([`pr/cstate.10`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.10) -> [`pr/cstate.11`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.10-rebase..pr/cstate.11)) due to conflict with #31519, also adding test comments.
Update 00c3e4db49ebd2c3bf2f3c7f2a8c9ba5dfb3c8f5 -> 137d5980abd49a1d2bf783cb5
...
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2702602071)
Thanks for the reviews
Rebased 467528960689c2913c101ef75bc833e8f04bd0f3 -> 00c3e4db49ebd2c3bf2f3c7f2a8c9ba5dfb3c8f5 ([`pr/cstate.10`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.10) -> [`pr/cstate.11`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.10-rebase..pr/cstate.11)) due to conflict with #31519, also adding test comments.
Update 00c3e4db49ebd2c3bf2f3c7f2a8c9ba5dfb3c8f5 -> 137d5980abd49a1d2bf783cb5
...
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2005575053)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004115942
> I find the entire `chainstatemanager_snapshot_init` subtest very confusing
Yes. And thanks for digging into the test. I added comments to try to make it less confusing. For reference the test was added [in e4d7995](https://github.com/bitcoin/bitcoin/pull/25667/commits/e4d799528696c5ede38c257afaffd367917e0de8#diff-dbada1fe3a3d0af884304dd28be8c9df74b592401dec2c6400f6b491aefe6c9bR472) and seems basically unchanged sin
...
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2005575053)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004115942
> I find the entire `chainstatemanager_snapshot_init` subtest very confusing
Yes. And thanks for digging into the test. I added comments to try to make it less confusing. For reference the test was added [in e4d7995](https://github.com/bitcoin/bitcoin/pull/25667/commits/e4d799528696c5ede38c257afaffd367917e0de8#diff-dbada1fe3a3d0af884304dd28be8c9df74b592401dec2c6400f6b491aefe6c9bR472) and seems basically unchanged sin
...
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2005739316)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004288056
Thanks, fixed commit message. Ultimately I think it would be best to drop the `m_target_utxohash` member and just check the `ReachedTarget()` condition everywhere instead of using `m_target_utxohash.has_value()`. But this needs to be done carefully, and it might require getting rid of some "belt and suspenders" checks in the current code which are technically unnecessary but give some confidence the code works correctly
...
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2005739316)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004288056
Thanks, fixed commit message. Ultimately I think it would be best to drop the `m_target_utxohash` member and just check the `ReachedTarget()` condition everywhere instead of using `m_target_utxohash.has_value()`. But this needs to be done carefully, and it might require getting rid of some "belt and suspenders" checks in the current code which are technically unnecessary but give some confidence the code works correctly
...
💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2005759806)
Interesting. If `Assume` can be optimized out during production possibly, is there any reason to prefer `Assert`?
There's an assert [here](https://github.com/bitcoin/bitcoin/blob/998386d4462f5e06412303ba559791da83b913fb/src/wallet/coinselection.cpp#L159) in a coin-selection algo that is meant to be performant. Maybe this should be changed to `Asume`.
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2005759806)
Interesting. If `Assume` can be optimized out during production possibly, is there any reason to prefer `Assert`?
There's an assert [here](https://github.com/bitcoin/bitcoin/blob/998386d4462f5e06412303ba559791da83b913fb/src/wallet/coinselection.cpp#L159) in a coin-selection algo that is meant to be performant. Maybe this should be changed to `Asume`.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2740633112)
`741f17e51d...f2f9ff9823`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2740633112)
`741f17e51d...f2f9ff9823`: rebase due to conflicts
💬 maflcko commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2005770365)
This could be an assume, given that the program can continue normally, albeit it is a pessimization or possible OOM on low memory machines?
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2005770365)
This could be an assume, given that the program can continue normally, albeit it is a pessimization or possible OOM on low memory machines?
💬 fjahr commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2740650390)
ACK c6eca6f3961d780d6a395c3346e44be1a0072f08
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2740650390)
ACK c6eca6f3961d780d6a395c3346e44be1a0072f08
💬 maflcko commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2005783208)
> There's an assert [here](https://github.com/bitcoin/bitcoin/blob/998386d4462f5e06412303ba559791da83b913fb/src/wallet/coinselection.cpp#L159) in a coin-selection algo that is meant to be performant. Maybe this should be changed to `Asume`.
I haven't measured this, but I'd say an assert on a value of a type of `size_t` shouldn't be noticeable in this context.
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2005783208)
> There's an assert [here](https://github.com/bitcoin/bitcoin/blob/998386d4462f5e06412303ba559791da83b913fb/src/wallet/coinselection.cpp#L159) in a coin-selection algo that is meant to be performant. Maybe this should be changed to `Asume`.
I haven't measured this, but I'd say an assert on a value of a type of `size_t` shouldn't be noticeable in this context.
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005785025)
> Otherwise, it fails to find XCB:
Is that a Qt bug? Seems odd that if we don't need it now, we'd need to reintroduce its usage after migrating to a newer version of Qt and migrating more of its build to CMake?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005785025)
> Otherwise, it fails to find XCB:
Is that a Qt bug? Seems odd that if we don't need it now, we'd need to reintroduce its usage after migrating to a newer version of Qt and migrating more of its build to CMake?
💬 maflcko commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2740670571)
lgtm ACK c6eca6f3961d780d6a395c3346e44be1a0072f08
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2740670571)
lgtm ACK c6eca6f3961d780d6a395c3346e44be1a0072f08
💬 sipa commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2740685232)
Concept ACK, this is largely how I've been using Assume().
My thinking is that due to the optimizing-out property of `Assume()`, the concern about potential performance impact is (even) lower than for `Assert()`/`assert()`, and thus one can be even more liberal when using these. Note that even `Assert()`/`assert()` tend to have very low cost for cheap conditions, as they're generally well-predicted by the CPU branch predictor (so the cost tends to be in the nanosecond or less range), but for
...
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2740685232)
Concept ACK, this is largely how I've been using Assume().
My thinking is that due to the optimizing-out property of `Assume()`, the concern about potential performance impact is (even) lower than for `Assert()`/`assert()`, and thus one can be even more liberal when using these. Note that even `Assert()`/`assert()` tend to have very low cost for cheap conditions, as they're generally well-predicted by the CPU branch predictor (so the cost tends to be in the nanosecond or less range), but for
...
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2740692162)
Rebased 7e7bcd26ad880643d21e9be081068d74c6fb0fd0 -> 5554f9cb4abf7e1852dd79d2d75e714fa8cbeadd ([`pr/mine.19`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.19) -> [`pr/mine.20`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.19-rebase..pr/mine.20)) due to conflict with #31866
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2740692162)
Rebased 7e7bcd26ad880643d21e9be081068d74c6fb0fd0 -> 5554f9cb4abf7e1852dd79d2d75e714fa8cbeadd ([`pr/mine.19`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.19) -> [`pr/mine.20`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.19-rebase..pr/mine.20)) due to conflict with #31866
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2005807278)
Adjusted in latest push, which is otherwise just a rebase.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2005807278)
Adjusted in latest push, which is otherwise just a rebase.