💬 sipa commented on issue "fuzz: minisketch: Undefined-shift in std::__1::vector<Field<unsigned int, 32, 141u, RecLinTrans<unsigned int, 6, 6, 5":
(https://github.com/bitcoin/bitcoin/issues/29799#issuecomment-2034842852)
See https://github.com/sipa/minisketch/pull/81 for a fix.

📝 hebasto opened a pull request: "ci: Drop duplicated compiler flags"
(https://github.com/bitcoin/bitcoin/pull/29800)
On the master branch @ 0d509bab45d292caeaf34600e57b5928757c6005, it is easy to check the _"Options used to compile and link"_ section in the `configure` script output and observe duplicated compiler flags.

This PR cleans such cases up.

💬 maflcko commented on pull request "ci: Drop duplicated compiler flags":
(https://github.com/bitcoin/bitcoin/pull/29800#issuecomment-2034899685)
lgtm ACK be97652c07a10399c08ecb98dbbaeb33b84af774

The depends ones are not needed, because depends already passes the flags through. The `-g` isn't needed, because it is always set by default.

💬 m3dwards commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034922309)
Running IBD in a loop, will report back tomorrow.

🤔 vasild reviewed a pull request: "Simplify network-adjusted time warning logic"
(https://github.com/bitcoin/bitcoin/pull/29623#pullrequestreview-1976691061)
Approach ACK 2ef71c73582be554e565ada3f8a6ca77c2cd79f1

Wrt adjusting the peer's offset with local clock corrections in order to stop the warnings immediately after the local clock is fixed (instead of after 4-5h) https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1544783265, consider this:

<details>
<summary>[patch] adjust peer time with local clock corrections (untested)</summary>

```diff
diff --git i/src/node/timeoffsets.cpp w/src/node/timeoffsets.cpp
index 8488e47ff5..4dce6f
...

💬 vasild commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549713660)
This implies that `int64_t` is the same as the internal type of `std::chrono::seconds`. Better get a random integer in the range of `[std::chrono::seconds::min().count(), std::chrono::seconds::max().count()]`.

💬 vasild commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549743266)
```suggestion
static bool IsWarningRaised(std::vector<std::chrono::seconds> check_offsets)
```

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