💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1662247336)
The Tor connectivity could be "off" during startup and become "on" later at runtime, when we connect to the Tor daemon and it tells us the Tor proxy address. This is why the startup condition is a bit different, it has the `onion_may_become_reachable` variable.

👍 stickies-v approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2153373722)
re-ACK 064a401e10f2b2da8f31f682929431d06c671d93 , main change is using a single nonce for both caches

I think the last commit 064a401e10f2b2da8f31f682929431d06c671d93 is unnecessary scope creep for this PR, and although it looks safe to me, I am not 100% confident I am grasping the full extent of the implications, and I think a separate PR would've been more suitable.

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1662270745)
from comment https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1659087156:
> Why didn't we need to do this on master?
> The old log message was `Connected & Listening: 127.0.0.1:15689` not `Connected & Listening: 0:0`

When accepting connections (from the python node point of view), then indeed 0:0 is printed (on `master`):

```
$ ./test/functional/p2p_addr_relay.py -l debug --nocleanup
$ ./test/functional/combine_logs.py |grep 'Connected & L'
...
test 2024-07-02T10:18:18.494
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1662271368)
I replied to that comment to keep the discussion in one place.

💬 Sjors commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2202687388)
Nice! Will try this out soon.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2202707437)
`7a7e7d189d...3bd618a91f`: rebase and address suggestions

📝 hodlinator opened a pull request: "refactor: Make uint256S(const char*) consteval"
(https://github.com/bitcoin/bitcoin/pull/30377)
`uint256S()` calls taking C-string literals are littered throughout the code and were executed at runtime to perform parsing unless a given optimizer was surprisingly efficient. While this may not be a hot spot, it's better hygiene *in C++20* to store the parsed data blob directly in the binary, without any parsing at runtime.

**transaction_identifier.h** - Fixed dormant bug in `TxidFromString()` where the `string_view` length wasn't respected(!).

Added runtime overload taking `std::string
...

💬 vasild commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2202728919)
In case you settle with adding to `m_nodes` before `PushNodeVersion()` and I do not manage to review, it is good to ensure that no code will be upset by an existence of an (oubound) entry in `m_nodes` that does not have version pushed.

💬 maflcko commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2202734365)
> I don't know how to evaluate if this is a good idea but I'm also not using the test runner, so wouldn't mind. Also, I don't think oss-fuzz is doing this, so how come they found the bug but we didn't?

Good points. Possibly, the single run on my laptop was just extremely lucky and it is highly variant to find the bug regardless of max_len. The high variance will probably make this hard to benchmark.

🤔 glozow reviewed a pull request: "doc: use TRUC instead of v3 and add release note"
(https://github.com/bitcoin/bitcoin/pull/30272#pullrequestreview-2153448506)
thanks @ismaelsadeeq! Took your suggestions and fixed docs in a few more places

💬 glozow commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#discussion_r1662304753)
added

💬 glozow commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#discussion_r1662304855)
added

💬 glozow commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#discussion_r1662322635)
updated all

💬 glozow commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#discussion_r1662305149)
fixed

💬 glozow commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#discussion_r1662305299)
did unit test docs as well

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1662344638)
Looking at this some more, I wonder if the `signature_cache` should be static. Before, the test was re-using the same cache, now it makes a new one on every run. Maybe this should be set up once in `initialize_script_sigcache()`? On the other hand I think it would also be nice to apply your suggestions and test with different nonces and sizes.

✅ paplorinc closed a pull request: "WIP: Simplify SipHash"
(https://github.com/bitcoin/bitcoin/pull/30317)

💬 vasild commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2202968468)
Ok, in the case with `python3 -m http.server 60000` the reply is `<!DOCTYPE HTML>`. In the CI the reply is `HELLO VERSION MIN=3.1 MAX=3.1`, same as the request. So some "echo tcp server" is running on port 60000!?

I wonder what is a sure way to find an address and port that will fail the connection quickly? Maybe pick one of the "reserved" ports from https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers, e.g. `127.0.0.1:250`? Or use the P2P port of node0 (`p2p_port(0)`), `bitcoind` i
...

💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-2202983850)
@cbergqvist,

> The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor

Only tests that use `bind_to_localhost_only` will do. We have 5 such tests.

💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1662415420)
I forgot. Applied it now.