💬 theStack commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200034205)
Seems like it's possible that the VERSION message for a new outbound connection is sent out, received locally and processed _before_ the corresponding `CNode` instance is added to the `m_nodes` list, so the self-connection can't be detected yet. I can reproduce the failure locally with the following patch:
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 990c58ee3d..64b5995dbe 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2921,6 +2921,7 @@ void CConnman::OpenNetworkConnection(const C
...

💬 Sjors commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200035584)
Noticed as well for #30356 which is _not_ based on https://github.com/bitcoin/bitcoin/pull/30362, but I guess CI runs on a merge commit? https://github.com/bitcoin/bitcoin/actions/runs/9742619062/job/26884283814?pr=30356

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1660993146)
Fixed and added `@param[in]` comments.

What is enforced by clang (not by doxygen) via `-Wdocumentation` is a mismatch in the parameter names - e.g. if there is `@param[in] throughput ...` and the actual parameter is `int thrughput`, then it will give a warning (or error if `-Werror` is used).

💬 maflcko commented on issue "fuzz: Fix stability, determinism issues":
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-2200045414)
The latest build log from https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core says:

```
Step #4 - "build-check-afl-address-x86_64": Retrying failed fuzz targets sequentially 4
Step #4 - "build-check-afl-address-x86_64": INFO: performing bad build checks for /tmp/not-out/tmpjtp3xr64/process_message
Step #4 - "build-check-afl-address-x86_64": INFO: performing bad build checks for /tmp/not-out/tmpjtp3xr64/process_messages
Step #4 - "build-check-afl-address-x86_64": INF
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1660998055)
Could be either way. If there is more than one caller of this function, then having it here would avoid duplicating the check in every caller, but there is one caller so far.

💬 maflcko commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200056770)


I don't think this is used in a critical path, to warrant a speedup?

💬 paplorinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200078041)
> I don't think this is used in a critical path, to warrant a speedup?

It's literally the example from the [benchmarking docs](https://github.com/bitcoin/bitcoin/blob/master/doc/benchmarking.md):
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/822e347b-0064-4343-8292-bf753530a7e9">

which was specifically optimized in various other commits before: https://github.com/bitcoin/bitcoin/commit/bcab47bc1b2bfdd29f8b89f8a211755299938aea, https://github.com/bitcoin/bitcoin/commit/159ed
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661016997)
Because we want to ignore incoming messages after a successful handshake. Ignoring the `VERACK` would disturb the handshake.

💬 maflcko commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200092884)
What you link is supporting my point, is it not? https://github.com/bitcoin/bitcoin/pull/12704#issuecomment-374835228

> Stop wasting time on discussing the performance. This does not matter. Decoding an address could take 50 us and I don't think anyone would notice.

> If the resulting code looks better, go for it. Otherwise, don't.

> -0

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661030282)
Note that this would only be reached if `m_by_priority` contains txid that does not have an entry in `m_by_txid` which is a gross bug in the `PrivateBroadast` class. IIRC this was an assert before but somebody suggested to use `Assume()` instead.

💬 paplorinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200101421)
What's the point of the benchmarks in that case exactly?
Anyway, I can make the code slightly simpler and similarly performant, would that be welcome?

💬 maflcko commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200102096)
Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in `listunspent`. Seems fine, if this is still the case. But this will need to be checked first.

🚀 glozow merged a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237)

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661038246)
We don't want to throw an error if `fOk` is false. This happens in tests when using mock parent caches that just return `false` and don't unset any flags.

👍 maflcko approved a pull request: "Show maximum mempool size in information window"
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2151372594)
lgtm

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661061816)
Yeah, my suggestion was completely off here, thanks for checking.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661067593)
Does it make sense to check what the main compilers do in this case (the ones I checked via godbolt automatically inline), or was that just a waste of time, since seemingly unrelated changes could change the outcome (but if that's the case, shouldn't we let the compiler decide)?

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661067868)
Replaced with `NodeClock::now()`.

🤔 Sjors reviewed a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2151386338)
I'm still a bit confused on how to use this, e.g. in #30332.

The first step is to override `CreateSocket()` in `sv2_template_provider_tests.cpp`, so that when the `TemplateProvider` calls it we get a mock socket.

But how? Something like:

```cpp
CreateSock = [](int, int, int) {
return std::make_unique<DynSock>(...);
};
```

The test contains a `TPTester` helper which has `std::unique_ptr<DynSock> m_peer_socket;` to represent the other side of the connection.

This is inialit
...

💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1661049774)
06b21ab6cb868f6b1e41e36a1f3a4d4c9c51558b: is this still useful now that you have `GetBytes`? It seems it's the job for `Transport` to turn bytes into a `CNetMessage`.

Same question for `PushNetMsg` below.

See earlier discussion: https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1494319538