💬 maflcko commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549731528)
nit: Could also mention the RPCs to get the per-peer offset, and the median, as well as the NodeClock time?

💬 m3dwards commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034615159)
I can try and recreate this. @vostrnad do you have a script that records / plots the graphs?

⚠️ maflcko opened an 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)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Undefined-shift

### Expected behaviour

no Undefined-shift

### Steps to reproduce

* Compile fuzz targets with `./configure CC=clang CXX=clang++ --enable-fuzz --with-sanitizers=fuzzer,undefined`
* Create crash input: `echo 'Av////////////8gICD///8gIP8g/yAg/yA=' | base64 --decode > /tmp/crash.bin`
* Run Fuzz target: `FUZZ=minisketch ./src/test/fuzz/fuzz /tmp/crash.bin`

### Relevant log
...

💬 maflcko 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-2034781086)
Ref https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67799

💬 Sjors commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2034809713)
@mzumsande I don't know, but it seems such applications should use a more portable format like SQL, see #27432

💬 Sjors commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2034831892)
Concept ACK

💬 Sjors commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1549905845)
TIL we compress certain standard `scriptPubKey` types.

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