💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711338908)
As an alternative workaround, I'd suggest to use an extra configuration option `-DAPPEND_LDFLAGS="-Wl,-no_warn_duplicate_libraries"`.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711338908)
As an alternative workaround, I'd suggest to use an extra configuration option `-DAPPEND_LDFLAGS="-Wl,-no_warn_duplicate_libraries"`.
📝 maflcko opened a pull request: "net: Clarify that m_addr_local is only set once"
(https://github.com/bitcoin/bitcoin/pull/30617)
The function is supposed to be only called once when the version msg arrives (a single time). Calling it twice would be an internal logic bug. However, the `LogError` in this function has many issues:
* If the error happens in tests, as is the case for the buggy fuzz test, it will go unnoticed
* It is dead code, unless a bug is introduced to execute it
Fix all issues by using `Assume(!m_addr_local.IsValid())` instead. Idea taken from https://github.com/bitcoin/bitcoin/pull/30364#discussio
...
(https://github.com/bitcoin/bitcoin/pull/30617)
The function is supposed to be only called once when the version msg arrives (a single time). Calling it twice would be an internal logic bug. However, the `LogError` in this function has many issues:
* If the error happens in tests, as is the case for the buggy fuzz test, it will go unnoticed
* It is dead code, unless a bug is introduced to execute it
Fix all issues by using `Assume(!m_addr_local.IsValid())` instead. Idea taken from https://github.com/bitcoin/bitcoin/pull/30364#discussio
...
💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1711345479)
Fixed in https://github.com/bitcoin/bitcoin/pull/30617, due to lack of progress here.
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1711345479)
Fixed in https://github.com/bitcoin/bitcoin/pull/30617, due to lack of progress here.
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711371725)
btw, is it expected that modifying the `try_append_linker_flag("-Wl,-fatal_warnings" VAR working_linker_werror_flag)` line and rerunning `cmake -B build_fuzz` ignores the change? Shouldn't it detect that this file was changed?
I need to `rm -rfd build_fuzz && cmake -B build_fuzz` for it to be picked up.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711371725)
btw, is it expected that modifying the `try_append_linker_flag("-Wl,-fatal_warnings" VAR working_linker_werror_flag)` line and rerunning `cmake -B build_fuzz` ignores the change? Shouldn't it detect that this file was changed?
I need to `rm -rfd build_fuzz && cmake -B build_fuzz` for it to be picked up.
💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711383749)
As a side note, the UBSan errors observed [here](https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041) are not caused by CMake. Other people have observed the same Mac specific behavior with autotools: https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2260863971.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711383749)
As a side note, the UBSan errors observed [here](https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041) are not caused by CMake. Other people have observed the same Mac specific behavior with autotools: https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2260863971.
💬 fjahr commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1711387169)
> let's define a separate constant
done
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1711387169)
> let's define a separate constant
done
💬 fjahr commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#issuecomment-2277852126)
Addressed @Sjors feedback and introduced a separate constant `MAX_TIMEWARP`.
(https://github.com/bitcoin/bitcoin/pull/30604#issuecomment-2277852126)
Addressed @Sjors feedback and introduced a separate constant `MAX_TIMEWARP`.
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711390540)
> errors observed https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041 are not caused by CMake
Yeah, my example in https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707893570 was also in Autotools.
The same happens with CMake in https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711390540)
> errors observed https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041 are not caused by CMake
Yeah, my example in https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707893570 was also in Autotools.
The same happens with CMake in https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041
💬 fjahr commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1711395593)
> And see https://github.com/bitcoin/bitcoin/issues/30614, but that should probably be its own followup.
Agree, it seems that we currently have the safer option given we don't really know what other mining software is doing (at least that's my feeling). If we do still get consensus on tightening the limit we can do that in another follow-up in the next two weeks but it shouldn't hold up this PR here.
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1711395593)
> And see https://github.com/bitcoin/bitcoin/issues/30614, but that should probably be its own followup.
Agree, it seems that we currently have the safer option given we don't really know what other mining software is doing (at least that's my feeling). If we do still get consensus on tightening the limit we can do that in another follow-up in the next two weeks but it shouldn't hold up this PR here.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711401797)
> btw, is it expected that modifying `TryAppendLinkerFlag.cmake` (e.g. the `try_append_linker_flag("-Wl,-fatal_warnings" VAR working_linker_werror_flag)` line) and rerunning `cmake -B build_fuzz` ignores the update? Shouldn't it detect that this file was changed? I need to `rm -rfd build_fuzz && cmake -B build_fuzz` for it to be picked up.
Yes, that is because the result is cached.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711401797)
> btw, is it expected that modifying `TryAppendLinkerFlag.cmake` (e.g. the `try_append_linker_flag("-Wl,-fatal_warnings" VAR working_linker_werror_flag)` line) and rerunning `cmake -B build_fuzz` ignores the update? Shouldn't it detect that this file was changed? I need to `rm -rfd build_fuzz && cmake -B build_fuzz` for it to be picked up.
Yes, that is because the result is cached.
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711403435)
Of course, but shouldn't this change invalidate the cache?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711403435)
Of course, but shouldn't this change invalidate the cache?
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711409392)
Apparently, not every change invalidates the cache. Mind reporting to https://gitlab.kitware.com/cmake/cmake/-/issues?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711409392)
Apparently, not every change invalidates the cache. Mind reporting to https://gitlab.kitware.com/cmake/cmake/-/issues?
💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777)
I think we can improve the output when `-DWITH_CCACHE=OFF` is used. Depending on the system, that output might be:
```bash
cmake -B build -DWITH_CCACHE=OFF
< snip >
Use ccache for compiling .............. ccache masquerades as the compiler
```
We should probably at least indicate that the option was respected by the build-system.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777)
I think we can improve the output when `-DWITH_CCACHE=OFF` is used. Depending on the system, that output might be:
```bash
cmake -B build -DWITH_CCACHE=OFF
< snip >
Use ccache for compiling .............. ccache masquerades as the compiler
```
We should probably at least indicate that the option was respected by the build-system.
🤔 theStack reviewed a pull request: "depends: Make default `host` and `build` comparable"
(https://github.com/bitcoin/bitcoin/pull/30584#pullrequestreview-2230130108)
@hebasto: Ok, I can confirm that with OpenBSD 7.5 on master, the host/build values differ:
```
$ gmake print-host
host=amd64-unknown-openbsd7.5
$ gmake print-build
build=x86_64-unknown-openbsd7.5
```
while on the PR branch they are equal :heavy_check_mark:
```
$ gmake print-host
host=x86_64-unknown-openbsd7.5
$ gmake print-build
build=x86_64-unknown-openbsd7.5
```
(https://github.com/bitcoin/bitcoin/pull/30584#pullrequestreview-2230130108)
@hebasto: Ok, I can confirm that with OpenBSD 7.5 on master, the host/build values differ:
```
$ gmake print-host
host=amd64-unknown-openbsd7.5
$ gmake print-build
build=x86_64-unknown-openbsd7.5
```
while on the PR branch they are equal :heavy_check_mark:
```
$ gmake print-host
host=x86_64-unknown-openbsd7.5
$ gmake print-build
build=x86_64-unknown-openbsd7.5
```
💬 Sjors commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2277895352)
New torrent (untested and seeding might take a while to kick in): `magnet:?xt=urn:btih:596c26cc709e213fdfec997183ff67067241440c&dn=utxo-840000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2277895352)
New torrent (untested and seeding might take a while to kick in): `magnet:?xt=urn:btih:596c26cc709e213fdfec997183ff67067241440c&dn=utxo-840000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
💬 paplorinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-2277900785)
I understand this is still WIP, but checked quickly how much the current impl would speed up the IBD (first 500k between https://github.com/bitcoin/bitcoin/commit/27a770b34b8f1dbb84760f442edb3e23a0c2420b and https://github.com/bitcoin/bitcoin/commit/2a9c93871cea8c3b1d927a2559ef69ea76c9faf9), got the following:
```bash
hyperfine \
--runs 3 \
--parameter-list COMMIT 27a770,2a9c93 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && ./autogen.sh && ./configure && make -j
...
(https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-2277900785)
I understand this is still WIP, but checked quickly how much the current impl would speed up the IBD (first 500k between https://github.com/bitcoin/bitcoin/commit/27a770b34b8f1dbb84760f442edb3e23a0c2420b and https://github.com/bitcoin/bitcoin/commit/2a9c93871cea8c3b1d927a2559ef69ea76c9faf9), got the following:
```bash
hyperfine \
--runs 3 \
--parameter-list COMMIT 27a770,2a9c93 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && ./autogen.sh && ./configure && make -j
...
💬 Sjors commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2277905851)
I partially wiped guix on my (AMD) Ubuntu 24.04 system (`sudo rm -rf /root/.cache/guix /var/guix /gnu/store`).
```sh
$ which guix
/usr/local/bin/guix
$ guix --version
guix (GNU Guix) 1.4.0
$ sudo --login guix pull --commit=efc26826400762207cde9f23802cfe75a737963c
```
This fails at `/gnu/store/bfirgq65ndhf63nn4q6vlkbha9zd931q-openssl-1.1.1l.drv`.
After the #30609 bump to `--commit=7bf1d7aeaffba15c4f680f93ae88fbef25427252` it still tries to build this OpenSSL version.
So even tho
...
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2277905851)
I partially wiped guix on my (AMD) Ubuntu 24.04 system (`sudo rm -rf /root/.cache/guix /var/guix /gnu/store`).
```sh
$ which guix
/usr/local/bin/guix
$ guix --version
guix (GNU Guix) 1.4.0
$ sudo --login guix pull --commit=efc26826400762207cde9f23802cfe75a737963c
```
This fails at `/gnu/store/bfirgq65ndhf63nn4q6vlkbha9zd931q-openssl-1.1.1l.drv`.
After the #30609 bump to `--commit=7bf1d7aeaffba15c4f680f93ae88fbef25427252` it still tries to build this OpenSSL version.
So even tho
...
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711452798)
Addressed in https://github.com/hebasto/bitcoin/pull/316.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711452798)
Addressed in https://github.com/hebasto/bitcoin/pull/316.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711453187)
Addressed in https://github.com/hebasto/bitcoin/pull/316.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711453187)
Addressed in https://github.com/hebasto/bitcoin/pull/316.
💬 maflcko commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2277931416)
> So even though Bitcoin Core doesn't need it anymore, Guix itself still does
Would be nice to report this upstream, so that openssl-1.1.1l can be removed, but I guess the removal may not be trivial
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2277931416)
> So even though Bitcoin Core doesn't need it anymore, Guix itself still does
Would be nice to report this upstream, so that openssl-1.1.1l can be removed, but I guess the removal may not be trivial
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711453782)
> As an alternative workaround, I'd suggest to use an extra configuration option `-DAPPEND_LDFLAGS="-Wl,-no_warn_duplicate_libraries"`.
Addressed in https://github.com/hebasto/bitcoin/pull/316.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711453782)
> As an alternative workaround, I'd suggest to use an extra configuration option `-DAPPEND_LDFLAGS="-Wl,-no_warn_duplicate_libraries"`.
Addressed in https://github.com/hebasto/bitcoin/pull/316.