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

💬 maflcko commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r2590403179)
It is an internal bug, so it is dead code and doesn't really matter much. I think internal bugs should be using `Assume(false)`. Logs could go with `LogError("%s", STR_INTERNAL_BUG("..."))`, but I wanted to keep the changes here minimal. Happy to go with `LogError("%s", STR_INTERNAL_BUG(` if you feel strongly.

🤔 darosior reviewed a pull request: "test: Add DERSIG unit tests to script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3541895187)
Makes sense to test `DERSIG` separately from `STRICTENC` since it's the only one enabled by consensus. Concept/Approach ACK.

Maintainers could we get the CI ran here?

💬 ryanofsky commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3614166836)
<!-- begin push-2 -->
Updated d179f713792432c82befd39a288ab16fec13bab1 -> 6ef092c0e4343fc421c323ff09d3c791fb4bc33a ([`pr/pyipc.1`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.1) -> [`pr/pyipc.2`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/pyipc.1..pr/pyipc.2))<!-- end --> splitting into two commits

re: https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611494379

> > Destroy calls were being made at
...

🤔 stickies-v reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3541953136)
Concept ACK. Functions with return type `std::optional<E>` returning a truthy value on failure is unintuitive, this is much better.

👍 darosior approved a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541954260)
utACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

🤔 mzumsande reviewed a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541981127)
ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

didn't test it, but but the explanation of the bug make sense and since all the network threads have been stopped at this point anyway I can't see a downside to clearing `m_reconnections` here.

👍 maflcko approved a pull request: "test: Remove `system_tests/run_command` runtime dependencies"
(https://github.com/bitcoin/bitcoin/pull/33929#pullrequestreview-3542024077)
lgtm

💬 maflcko commented on pull request "test: Remove `system_tests/run_command` runtime dependencies":
(https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065)
nit: This will fail with spaces in the dir:


```
$ ./a\ space/test_bitcoin -t system_tests
Running 1 test case...
unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)
test/system_tests.cpp(29): last checkpoint

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"