👍 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

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613633416)
I think that makes more sense

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2129822038)
Thanks for the suggestions @vasild, I've partially covered them and commented on some of the pending ones

💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613674426)
Fixed.

`inline` does have an effect beyond making it an inline function (in GCC and Clang it increases the eagerness of the compiler to actually inline the function), but that's the sort of optimization one should only do guided by benchmarks, which I haven't done, so I dropped the `inline` here.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613674776)
Done.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613675392)
Replaced with `Overlaps()`.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613675918)
Replaced with `IsSubsetOf()` and `IsSupersetOf()`.