💬 maflcko commented on pull request "code style: update .editorconfig file":
(https://github.com/bitcoin/bitcoin/pull/30877#discussion_r1756938195)
```suggestion
# .cirrus.yml, etc.
```
(https://github.com/bitcoin/bitcoin/pull/30877#discussion_r1756938195)
```suggestion
# .cirrus.yml, etc.
```
📝 willcl-ark opened a pull request: "test: re-bucket p2p_node_network_limited"
(https://github.com/bitcoin/bitcoin/pull/30879)
Locally these tests take about 20 seconds and appear in the correct bucket.
On CI runners they are taking upwards of 3 minutes which could be a contributing factor to some CI timeouts.
(https://github.com/bitcoin/bitcoin/pull/30879)
Locally these tests take about 20 seconds and appear in the correct bucket.
On CI runners they are taking upwards of 3 minutes which could be a contributing factor to some CI timeouts.
💬 willcl-ark commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2346397042)
I opened https://github.com/bitcoin/bitcoin/pull/30879 to re-bucket these tests which are taking a few minutes longer on CI than they are bucketed for. It's not much
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2346397042)
I opened https://github.com/bitcoin/bitcoin/pull/30879 to re-bucket these tests which are taking a few minutes longer on CI than they are bucketed for. It's not much
👍 pablomartin4btc approved a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2300431928)
tACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac
I was reviewing it before merge, went thru the `src` code change and the test, which are both very well documented and easy to follow.
I agree [with](https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750378079) the split of `feature_assumeutxo.py` for different scenarios required by a particular module/ area (p2p, wallet, etc.) having a base or common file too when possible.
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2300431928)
tACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac
I was reviewing it before merge, went thru the `src` code change and the test, which are both very well documented and easy to follow.
I agree [with](https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750378079) the split of `feature_assumeutxo.py` for different scenarios required by a particular module/ area (p2p, wallet, etc.) having a base or common file too when possible.
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2346405443)
Yeah, I think we keep finding small mistakes, but there still must be a large underlying problem. In all the years, I've never seen the CI go from working fine to a large chunk of tasks timing out after 2(!) hours with no apparent reason, even across completely separate infrastructures (GHA, Cirrus workers)
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2346405443)
Yeah, I think we keep finding small mistakes, but there still must be a large underlying problem. In all the years, I've never seen the CI go from working fine to a large chunk of tasks timing out after 2(!) hours with no apparent reason, even across completely separate infrastructures (GHA, Cirrus workers)
💬 l0rinc commented on something "":
(https://github.com/bitcoin/bitcoin/commit/73fe7d723084653671f2178ea1177a8627edfafa#r146619899)
nit: this introduced a lint-spelling warning:
> % test/lint/lint-spelling.py
src/ipc/protocol.h:57: neccessary ==> necessary
(https://github.com/bitcoin/bitcoin/commit/73fe7d723084653671f2178ea1177a8627edfafa#r146619899)
nit: this introduced a lint-spelling warning:
> % test/lint/lint-spelling.py
src/ipc/protocol.h:57: neccessary ==> necessary
💬 l0rinc commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1756962957)
nit: this introduced a lint-spelling warning:
> % test/lint/lint-spelling.py
src/ipc/protocol.h:57: neccessary ==> necessary
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1756962957)
nit: this introduced a lint-spelling warning:
> % test/lint/lint-spelling.py
src/ipc/protocol.h:57: neccessary ==> necessary
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2346432017)
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
The change progressed a lot from its first version and based on the enthusiasm among reviewers I'm looking forward to continuing these in follow-up PRs.
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2346432017)
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
The change progressed a lot from its first version and based on the enthusiasm among reviewers I'm looking forward to continuing these in follow-up PRs.
📝 fjahr opened a pull request: "test: Wait for local services to update in feature_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/30880)
Closes #30878
It seems like there is a race where the block is stored locally and `getblock` does not error anymore, but `ActivateBestChain` has not finished yet, so the local services are not updated yet either. Fix this by waiting for the local services to update.
(https://github.com/bitcoin/bitcoin/pull/30880)
Closes #30878
It seems like there is a race where the block is stored locally and `getblock` does not error anymore, but `ActivateBestChain` has not finished yet, so the local services are not updated yet either. Fix this by waiting for the local services to update.
💬 maflcko commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#issuecomment-2346436631)
re-review ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8
only change is a comment
(https://github.com/bitcoin/bitcoin/pull/30865#issuecomment-2346436631)
re-review ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8
only change is a comment
💬 achow101 commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2346444421)
Discussion from today's IRC meeting is that we should move forward with @davidgumberg's patch and default to false on Windows for this release.
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2346444421)
Discussion from today's IRC meeting is that we should move forward with @davidgumberg's patch and default to false on Windows for this release.
💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346447243)
(Let's merge this minimal bugfix first, and then change the output in a follow-up. Otherwise, this will be delayed and running the s390x will not be possible out of the box)
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346447243)
(Let's merge this minimal bugfix first, and then change the output in a follow-up. Otherwise, this will be delayed and running the s390x will not be possible out of the box)
🤔 pablomartin4btc reviewed a pull request: "test: Wait for local services to update in feature_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/30880#pullrequestreview-2300493751)
ACK d823f59c91cb00fe763b0c036454440f1948bf48
Perhaps also at this line L701 too:
`assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])`
(It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)
(https://github.com/bitcoin/bitcoin/pull/30880#pullrequestreview-2300493751)
ACK d823f59c91cb00fe763b0c036454440f1948bf48
Perhaps also at this line L701 too:
`assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])`
(It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)
💬 fjahr commented on pull request "test: Wait for local services to update in feature_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346457532)
> Perhaps also at this line L701 too:
>
> `assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])`
>
> (It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)
Yeah, I just saw that after I succeeded to reproduce it locally, so don't merge yet please.
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346457532)
> Perhaps also at this line L701 too:
>
> `assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])`
>
> (It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)
Yeah, I just saw that after I succeeded to reproduce it locally, so don't merge yet please.
👍 fanquake approved a pull request: "build: Skip secp256k1 ctime tests when tests are not being built"
(https://github.com/bitcoin/bitcoin/pull/30865#pullrequestreview-2300503583)
ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8
(https://github.com/bitcoin/bitcoin/pull/30865#pullrequestreview-2300503583)
ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8
🚀 fanquake merged a pull request: "build: Skip secp256k1 ctime tests when tests are not being built"
(https://github.com/bitcoin/bitcoin/pull/30865)
(https://github.com/bitcoin/bitcoin/pull/30865)
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756990148)
But we don't need `bool` for dirty anymore, only for freshness - so we can even get rid of the enum (moving it over to the test maybe). I'll experiment to see what the next smallest change is before this PR, we need to have tests that are more representative of what's actually possible.
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756990148)
But we don't need `bool` for dirty anymore, only for freshness - so we can even get rid of the enum (moving it over to the test maybe). I'll experiment to see what the next smallest change is before this PR, we need to have tests that are more representative of what's actually possible.
🚀 fanquake merged a pull request: "test: fix exclude parsing for functional runner"
(https://github.com/bitcoin/bitcoin/pull/30872)
(https://github.com/bitcoin/bitcoin/pull/30872)
💬 fjahr commented on pull request "test: Wait for local services to update in feature_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346486397)
Ok, `feature_assumeutxo.py` should be good now with this, ready for review.
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346486397)
Ok, `feature_assumeutxo.py` should be good now with this, ready for review.
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757005855)
9378b30774d40c8a093c69d2b1807c9eea32455a: I think you need to wrap this in `if(BUILD_TESTS)`
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757005855)
9378b30774d40c8a093c69d2b1807c9eea32455a: I think you need to wrap this in `if(BUILD_TESTS)`