💬 brunoerg commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#issuecomment-2143444520)
Concept ACK

🤔 Amygo777 reviewed a pull request: "JSON-RPC request Content-Type is application/json"
(https://github.com/bitcoin/bitcoin/pull/29946#pullrequestreview-2092111082)
Nice

💬 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-2143471790)
`d0c8109dd0...2e5e69a2d6`: rebase, address suggestions and most importantly - @achow101, wow! thank you for catching this! The problem was that when a functional test requests `bind_to_localhost_only=False` then the framework would not use `bind=127.0.0.1` and it will start `bitcoind` without any `bind=...` and thus would still try to bind on `127.0.0.1:18445` causing collisions. The solution is to use `bind=0.0.0.0` in that case.

💬 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_r1623233243)
Dropped this line altogether.

💬 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_r1623233322)
Changed to use F-strings.

💬 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_r1623235276)
Done, thanks!

💬 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-2143477308)
`2e5e69a2d6...8c3087150c`: forgot to address one suggestion, sorry for the noise

👍 vasild approved a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065#pullrequestreview-2092138396)
ACK a9e211a5302630635b825655cd984683794aa42c

Note: this is the last but one commit. The last commit 23f843d816 `init: fix min required fds check and account for outbounds` now contains a single dummy change of removing an empty line. Can/should be dropped altogether. You can `push -f` a9e211a5302630635b825655cd984683794aa42c into this branch.

Thanks!

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1623248432)
Excellent! Some simplifications leading to further simplifications. :heart:

✅ kosuodhmwa closed an issue: "VM disk for OS (Debian 12.x) gets smaller and smaller - NOT the same disk i used for .bitdoin data directory which is mounted on another disk"
(https://github.com/bitcoin/bitcoin/issues/30191)

💬 kosuodhmwa commented on issue "VM disk for OS (Debian 12.x) gets smaller and smaller - NOT the same disk i used for .bitdoin data directory which is mounted on another disk":
(https://github.com/bitcoin/bitcoin/issues/30191#issuecomment-2143529011)
solved

✅ kosuodhmwa closed an issue: "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?"
(https://github.com/bitcoin/bitcoin/issues/29759)

💬 kosuodhmwa commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2143529197)
closed

🤔 vasild reviewed a pull request: "fuzz: Make FuzzedSock fuzz friendlier"
(https://github.com/bitcoin/bitcoin/pull/30211#pullrequestreview-2092217352)
Approach ACK 617d43e656816a532ca275707dffeb6e2a2e1fc7

Just one blocker below (about the overflow).

Thank you for the improvements!

💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623330207)
from the commit message:

> FuzzedSock only supports peeking at one byte at a time, which is not
> fuzzer friendly when trying to receive long data.

I am not sure how likely is this to happen, but at least it is not ruled out by the `recv(2)` documentation: a peek read of 10 bytes returns just 1 byte and repeating the peek read of 10 bytes returns the same 1 byte over and over again. The app can never peek past the 1 byte without actually consuming/reading that byte. I guess it must be rea
...

💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623329527)
This will overflow the buffer in `buf` in the memcopy below if peek read is called with N bytes and then non-peek read is called with M bytes and M < N.

💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623328097)
I guess using `ConsumeRandomLengthByteVector()` is slower than `ConsumeBytes()` because it uses an intermediate `std::string`, but is probably insignificant.

The comment says:

```cpp
// Returns a std::string of length from 0 to |max_length|. When it runs out of
// input data, returns what remains of the input. Designed to be more stable
// with respect to a fuzzer inserting characters than just picking a random
// length and then consuming that many bytes with |ConsumeBytes|.
inline s
...

💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623330824)
Wow! Good catch! Maybe this deserves a comment because there is deeper reason for the way it is written than one may see at a first glance. (aka to prevent somebody restoring it back to `? requested : 0` in a future refactor, code reorganization).

🤔 glozow reviewed a pull request: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186#pullrequestreview-2092227439)
utACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09

💬 glozow commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1623341181)
thanks! fixed these now