💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2436261655)
> > Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?
>
> We should definitely be able to see easily what the invalid values were (and add unit tests for those cases)

(Not sure that would add much value, we know the fuzzing will produce mostly garbage format strings that will trigger errors).

💬 maflcko commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436264728)
lgtm ACK 33a28e252a7349c0aa284005aee97873b965fcfe

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815667257)
Usually these optimizations concentrate on the measurable parts based on the profiling results that I'm getting during reindexing or IBD. Obfuscating a single bit (i.e. `XorSmall`) wasn't my focus, it's already very fast, didn't seem like the bottleneck.
Would you like me to concentrate on that scenario instead? Or would it make more sense to serialize a block and use that as the basis for the benchmarks?

> C++ compiler .......................... AppleClang 16.0.0.16000026

Before:
|
...

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436298046)
> Are they saying that IBD was 4% faster?

That's what I'm measuring currently, but I don't expect more than 2% difference here.

Posting the perf here for reference:
<img width="500" alt="image" src="https://github.com/user-attachments/assets/6dc99dee-bda8-40d0-bdb0-2030982e0645">
Reindexing until 300k blocks reveals that XOR usage was reduced:
<img width="500" alt="image" src="https://github.com/user-attachments/assets/fe308c34-da65-42f3-9b10-7d34aa9a8056">

💬 maflcko commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815699568)
> Would you like me to concentrate on that scenario instead? Or would it make more sense to serialize a block and use that as the basis for the benchmarks?

Well no. I think this has been mentioned previously. Generally, optimizing for micro benchmarks may not yield results that are actually meaningful or visible for end-users, because the benchmarks capture only a very specific and narrow view. Optimizing for one could even make the code slower for another (as observed above). Adding a bench
...

💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815522185)
same as 2 lines above

🤔 mzumsande reviewed a pull request: "test: extend the SOCKS5 Python proxy to actually connect to a destination"
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2393420379)
Code review / tested ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0

This is pretty cool, and could be used in other places than just for the private transaction relay (e.g. whenever we'd like to have an onion outbound peer in a functional test and exchange messages with it)

I tested this by extending `feature_anchors.py` to use a `destinations_factory` and checked that it completes the version handshake and behaves like a normal connection.

💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815521913)
This log isn't accurate if `keep_alive` is set, because then we'll log that we close the connection but actually stay connected.

💬 achow101 commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436408742)
ACK 33a28e252a7349c0aa284005aee97873b965fcfe

🤔 hodlinator reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2393651994)
Concept ACK a1c9d23ff587b84d37175b2993ea6760f9762177

Good to fix weird negation corner cases.

(Very tempting to slap `DISALLOW_NEGATION` on non-list args for some reason, but I guess someone could be using `norpcwallet` to reset earlier values instead of `rpcwallet=`).

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815689215)
Better to explain *why* than *what* (log message already explains *what*). To me it's not immediately obvious that we should avoid using DNS seeds when having a Tor SOCKS5 proxy?

Furthermore, as I understand it we don't really ignore `-dnsseed` when having a proxy; we use them differently, making the log message not correct to begin with?:
https://github.com/bitcoin/bitcoin/blob/a1c9d23ff587b84d37175b2993ea6760f9762177/src/net.cpp#L2284-L2288

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815722428)
More useful comment IMO:
```suggestion
// Populate outgoing connections unless negated or disabled.
if (connect.size() != 1 || connect[0] != "0") {
connOptions.m_specified_outgoing = connect;
}
```

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815672325)
nit: `LogPrintf`-string provides enough info that I find this added comment redundant.

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815740964)
nit: Would move this line resetting the config after the `for`-loop to clarify that it does not affect it (though I understand the urge to keep it close to the original replace).

(Verified locally that test still succeeds with the move just to be sure).
```suggestion
for msg in [dnsseed_disabled, listen_disabled]:
if msg not in info.log:
raise AssertionError(f"Expected {msg!r} in -noconnect log message")
self.nodes[0].replace_in
...

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815702002)
Use local variable.
```suggestion
onlynets.empty() ||
```

💬 achow101 commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2436410526)
ACK 9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5

🚀 achow101 merged a pull request: "doc: replace `-?` with `-h` and `-help`"
(https://github.com/bitcoin/bitcoin/pull/31118)

✅ achow101 closed an issue: "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it"
(https://github.com/bitcoin/bitcoin/issues/30390)

🚀 achow101 merged a pull request: "util: Remove RandAddSeedPerfmon"
(https://github.com/bitcoin/bitcoin/pull/31124)

💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2436439210)
Turned off hourly CI for now, since fix removing the offending function was merged into master.

Intend to experiment with more surgical solutions and document that here in the future.