💬 m3dwards commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711219004)
@hebasto I had a stale version of this page open and hadn't seen the latest comments 🤦

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711222223)
> blindly dereferencing it is not good practice

In C++23 we'll be able to chain them properly via something like `auto hex_value = sanitized_hex.and_then(uint256::FromHex);`
If there's a C++23 checklist somewhere (haven't seen any), we could just leave the dereference as is and fix it after we migrate to C++23 instead.

💬 maflcko commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2277655269)
Given that you can reproduce on 50% of your new action runs, does the following diff work around the bug?


```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index cf4dce3762..d2b6969370 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1873,10 +1873,13 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra

for (const COutPoint& hashTx : coins_to_uncache)
active_chainstate.CoinsTip().Uncache(hashTx);
+
...

💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1711245423)
Fixed. Should be trivial to re-ack

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

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711262045)
On my macOS Sonoma 14.6.1:
```
% cat test.cpp
int main() {}
% /opt/homebrew/opt/llvm/bin/clang++ --version
Homebrew clang version 18.1.8
Target: arm64-apple-darwin23.6.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
% /opt/homebrew/opt/llvm/bin/clang++ -fsanitize=fuzzer -o test test.cpp
ld: warning: ignoring duplicate libraries: '-lc++'
```

📝 ismaelsadeeq opened a pull request: "[test]: remove `ExtractDestination` false assertion for `ANCHOR` script"
(https://github.com/bitcoin/bitcoin/pull/30616)
This PR fixes #30615

`ExtractDestination` returns `true` when `TxoutType` is `ANCHOR` see https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2277538703

💬 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