💬 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
```

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2129605456)
rebased on master :rocket:

👍 instagibbs approved a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2074162009)
utACK ef8de26be5478729328ac9d8a9ad6898351552b6

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611810616)
non-blocking nit: commit message still referencing TxRequest

💬 theuni commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613558242)
`.Subset()` and `.Superset()`?

Without these comments, I would have to guess at the meanings (and I would have guessed wrong).

💬 maflcko commented on issue "Restore wallet taking forever to load":
(https://github.com/bitcoin/bitcoin/issues/30108#issuecomment-2129698658)
Any update on this?

💬 jonatack commented on issue "Apparently CJDNS network does not work with Tor on mainnet.":
(https://github.com/bitcoin/bitcoin/issues/24450#issuecomment-2129729113)
Network redundancy is valuable. We’ve seen attacks on Tor and I2P -- the latter very recently. If both Tor and I2P are taken down the same time, CJDNS use may increase quickly.

I think CJDNS mainly needs more awareness, an easy tutorial, and integration by node-in-box packages (see https://x.com/jonatack/status/1794016131272814708 that I just posted, suggesting bounties for that). These actions made a large difference in I2P use by bitcoin nodes.

💬 maflcko commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613608895)
```suggestion
size_t FirstPart() const noexcept { return std::min(m_capacity - m_offset, m_size); }
```

style-nit: I think `inline` can be dropped, according to https://en.cppreference.com/w/cpp/language/inline

> A function defined entirely inside a [class/struct/union definition](https://en.cppreference.com/w/cpp/language/classes), whether it's a member function or a non-member friend function, is implicitly an inline function [...]

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613613375)
I'm happy to rename `nFD`, but I kept it because it is not locally defined, but defined in the namespace, so at least one other method is using it. If we don't mind a bigger diff I'm happy to change it

👍 alfonsoromanz approved a pull request: "Bugfix - don't allow multiple dialogs for same tx in TransactionView"
(https://github.com/bitcoin-core/gui/pull/817#pullrequestreview-2077176859)
Re ACK ba13f10f9ca5bad7de0e140b667fbae5e8b7b9a3. I tested in Mac and now the dialog is brought to the foreground

https://github.com/bitcoin-core/gui/assets/19962151/dd21afaa-b48d-4082-be4c-3c3bdc965cfa