💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277722262)
Had a look at a Guix build. Stripping the macOS binaries is broken:
```bash
-- Installing: /distsrc-base/distsrc-ad2140d4d8cc-arm64-apple-darwin/installed/bitcoin-ad2140d4d8cc/bin/bitcoind
/root/.guix-profile/bin/llvm-strip: error: unknown argument '-u'
```

💬 maflcko commented on pull request "Lint: improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#discussion_r1711288337)
nit in 9d1423a90a3b9748426deec7bafe8b12113e1baf: Not sure about moving a single string literal that is used in a single function to a new module into a new function.

`args` allows for chaining, so if you want to append more args to the existing ones, my recommendation would be to just call `args` one more time with the args that you want to append.

I think the lint tests can be easy to understand and pragmatic, and adding too many layers may not be useful to understand them.

💬 maflcko commented on pull request "[test]: remove `ExtractDestination` false assertion for `ANCHOR` script":
(https://github.com/bitcoin/bitcoin/pull/30616#issuecomment-2277736798)
review ACK a4f2b185732649eeea4a042cebd90d0e0e12cc92

ANCHOR has a destination (it is basically a witness program version + witness program data), so the change looks correct.

(Nit: You can replace `[test]` with `test:` in the GitHub pull title to have DrahtBot assign the pull request label for you.)

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711302662)
> Similar problem: [github.com/orgs/Homebrew/discussions/4794](https://github.com/orgs/Homebrew/discussions/4794)

See: https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710280041

👍 theStack approved a pull request: "test: remove `ExtractDestination` false assertion for `ANCHOR` script"
(https://github.com/bitcoin/bitcoin/pull/30616#pullrequestreview-2229987741)
utACK a4f2b185732649eeea4a042cebd90d0e0e12cc92

💬 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"`.

📝 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
...

💬 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.

💬 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.

💬 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.

💬 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

💬 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`.

💬 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

💬 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.

💬 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.

💬 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?

💬 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?

💬 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.

🤔 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
```

💬 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`