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

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

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

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

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

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

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

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

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

👋 maflcko's pull request is ready for review: "scripted-diff: Use LogInfo over LogPrintf"
(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

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

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

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

💬 0xB10C commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3613987461)
Saw one today with https://github.com/0xB10C/bitcoin/commit/159b6fd80d8a31c01f3b62d9194cf926c007dc37. Will wait for a few more.

```
Rate-limited addr 98.97.137.54:8333 SELF-ANNOUNCEMENT!? from peer=2324 addr=98.97.137.54:46196 ua=/Satoshi:22.0.0(FutureBit-Apollo-Node)/ processed=1 rate-limited=1 age=478ms type=inbound
```

💬 hMsats commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3613993231)
ACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e

💬 darosior commented on issue "Syncing headers with feeler-peers":
(https://github.com/bitcoin/bitcoin/issues/16859#issuecomment-3614051541)
Is there any reason for keeping this open now that #19858 was merged? Good arguments for not implementing this behaviour through feeler connections were given above, and #19858 implemented this through a new type of temporary outbound connections instead.

🤔 stickies-v reviewed a pull request: "log: don't rate-limit "new peer" with -debug=net"
(https://github.com/bitcoin/bitcoin/pull/34008#pullrequestreview-3541816076)
Concept ACK

Another approach could be to exempt all (not just debug) logs from a debug-enabled category? I think conceptually that makes sense, and I suspect there will be more places where this is helpful so we can minimize the workarounds needed?

For example:

<details>
<summary>git diff on 9e02f78089</summary>

```diff
diff --git a/src/logging.h b/src/logging.h
index defff61d30..074096ed5f 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -377,16 +377,18 @@ inline void LogPrin
...

✅ sdaftuar closed an issue: "Syncing headers with feeler-peers"
(https://github.com/bitcoin/bitcoin/issues/16859)

🤔 darosior reviewed a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3541861752)
Concept/Approach ACK.