👍 hebasto approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2300357639)
ACK 253a691441ddb04eac2b318112064e69f4b837b7.
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2300357639)
ACK 253a691441ddb04eac2b318112064e69f4b837b7.
📝 hebasto converted_to_draft a pull request: "build: Improve `ccache` performance for different build directories"
(https://github.com/bitcoin/bitcoin/pull/30861)
This PR makes it possible to reuse the `ccache` cache populated during a build in another build directory:
```
$ cmake -B build_1 -DWITH_CCACHE=ON
$ cmake --build build_1 -t bitcoind
$ cmake -B build_2 -DWITH_CCACHE=ON
$ cmake --build build_2 -t bitcoind # 100% cache hit rate is expected.
```
The first commit has been picked from https://github.com/bitcoin/bitcoin/pull/30803.
Please refer to https://github.com/bitcoin/bitcoin/pull/20353 for historical context.
Addresses this [com
...
(https://github.com/bitcoin/bitcoin/pull/30861)
This PR makes it possible to reuse the `ccache` cache populated during a build in another build directory:
```
$ cmake -B build_1 -DWITH_CCACHE=ON
$ cmake --build build_1 -t bitcoind
$ cmake -B build_2 -DWITH_CCACHE=ON
$ cmake --build build_2 -t bitcoind # 100% cache hit rate is expected.
```
The first commit has been picked from https://github.com/bitcoin/bitcoin/pull/30803.
Please refer to https://github.com/bitcoin/bitcoin/pull/20353 for historical context.
Addresses this [com
...
👍 hodlinator approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300417228)
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
`git range-diff master fae8c25 fa5bc45`
Only minor comment adjustments and clearer variable names since last reviewed commits.
Since GitHub is [troublesome](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561525), would prefer this change if you re-touch:
```diff
- PassFmt<129>("%129$s 999$s %2$s");
+ PassFmt<12>("%12$s 999$s %2$s");
```
Makes it more reasonable to add comparison to tinyformat in follow-up with 12 arg
...
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300417228)
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
`git range-diff master fae8c25 fa5bc45`
Only minor comment adjustments and clearer variable names since last reviewed commits.
Since GitHub is [troublesome](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561525), would prefer this change if you re-touch:
```diff
- PassFmt<129>("%129$s 999$s %2$s");
+ PassFmt<12>("%12$s 999$s %2$s");
```
Makes it more reasonable to add comparison to tinyformat in follow-up with 12 arg
...
👍 TheCharlatan approved a pull request: "code style: update .editorconfig file"
(https://github.com/bitcoin/bitcoin/pull/30877#pullrequestreview-2300418237)
ACK e118dde75ad3a8e08051b4189e1e75c172a98043
(https://github.com/bitcoin/bitcoin/pull/30877#pullrequestreview-2300418237)
ACK e118dde75ad3a8e08051b4189e1e75c172a98043
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2346382089)
macOS also seems to time out: https://github.com/bitcoin/bitcoin/actions/runs/10828462406/job/30043878794 on GHA
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2346382089)
macOS also seems to time out: https://github.com/bitcoin/bitcoin/actions/runs/10828462406/job/30043878794 on GHA
👍 maflcko approved a pull request: "code style: update .editorconfig file"
(https://github.com/bitcoin/bitcoin/pull/30877#pullrequestreview-2300423885)
I don't use it, so I don't care, but the changes look plausible
lgtm
(https://github.com/bitcoin/bitcoin/pull/30877#pullrequestreview-2300423885)
I don't use it, so I don't care, but the changes look plausible
lgtm
💬 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.