💬 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

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