💬 vasild commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549674844)
Same as above:

```suggestion
static constexpr std::chrono::minutes WARNING_WAIT{60};
```

💬 vasild commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549666557)
Since this is a constant, like `N`:

```suggestion
static constexpr std::chrono::minutes WARN_THRESHOLD{10};
```

💬 vasild commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549730951)
In this case performance is irrelevant but in general `in` arguments that are expensive to copy like `vector` are passed by const reference.

💬 emc99 commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2034942067)
Concept ACK

💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1549991365)
Calling this with `NONE` does not make sense logically and no caller does it currently. Thus I removed this. But you are right that a (nonsensical?) caller could be added in the future. Would you prefer an assert like `assert(category != LogFlags::NONE);` here or to restore this line as it is (was)?

💬 fanquake commented on pull request "ci: Drop duplicated compiler flags":
(https://github.com/bitcoin/bitcoin/pull/29800#discussion_r1549996173)
I guess going forward, we'll have to remember to never add any logic/other flags to the `--with-sanitizers/cmake equivalent` option, that differ from just setting `-fsanitize=x`? Otherwise they could possibly get missed in CI.

Removing this also now skips the link check(s) and I don't see where flags are getting added to LDFLAGS elsewhere? So this is at least a change in behaviour and I'd say somewhat less clear.

💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2034968337)
> See #27231.

There you mention:
> I'm going to open an alternate pull that does what you suggest...

Is the current PR doing what you meant? E.g. drop that help text and return an error for "0" ("none" already returns an error in `master`)?

💬 TheCharlatan commented on pull request "guix: bump time-machine to dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a":
(https://github.com/bitcoin/bitcoin/pull/29651#issuecomment-2034978341)
```
uname --machine && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
riscv64
2cf27183bc9964708fe12465fa3bb216de6e109ecc9f7ead5d6eff29a178eca8 guix-build-cf5faf73c991/output/aarch64-linux-gnu/SHA256SUMS.part
ad97244ca2ebe9fe715bcfb9838d30436cb1e797d32cb6435615df5d35f564d9 guix-build-cf5faf73c991/output/aarch64-linux-gnu/bitcoin-cf5faf73c991-aarch64-linux-gnu-debug.tar.gz
ab34959b100669ebeca4c174660b26ca8eb6c93d33282459
...

⚠️ achow101 opened an issue: "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm"
(https://github.com/bitcoin/bitcoin/issues/29801)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Configure with
```
./configure --enable-fuzz --enable-debug --with-sanitizers=address,fuzzer,undefined,integer CC=clang CXX=clang++
```

Compile fails:
```
crypto/sha256_sse4.cpp:44:9: error: expected relocatable expression
44 | "shl $0x6,%2;"
| ^
<inline asm>:1:1882: note: instantiated into assembly here
crypto/sha256_sse4.cpp:44:9: error: expected r
...

💬 maflcko commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2034994885)
Recalling the IRC discussion, I presume this happens since `-O0` in https://github.com/bitcoin/bitcoin/pull/16435 ?

💬 maflcko commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035002791)
Ref:

```
1782024-04-01T18:08:36 <cfields> can repro with: clang++ -std=c++20 -O0 -fsanitize=fuzzer -c crypto/sha256_sse4.cpp -o out.o
```

https://www.erisian.com.au/bitcoin-core-dev/log-2024-04-02.html

💬 fanquake commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035003353)
The broken combination is `clang++ -std=c++20 -O0 -fsanitize=address -c crypto/sha256_sse4.cpp -o out.o`, pointed out by Cory. Happens for me with Clang-18.

💬 fanquake commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035010355)
I guess the next step is to followup / file an issue with LLVM. I've got #29796 open which may "fix" this by just dropping `-O0`, depending on the outcome of discussion.

💬 fanquake commented on pull request "guix: Remove another leftover from #29648":
(https://github.com/bitcoin/bitcoin/pull/29797#issuecomment-2035021336)
Guix Build (aarch64):
```bash
8e7bddb8fa49c857bc9a815a7f27f2d60f8a2f8955966eff467ee86c0d0776f6 guix-build-3cb80febb876/output/aarch64-linux-gnu/SHA256SUMS.part
de0c0624b55418b6638b4852cbe4527c5fb4a2ccbb82d442e5dff0febc947a70 guix-build-3cb80febb876/output/aarch64-linux-gnu/bitcoin-3cb80febb876-aarch64-linux-gnu-debug.tar.gz
5f37064b7496125b90f7df1fda7df3e6545149274bf737361579c0e9be94f6f8 guix-build-3cb80febb876/output/aarch64-linux-gnu/bitcoin-3cb80febb876-aarch64-linux-gnu.tar.gz
02ad13
...

🤔 glozow reviewed a pull request: "Simplify network-adjusted time warning logic"
(https://github.com/bitcoin/bitcoin/pull/29623#pullrequestreview-1977282272)
approach ACK

💬 glozow commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1550035286)
needs doc?

💬 glozow commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1550033829)
nit: this is unnecessary

💬 glozow commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1550056609)
nit: documentation please, e.g. number of elements to track

💬 glozow commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1550045138)
+1, was going to say this should be removed in 2ef71c73582be554e565ada3f8a6ca77c2cd79f1, or say we are avoiding false warnings instead of preventing tampering of adjusted time

💬 glozow commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1550042121)
Or its own rpc commit?