💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2129447054)
Took about 20 minutes. They all match.

Fri May 24 12:12:33 UTC 2024
{
"height": 844917,
"bestblock": "000000000000000000017dd5f59b73629f6f88797e90017a9df39c5e435296bf",
"txouts": 181984254,
"bogosize": 13994337434,
"muhash": "b97a3fb13a61e8c889668d064f7ae5408a78ee7e4c6a4fdebedb30ecb2f23378",
"total_amount": 19702649.24256659,
"transactions": 125720653,
"disk_size": 12220356091
}
Fri May 24 12:39:58 UTC 2024

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2129452748)
Thanks for the reviews @maflcko,

891784ce73a1d911d33b125b66f3856fe6cda56b -> eeea0818c1a20adc5225b98b185953d386c033e0 ([preserveIndexOnRestart_3](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_3) -> [preserveIndexOnRestart_4](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/preserveIndexOnRestart_3..preserveIndexOnRestart_4))

* Addressed @maflcko's [comment](https://github.com/bitcoin/bitc
...

✅ zefir-k closed an issue: "prune shall not delete blocks it did not download"
(https://github.com/bitcoin/bitcoin/issues/30163)

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1613414830)
Yeah, I was thinking this might be used for some more regresssion tests, but as it is right now, it is useless. Will remove.

💬 fanquake commented on pull request "[PoC] ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2129492145)
> I believe, though we'll have to install clang 15 on it.

Then we should pick something newer, or which won't be a blocker to using a newer Clang. Because soon our minimum Clang will be 16.

🤔 vasild reviewed a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065#pullrequestreview-2076548666)
Approach ACK 55f16f56f4

Might as well squash the last commit 55f16f56f4 `init: Remove FreeBSD workaround to #2695` into some of the previous ones that touch that line.

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613282956)
```suggestion
// If we are using select instead of poll, our actual limit may be even smaller
```

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613272500)
In the commit message of a44da681f5 `init: fixes fd accounting regarding poll/select`:

> ... file descriptions limits ...

s/descriptions/descriptors/

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613459411)
snake_case for variables. I think `available` or `available_fds` is a good name:

```suggestion
available_fds = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);
```

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613376297)
An alternative to `max(min())` is [`clamp()`](https://en.cppreference.com/w/cpp/algorithm/clamp):

```suggestion
nMaxConnections = std::clamp(nFD - nMinRequiredFds, 0, nMaxConnections);
```

(feel free to ignore if you don't find it more readable)

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613414626)
What about giving an `InitError` if the provided value is negative instead of silently clamping it to 0? Surely `-maxconnections=-50` looks like a typo and probably the intention is not to end up with `-maxconnections=0`.

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613392850)
```suggestion
// Maximum number of connections with other nodes, this accounts for all types of outbounds and inbounds except for manual
```

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613422626)
according to `doc/developer-notes.md` variables should use snake_case and no need to denote the type in the variable name (`n` for integer): `min_required_fds`.

But more importantly, the name `nMinRequiredFds` is misleading because we accept a smaller number - a bit below we check `if (nFD < MIN_CORE_FDS) return error...`, so it is not "minimum required". I think this variable here should be: `min_required_fds = MIN_CORE_FDS + nBind`, that check should be `if (nFD < min_required_fds) return e
...

🤔 stickies-v reviewed a pull request: "[27.x] Backports and rc1"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2076907949)
ACK cb6def3855427b613357696ba16a431c7964dbcc modulo small release notes fix

All backport commits are clean, except for:
- 77b2321ca03dcbd5f77060510dc8a19e7f4fdfa2 backported from 21b8a14d37c19ce292d5529597e0d52338db48a9: version bumped in https://github.com/bitcoin/bitcoin/pull/29707/ - LGTM

No diff with my local manpage generation.

💬 stickies-v commented on pull request "[27.x] Backports and rc1":
(https://github.com/bitcoin/bitcoin/pull/30092#discussion_r1613471306)
```suggestion
Bitcoin Core version 27.1rc1 is now available from:

<https://bitcoincore.org/bin/bitcoin-core-27.1/test.rc1/>
```

💬 stickies-v commented on pull request "refactor: Use type-safe time in txorphanage":
(https://github.com/bitcoin/bitcoin/pull/30170#issuecomment-2129540582)
Concept ACK

👍 stickies-v approved a pull request: "doc: update mention of generating bitcoin.conf"
(https://github.com/bitcoin/bitcoin/pull/30154#pullrequestreview-2076957908)
ACK 9013e2b97e8f50d2be63ce740c42d0b0e0b9b7f2

meta nit: for trivial changes, discussion can happen on the PR directly, no need to open a separate issue first

💬 kevkevinpal commented on pull request "doc: update mention of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2129562051)
reACK [9013e2b](https://github.com/bitcoin/bitcoin/pull/30154/commits/9013e2b97e8f50d2be63ce740c42d0b0e0b9b7f2)

💬 vasild commented on pull request "BufferedFile: fclose at destruction":
(https://github.com/bitcoin/bitcoin/pull/29614#discussion_r1613507503)
Is it not the case that now this destructor is not needed because you explicitly call `fclose()`?

💬 vasild commented on issue "Apparently CJDNS network does not work with Tor on mainnet.":
(https://github.com/bitcoin/bitcoin/issues/24450#issuecomment-2129588944)
Taking a step back from `-proxy=1.2.3.4:5678=ipv4` vs `-cjdnsproxy=`: if very few people use CJDNS for Bitcoin, is it worth spending effort on CJDNS at all? On my node there are 3 CJDNS addresses in addrman:

```sh
$ bitcoin-cli getpeerinfo |jq 'map(select(.network == "cjdns")) |length'
3
```