💬 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

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3427748120)
Thank you for your review @l0rinc!

> I love the new structure, it's a lot easier to track the progress compared to previous versions. It's also measurably faster than previous versions, we seem to be nearing ~20% - sweeeet!

:rocket:

> we can make this as simple and useful as possible

What else do you have in mind for this being useful? It has a very focused purpose in my view. I would love to make it as simple as possible.

> using barriers

Done :)

> filtering and sorting bef
...

📝 ajtowns opened a pull request: "[wip] wallet: Add separate balance info for non-mempool wallet txs"
(https://github.com/bitcoin/bitcoin/pull/33671)
Changes `getbalances` to report the balance of outputs for transactions that aren't confirmed nor in the mempool (eg due to being part of too long a mempool chain, or spending non-standard outputs, or having a datacarrier output that exceeds `-datacarriersize`, etc)

```
$ bitcoin-cli -regtest getbalances
{
"mine": {
"trusted": 5764.96604310,
"untrusted_pending": 0.00000000,
"immature": 3325.00009446,
"nonmempool": 19.99995580
},
"lastprocessedblock": {
"has
...

💬 rkrux commented on pull request "wallet: refactor to remove redundant sighash calculation":
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3427816798)
Pushed the second commit to fix the fuzz test failures.

👋 rkrux's pull request is ready for review: "wallet: refactor to remove redundant sighash calculation"
(https://github.com/bitcoin/bitcoin/pull/33665)