🤔 glozow reviewed a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237#pullrequestreview-2151265702)
ACK 55eea003af24169c883e1761beb997e151845225

💬 glozow commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1660980865)
Another PR could change these calls to use `PeerManagerImpl::m_rng` though it would require taking off the `g_msgproc_mutex` guard.

💬 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()`.