💬 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
(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
(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
(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.
(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.
(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()`.
(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()`.
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613675918)
Replaced with `IsSubsetOf()` and `IsSupersetOf()`.
💬 theuni commented on pull request "fuzz: Fix wallet_bdb_parser stdlib error matching":
(https://github.com/bitcoin/bitcoin/pull/30169#issuecomment-2129867229)
utACK.
Was this caught by review or did you run into it? Just curious if there are others.
(https://github.com/bitcoin/bitcoin/pull/30169#issuecomment-2129867229)
utACK.
Was this caught by review or did you run into it? Just curious if there are others.
💬 achow101 commented on pull request "fuzz: Fix wallet_bdb_parser stdlib error matching":
(https://github.com/bitcoin/bitcoin/pull/30169#issuecomment-2129882575)
ACK fac72985292b516919a216d9a78cf84418cd7f96
(https://github.com/bitcoin/bitcoin/pull/30169#issuecomment-2129882575)
ACK fac72985292b516919a216d9a78cf84418cd7f96
💬 jonatack commented on issue ""netinfo" doesn't show IPv6 "Local addresses"":
(https://github.com/bitcoin/bitcoin/issues/30165#issuecomment-2129888738)
Thanks @lcharles123 for the info.
> "Local addresses" section of "netinfo" does not show the IPv6 addresses added using "externalip" config option. (Probably there is no announce of this addresses).
CLI `-netinfo` will only return the local addresses that are returned by RPC `getnetworkinfo`, as under the hood, -netinfo calls getnetworkinfo to fetch that info.
Of the networks getnetworkinfo returns to you, IPv6 (and CJDNS, which is an IPv6 overlay network) are not reachable, which is wh
...
(https://github.com/bitcoin/bitcoin/issues/30165#issuecomment-2129888738)
Thanks @lcharles123 for the info.
> "Local addresses" section of "netinfo" does not show the IPv6 addresses added using "externalip" config option. (Probably there is no announce of this addresses).
CLI `-netinfo` will only return the local addresses that are returned by RPC `getnetworkinfo`, as under the hood, -netinfo calls getnetworkinfo to fetch that info.
Of the networks getnetworkinfo returns to you, IPv6 (and CJDNS, which is an IPv6 overlay network) are not reachable, which is wh
...
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613663028)
`static_assert(!std::is_trivially_copyable_v<TrackedObj<1>>)` ?
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613663028)
`static_assert(!std::is_trivially_copyable_v<TrackedObj<1>>)` ?
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613595996)
```suggestion
// Pick one operation based on value of command. Not all operations are always applicable.
// Loop through the applicable ones until command reaches 0 (avoids the need to compute
// the number of applicable commands ahead of time).
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613595996)
```suggestion
// Pick one operation based on value of command. Not all operations are always applicable.
// Loop through the applicable ones until command reaches 0 (avoids the need to compute
// the number of applicable commands ahead of time).
```
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613702058)
I was slightly confused that this was called `RingBuffer` since the data structure itself doesn't act like a ring buffer i.e. will reallocate if adding items beyond capacity (I wrote `vExtraTxnForCompact` to use it before looking at the implementation and then realized I misunderstood the interface). But I now have read that `m_buffer` is the ring buffer - oops.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613702058)
I was slightly confused that this was called `RingBuffer` since the data structure itself doesn't act like a ring buffer i.e. will reallocate if adding items beyond capacity (I wrote `vExtraTxnForCompact` to use it before looking at the implementation and then realized I misunderstood the interface). But I now have read that `m_buffer` is the ring buffer - oops.
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613576870)
nit: Seems like this would be easier?
```suggestion
unsigned idx = real.empty() ? 0 : provider.ConsumeIntegralInRage<unsigned>(0, real.size() - 1);
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613576870)
nit: Seems like this would be easier?
```suggestion
unsigned idx = real.empty() ? 0 : provider.ConsumeIntegralInRage<unsigned>(0, real.size() - 1);
```
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613660947)
```suggestion
// Create entry for this object in g_tracker and populate m_track_entry
void Register()
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613660947)
```suggestion
// Create entry for this object in g_tracker and populate m_track_entry
void Register()
```
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613661502)
```suggestion
// Get value corresponding to this object in g_tracker
std::optional<uint64_t>& Deref()
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613661502)
```suggestion
// Get value corresponding to this object in g_tracker
std::optional<uint64_t>& Deref()
```
💬 glozow commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613637296)
There's an assumption that at least one of the operations is applicable every time. Pretty obviously true but I assigned variables to all the possible conditions while reviewing and used it to add an assertion. Might be easier to read so I figured I'd leave a comment.
```suggestion
const bool non_empty{num_buffers != 0};
const bool non_full{num_buffers < MAX_BUFFERS};
const bool partially_full{num_buffers > 0 && num_buffers < MAX_BUFFERS};
const bool multiple
...
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613637296)
There's an assumption that at least one of the operations is applicable every time. Pretty obviously true but I assigned variables to all the possible conditions while reviewing and used it to add an assertion. Might be easier to read so I figured I'd leave a comment.
```suggestion
const bool non_empty{num_buffers != 0};
const bool non_full{num_buffers < MAX_BUFFERS};
const bool partially_full{num_buffers > 0 && num_buffers < MAX_BUFFERS};
const bool multiple
...
⚠️ BenWestgate opened an issue: "Welcome screen uses GiB values as GB, risking running out of space"
(https://github.com/bitcoin-core/gui/issues/821)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The welcome screen correctly displays "55 GB of space available" while my system has 54322808 bytes available.
But it incorrectly says "... 12 GB of data will be stored in this directory" or "610 GB of data" if I uncheck the 2GB pruning. The values it is adding and comparing do not have the same unit. One is a GUI prune setting and/or space available in GB, the other is actually GiB: h
...
(https://github.com/bitcoin-core/gui/issues/821)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The welcome screen correctly displays "55 GB of space available" while my system has 54322808 bytes available.
But it incorrectly says "... 12 GB of data will be stored in this directory" or "610 GB of data" if I uncheck the 2GB pruning. The values it is adding and comparing do not have the same unit. One is a GUI prune setting and/or space available in GB, the other is actually GiB: h
...
🤔 glozow reviewed a pull request: "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)"
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2077346481)
Concept ACK!
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2077346481)
Concept ACK!
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613746327)
Hmm, this is a good point. The `RingBuffer` class' interface isn't a ring buffer, but a deque; the implementation happens to use a ring buffer. Suggestions/bikeshedding for a better name welcome.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613746327)
Hmm, this is a good point. The `RingBuffer` class' interface isn't a ring buffer, but a deque; the implementation happens to use a ring buffer. Suggestions/bikeshedding for a better name welcome.