💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711130542)
This definitely fixed it for me, how can I help with the other cases you've mentioned?

💬 ismaelsadeeq commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2277538703)
Yes I think it is, I've bisected and it starts crashing on 455fca86cfada1823aa28615b5683f9dc73dbb9a

IIUC I dont think we should `assert` that `TxoutType::ANCHOR` will return false after parsing a `CScript`.

It returns true.

https://github.com/bitcoin/bitcoin/blob/24ced5274438c0185963ae1296ec7dda97ea62f1/src/addresstype.cpp#L90-L92

So I think we should not `assert` for that?

<details>
<summary>diff</summary>

```diff
diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script
...

🤔 ismaelsadeeq reviewed a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598#pullrequestreview-2229782323)
Re-ACK 00618e8745192d209c23e3ae873c077e58168957

It looks like C.I failure is unrelated as this [diff](https://github.com/bitcoin/bitcoin/compare/6a020f019f709b595b68516bb3772b811a962e7c..00618e8745192d209c23e3ae873c077e58168957) is the last push.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711152030)
> and it doesn't explain why it doesn't happen for Autotools build

Autotools lack `-Wl,-fatal_warnings` in the following check: https://github.com/bitcoin/bitcoin/blob/24ced5274438c0185963ae1296ec7dda97ea62f1/configure.ac#L365-L375

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711157307)
Ok, but it's still not clear why a compiler we don't use / isn't installed, would be adding libraries to the link line.

🚀 fanquake merged a pull request: "guix: bump time-machine to 7bf1d7aeaffba15c4f680f93ae88fbef25427252"
(https://github.com/bitcoin/bitcoin/pull/30609)

💬 maflcko commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#issuecomment-2277582999)
Addressed all feedback. Should be easy to re-ack

💬 fjahr commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1711181802)
I will leave this unaddressed for now since either way seems ok and we have 3 acks.

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711188855)
The proposed `-no_warn_duplicate_libraries` will only fix it for duplicate libs warnings (but any other warning would fail the build). Can we check if we're getting any other warning - or whether the above change fixes it?

💬 m3dwards commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711197957)
On Arm Mac I get the same configuration problem. In the logs I'm seeing:

```shell
ld: warning: ignoring duplicate libraries: '-lc++'
ld: fatal warning(s) induced error (-fatal_warnings)
```

It works with autotools (perhaps warnings aren't errors in the configure tests?). I've passed the full log to @hebasto.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711200712)
@m3dwards

> It works with autotools (perhaps warnings aren't errors in the configure tests?).

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

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