💬 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

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613630608)
I agree the if check should be updated, but I don't necessarily agree with splitting `MAX_ADDNODE_CONNECTIONS` from `min_required_fds`.

There is the outstanding question of whether the minimum required fds to run Core should account for some of the connections. Currently, we are not accounting for any (given the check is performed against `MIN_CORE_FDS`), but I think we should be accounting for at least, outbound + manuals