💬 maflcko commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-3427342678)
for reference, the workaround isn't needed for the GHA runners, but it is needed locally and for the Cirrus runners, see also the workflow yaml: https://github.com/bitcoin/bitcoin/blob/c862936d16a640690ef4c89456738fe4bb99be54/.github/workflows/ci.yml#L554-L557

✅ willcl-ark closed an issue: "v27.2 guix build fails with hash mismatch"
(https://github.com/bitcoin/bitcoin/issues/31266)

💬 willcl-ark commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3427345542)
This issue is being closed because it falls outside the scope of this repository.

💬 maflcko commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3427350747)
> This issue is being closed because it falls outside the scope of this repository.

Not sure this is fully true, but this will be fixed by the next time-machine bump. C.f. https://github.com/bitcoin/bitcoin/pull/33185

💬 dergoegge commented on pull request "ci: add Valgrind fuzz":
(https://github.com/bitcoin/bitcoin/pull/33461#issuecomment-3427415276)
ACK c020ae082664438cf340cb196fc5bd0d31ed810f

I think it'd be good to have this in CI (for the rare case where it does find something).

📝 fanquake reopened a pull request: "ci: add Valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/33461)
Valgrind fuzz runtime?

🤔 stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3360113563)
Comments on logging support (c5f8bd0e).

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2447818262)
I guess we want to throw if `StartLogging()` returns `false` (and let the `catch` block to handle the cleanup and re-throw)?
Side question: Can `StartLogging()` itself actually throw? If not, this part can be simplified further.
```suggestion
try {
// Only start logging if we just added the connection.
if (LogInstance().NumConnections() == 1 && !LogInstance().StartLogging()) {
throw std::runtime_error("Failed to start logging");

...

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2447802589)
For `btck_logging_connection_create()` we are documenting that we return null on error. But it seems we either return a connection or crash here. Shouldn't we catch the exception here and return null?

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2448701985)
It seems the log level we set when `btck_LogCategory_ALL` is passed, is (also) used as a fallback; right? (according to [this](https://github.com/bitcoin/bitcoin/blob/c862936d16a640690ef4c89456738fe4bb99be54/src/logging.cpp#L164))

```suggestion
* @param[in] category If btck_LogCategory_ALL is chosen, sets both the global fallback log level
* used by all categories that don't have a specific level set, and also
* sets the log level for messages log
...

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2448325927)
```suggestion
} catch (std::exception& e) {
LogError("Logger start failed: %s", e.what());
LogInstance().DeleteCallback(connection);
if (user_data && user_data_destroy_callback) {
user_data_destroy_callback(user_data);
}
throw;
}
```

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2448571315)
I have the following suggestion here; bringing the `DisconnectTestLogger()` first, so removing the last callback and switching back to buffering happens atomically and no logs are missed in between.
```cpp
~LoggingConnection()
{
LOCK(cs_main);
LogDebug(BCLog::KERNEL, "Logger disconnected.");

// Switch back to buffering by calling DisconnectTestLogger if the
// connection that we are about to remove is the last one.
if (LogInstance().NumConnections() == 1) {

...

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2448774015)
Do we need to acquire `cs_main` here? Seems `SetLogLevel()` and `AddCategoryLogLevel()` calls are thread safe.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2448867717)
Looks like in `BCLog::Logger::DisableLogging()` we have an assertion on making sure we have no connections (`assert(m_print_callbacks.empty())`). Might be good to include the doc that this should not be called when we have existing connections.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2448844008)
nit
```suggestion
Logger logger{std::make_unique<TestLog>(), logging_options};
Logger logger_2{std::make_unique<TestLog>(), logging_options};
}
Logger logger{std::make_unique<TestLog>(), logging_options};
```

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2447873419)
Seems options are set globally. Wouldn't it be better to have a separate `btck_logging_set_options()` function? Keeping it as a param in `btck_logging_connection_create()` suggests it is set for the connection instance.

💬 maflcko commented on issue "fuzz: Fix stability, determinism issues":
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-3427502588)
> edit: oss-fuzz link https://oss-fuzz.com/fuzzer-stats?group_by=by-fuzzer&fuzzer=afl&job=afl_asan_bitcoin-core&project=bitcoin-core

Currently the link just returns a stability of 100 for all fuzz targets, so something is probably off at the oss-fuzz side.

💬 kevkevinpal commented on pull request "ci: add Valgrind fuzz":
(https://github.com/bitcoin/bitcoin/pull/33461#issuecomment-3427528357)
ACK [c020ae0](https://github.com/bitcoin/bitcoin/pull/33461/commits/c020ae082664438cf340cb196fc5bd0d31ed810f)

I agree with @dergoegge. Given the fact that it did find an issue, I think it's worth running.

📝 maflcko opened a pull request: "test: Use unassigned p2p_port instead of hardcoded 60000 in p2p_i2p_ports.py"
(https://github.com/bitcoin/bitcoin/pull/33670)
The goal is to fix https://github.com/bitcoin/bitcoin/issues/30030.

The root cause it unclear. However, hard-coding the port to 60000 does not seem ideal anyway. This could break in an unlikely setting where so many functional tests are run, such that the port is occupied. Also, it could fail when `TEST_RUNNER_PORT_MIN` is set sufficiently high.

So fix those issues (and hopefully also 30030) by using an unoccupied p2p_port.

The logic is similar to the `extra_port()` logic in the `featur
...

👍 laanwj approved a pull request: "[IBD] precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3361685613)
Code review ACK 89bbbbd257063118e6968c409e52632835b76ce8