💬 willcl-ark commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-3427299987)
Since the problem isn’t easily reproducible, I’m closing this for now. If it occurs again, please feel free to open a new issue or comment here so we can reopen it.
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-3427299987)
Since the problem isn’t easily reproducible, I’m closing this for now. If it occurs again, please feel free to open a new issue or comment here so we can reopen it.
✅ willcl-ark closed an issue: "[RFC] What security expectations does/should the RPC server have from credentialed RPC clients?"
(https://github.com/bitcoin/bitcoin/issues/32274)
(https://github.com/bitcoin/bitcoin/issues/32274)
💬 davidgumberg commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/33196#issuecomment-3427324550)
ACK 433954b
(https://github.com/bitcoin/bitcoin/pull/33196#issuecomment-3427324550)
ACK 433954b
💬 supertestnet commented on issue "Allow creating and injecting a custom coinbase into a block template":
(https://github.com/bitcoin/bitcoin/issues/28475#issuecomment-3427329409)
I think it's mostly superseded by this PR anyway: https://github.com/bitcoin/bitcoin/pull/32468
(https://github.com/bitcoin/bitcoin/issues/28475#issuecomment-3427329409)
I think it's mostly superseded by this PR anyway: https://github.com/bitcoin/bitcoin/pull/32468
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3427331772)
Small update, per discussion, will push the thread pool on an isolated PR in the coming week. Replacing the current http server `WorkQueue` and all its related code; it is an easy dedup that adds further test coverage to a never unit nor fuzz tested area. Plus a way to start using this class in the repo that allow others to use it.
Also, I'm investigating the tx index slow down with @andrewtoth. Trying to understand the differences between us. Worst-case scenario, we can disable parallel sync
...
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3427331772)
Small update, per discussion, will push the thread pool on an isolated PR in the coming week. Replacing the current http server `WorkQueue` and all its related code; it is an easy dedup that adds further test coverage to a never unit nor fuzz tested area. Plus a way to start using this class in the repo that allow others to use it.
Also, I'm investigating the tx index slow down with @andrewtoth. Trying to understand the differences between us. Worst-case scenario, we can disable parallel sync
...
💬 willcl-ark 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-3427339037)
Please update the CI readme.md with the needed info, thanks.
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-3427339037)
Please update the CI readme.md with the needed info, thanks.
💬 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
(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)
(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.
(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
(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).
(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?
(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).
(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");
...
(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?
(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
...
(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;
}
```
(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) {
...
(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.
(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.
(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.