📝 0xB10C opened a pull request: "log: don't rate-limit "new peer" with -debug=net"
(https://github.com/bitcoin/bitcoin/pull/34008)
Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogPrintf`. However, `LogPrintf` will get rate-limited since https://github.com/bitcoin/bitcoin/pull/32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited.
To fix this, use `LogDebug` if `debug=net` is enabled. Otherwise use `LogPrintf`. This means we don't rate-limit the message with `debug=net` (due to `LogDebug` not being rate-limited) but sti
...
(https://github.com/bitcoin/bitcoin/pull/34008)
Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogPrintf`. However, `LogPrintf` will get rate-limited since https://github.com/bitcoin/bitcoin/pull/32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited.
To fix this, use `LogDebug` if `debug=net` is enabled. Otherwise use `LogPrintf`. This means we don't rate-limit the message with `debug=net` (due to `LogDebug` not being rate-limited) but sti
...
💬 0xB10C commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590031496)
I'm not super happy with this approach of duplicating the log statement, but I think it's the clearest to a reader. If someone feels strongly, I'm very happy to change it to something better! There are two alternatives that came to mind:
1. Use a macro to avoid duplicating the log statements: Not sure if we want to define a macro extra for this, but could be done. This was my initial approach I dismissed.
2. Extract the log message string and format it with e.g. `std::format()` first, then pass
...
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590031496)
I'm not super happy with this approach of duplicating the log statement, but I think it's the clearest to a reader. If someone feels strongly, I'm very happy to change it to something better! There are two alternatives that came to mind:
1. Use a macro to avoid duplicating the log statements: Not sure if we want to define a macro extra for this, but could be done. This was my initial approach I dismissed.
2. Extract the log message string and format it with e.g. `std::format()` first, then pass
...
💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3613552394)
The [feedback](https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2550408178) from @fanquake has been addressed.
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3613552394)
The [feedback](https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2550408178) from @fanquake has been addressed.
💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3613627392)
> what do you think about wrapping all the code in `db_key.h` under a namespace like "util", "common", "index::util", or "util::index" (just threw some names), so it's clear when reading the code that these functions are defined elsewhere and shared, without needing to search around for the definition.
I did that in 5646e6c0d3581f12419913b88745f51c7a3161b9 - let me know what you think. I used `index_util`, turned out it was required a bit more often than I would have expected.
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3613627392)
> what do you think about wrapping all the code in `db_key.h` under a namespace like "util", "common", "index::util", or "util::index" (just threw some names), so it's clear when reading the code that these functions are defined elsewhere and shared, without needing to search around for the definition.
I did that in 5646e6c0d3581f12419913b88745f51c7a3161b9 - let me know what you think. I used `index_util`, turned out it was required a bit more often than I would have expected.
💬 ajtowns commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590075362)
Perhaps
```c++
auto mapped_as_info = [&]() -> std::string {
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr);
return (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : "");
};
if (!pfrom.IsInboundconn()) {
LogInfo("New %s %s peer ...", ..., mapped_as_info());
} else {
LogDebug(BCLog::NET, "New %s %s ...", ..., mapped_as_info());
}
```
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590075362)
Perhaps
```c++
auto mapped_as_info = [&]() -> std::string {
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr);
return (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : "");
};
if (!pfrom.IsInboundconn()) {
LogInfo("New %s %s peer ...", ..., mapped_as_info());
} else {
LogDebug(BCLog::NET, "New %s %s ...", ..., mapped_as_info());
}
```
💬 darosior commented on issue "ASN-based bucketing of the network nodes":
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-3613637707)
That makes sense, but maybe other mechanism could achieve the same without the same drawbacks. For instance is it still really necessary after https://github.com/bitcoin/bitcoin/pull/19858?
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-3613637707)
That makes sense, but maybe other mechanism could achieve the same without the same drawbacks. For instance is it still really necessary after https://github.com/bitcoin/bitcoin/pull/19858?
💬 purpleKarrot commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613640791)
> > > Some low-level code could benefit from being able to use `std::expected` from C++23.
> >
> > Can you elaborate? How would it benefit? Over what alternative?
>
> Sure. The alternatives were listed in doc diff in the commit, but I've pulled it out and put in the pull description as well.
What is wrong with the most obvious approach to error handling?
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613640791)
> > > Some low-level code could benefit from being able to use `std::expected` from C++23.
> >
> > Can you elaborate? How would it benefit? Over what alternative?
>
> Sure. The alternatives were listed in doc diff in the commit, but I've pulled it out and put in the pull description as well.
What is wrong with the most obvious approach to error handling?
💬 ajtowns commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590081585)
Or could make it `auto conn_details = [&]() ...` and `LogDebug(BCLog::NET, "New %s peer connected: %s", pfrom.ConnectionTypeAsString(), conn_details())` and move all the transport, version, mapped_as stuff into the lambda, moving the transport type to after the colon.
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590081585)
Or could make it `auto conn_details = [&]() ...` and `LogDebug(BCLog::NET, "New %s peer connected: %s", pfrom.ConnectionTypeAsString(), conn_details())` and move all the transport, version, mapped_as stuff into the lambda, moving the transport type to after the colon.
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3613693155)
> Given it's dropping the other CI, I think this PR should be completing #30210, dropping workarounds, updating any docs, and completing the migration.
More commits have been added to complete the migration.
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3613693155)
> Given it's dropping the other CI, I think this PR should be completing #30210, dropping workarounds, updating any docs, and completing the migration.
More commits have been added to complete the migration.
🤔 ryanofsky reviewed a pull request: "mining: add getMemoryLoad() and track template non-mempool memory footprint"
(https://github.com/bitcoin/bitcoin/pull/33922#pullrequestreview-3541246611)
Code review e8f8f7f677bcde0179526be3ed9a657c44998b93. This looks good except for a thread safety issue I think you can address by adding a mutex.
re: https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3611468373
> > Would be good if this said templates are also released
>
> Added a sentence to the PR description.
Sorry, I should have made a more specific suggestion. The problem is is that this sentence is not accurate: "templates are not released until the client disconnects or calls
...
(https://github.com/bitcoin/bitcoin/pull/33922#pullrequestreview-3541246611)
Code review e8f8f7f677bcde0179526be3ed9a657c44998b93. This looks good except for a thread safety issue I think you can address by adding a mutex.
re: https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3611468373
> > Would be good if this said templates are also released
>
> Added a sentence to the PR description.
Sorry, I should have made a more specific suggestion. The problem is is that this sentence is not accurate: "templates are not released until the client disconnects or calls
...
💬 ryanofsky commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2590041765)
In commit "mining: track non-mempool memory usage" (7c4d03d7b23417612fbca2f22e5bb1a198c9e5a2)
This map can updated from multiple threads, so it needs a mutex to be used safely. I think I'd suggest combining `template_tx_refs` and `gbt_template` variables and a mutex into single struct called something like `BlockTemplateState` and adding a `unique_ptr` to that struct as a member here. The struct could be replaced with a cache class in #33421.
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2590041765)
In commit "mining: track non-mempool memory usage" (7c4d03d7b23417612fbca2f22e5bb1a198c9e5a2)
This map can updated from multiple threads, so it needs a mutex to be used safely. I think I'd suggest combining `template_tx_refs` and `gbt_template` variables and a mutex into single struct called something like `BlockTemplateState` and adding a `unique_ptr` to that struct as a member here. The struct could be replaced with a cache class in #33421.
💬 ryanofsky commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2590089590)
In commit "ipc: add getMemoryLoad()" (e8f8f7f677bcde0179526be3ed9a657c44998b93)
Seems ok to assert this log message is logged, but I'm wondering if there was a particular reason for doing this. Was the idea to pair the LOG_TIME_MILLIS_WITH_CATEGORY and assert_debug_log calls together?
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2590089590)
In commit "ipc: add getMemoryLoad()" (e8f8f7f677bcde0179526be3ed9a657c44998b93)
Seems ok to assert this log message is logged, but I'm wondering if there was a particular reason for doing this. Was the idea to pair the LOG_TIME_MILLIS_WITH_CATEGORY and assert_debug_log calls together?
💬 ryanofsky commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2589990820)
In commit "rpc: move static block_template to node context" (a5eee29fd7d177f57c78da9773cb656a129de839)
I think it would actually be nice to move all these static variables to a struct or class like @ismaelsadeeq's BlockTemplateCache from #33421. But this could be a followup, and doesn't need to complicate this PR.
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2589990820)
In commit "rpc: move static block_template to node context" (a5eee29fd7d177f57c78da9773cb656a129de839)
I think it would actually be nice to move all these static variables to a struct or class like @ismaelsadeeq's BlockTemplateCache from #33421. But this could be a followup, and doesn't need to complicate this PR.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2590129610)
yes, leaving it seems fine. Just wanted to bring up the possibility.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2590129610)
yes, leaving it seems fine. Just wanted to bring up the possibility.
💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613873411)
Concept ACK. We have a lot of low-level code where `std::expected` would be useful, and this PR enables that without having to wait for C++23. (#34005 is another PR that does the same thing and seems like a good approach as well.)
I do think `util::Result` can provide a lot of useful functionality for higher-level code that `std::expected` doesn't, and I've been testing that idea in various PRs (#25665, #25722, #29700, #26022). But I think the fate of the `util::Result` class is a separate qu
...
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613873411)
Concept ACK. We have a lot of low-level code where `std::expected` would be useful, and this PR enables that without having to wait for C++23. (#34005 is another PR that does the same thing and seems like a good approach as well.)
I do think `util::Result` can provide a lot of useful functionality for higher-level code that `std::expected` doesn't, and I've been testing that idea in various PRs (#25665, #25722, #29700, #26022). But I think the fate of the `util::Result` class is a separate qu
...
👋 maflcko's pull request is ready for review: "scripted-diff: Use LogInfo over LogPrintf"
(https://github.com/bitcoin/bitcoin/pull/29641)
(https://github.com/bitcoin/bitcoin/pull/29641)
💬 fanquake commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#issuecomment-3613940445)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/34008#issuecomment-3613940445)
cc @dergoegge
💬 ryanofsky commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3613946152)
re: https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612589376
> Not sure they are conceptually compatible. 25665 aims to reduce the error footprint by wrapping it in a unique_ptr, whereas `std::expected` is basically a variant wrapper (keeps the memory in the struct itself).
Yes, it's true that hypothetically if this PR got merged and then #25665 got merged, performance of code using `util::Expected` could be affected because of `unique_ptr` use. And then, if there was a script
...
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3613946152)
re: https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612589376
> Not sure they are conceptually compatible. 25665 aims to reduce the error footprint by wrapping it in a unique_ptr, whereas `std::expected` is basically a variant wrapper (keeps the memory in the struct itself).
Yes, it's true that hypothetically if this PR got merged and then #25665 got merged, performance of code using `util::Expected` could be affected because of `unique_ptr` use. And then, if there was a script
...
💬 0xB10C commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590270751)
> Or could make it `auto conn_details = [&]() ...` and `LogDebug(BCLog::NET, "New %s peer connected: %s", pfrom.ConnectionTypeAsString(), conn_details())` and move all the transport, version, mapped_as stuff into the lambda, moving the transport type to after the colon.
Could do
```c++
const std::string details = std::format(
"transport={} version={} blocks={} peer={}{}{}",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(),
int{p
...
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590270751)
> Or could make it `auto conn_details = [&]() ...` and `LogDebug(BCLog::NET, "New %s peer connected: %s", pfrom.ConnectionTypeAsString(), conn_details())` and move all the transport, version, mapped_as stuff into the lambda, moving the transport type to after the colon.
Could do
```c++
const std::string details = std::format(
"transport={} version={} blocks={} peer={}{}{}",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(),
int{p
...
💬 0xB10C commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590287220)
I guess the lambda helps to not create the details string if we don't end up logging it.
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590287220)
I guess the lambda helps to not create the details string if we don't end up logging it.