💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827424352)
Q: we assume that `sizeof(v)` (the vector's fixed memory) was already included in the calculation before this call, right?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827341320)
Nit: Could we find better names than X and Y (e.g. to avoid readers thinking Y is the size), maybe something like `template<typename ElementType, typename AllocatorType>`

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827573203)
Slightly unrelated, but `MemUsage` could be generalized to explain the alignment and rounding:

<details>
<summary>Details</summary>

```C++
static inline size_t MallocUsage(const size_t alloc)
{
if (alloc == 0) return 0;
constexpr size_t alignment{2 * sizeof(void*)};
constexpr size_t round_up{alignment * 2 - 1};
constexpr size_t mask{~(alignment - 1)};
return alloc + round_up & mask;
}
```
and
```C++
BOOST_AUTO_TEST_CASE(malloc_usage)
{
auto referenceM
...

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827588117)
I don't actually understand why we're ignoring `m_type`, the comment doesn't help.

Is it because of https://github.com/bitcoin/bitcoin/blob/master/src/netmessagemaker.h#L17, i.e. the string being moved around instead of being owned?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827592846)
we still have a few `m_raw_message_size` usages, can you please explain why that's still needed?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827589976)
given that we're ignoring `m_type`, is it still fair to say that we're counting `total memory usage`?

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827709219)
The comment in the referred function explains it.
```
// Don't count the dynamic memory used for the m_type string, by assuming it fits in the
// "small string" optimization area (which stores data inside the object itself, up to some
// size; 15 bytes in modern libstdc++).
```
It has to do with the internal implementation of std::string, where it has a fixed-size buffer that it will use for short strings (such as the message types) before allocating memory on the heap.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827714339)
i do agree that over-counting is probably better than under-counting here, but i just took this over from `CSerializedNetMsg` as i assumed this was the right thing to do. If we change it it needs to be changed in both places.

💬 maflcko commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454689466)
Every use of `getInt` represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?

Tend toward NACK for now, unless there is a reason.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827719079)
Right, it can, when `reserve()` is called with a larger amount, or when the vector is shortened. But it does represent the actually-allocated size which seems what we want here.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827719853)
Can we measure whether this is indeed the case? What is the reason for `assuming it fits in the "small string" optimization area`?

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827720254)
Correct, it's computes the dynamic usage only.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827722013)
It's the total memory, making the assumption about the underlying implementation of std::string. Could add that here too, sure.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827724311)
Don't need to duplicate it, just want to make sure I understand this exceptional case.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827725377)
It's still useful for one purpose: to count bytes going over the network for bandwidth monitoring. This icnludes the statistics per message type.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827726883)
Got it, thanks for clarifying!

💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827728402)
Message type strings are at most 12 bytes long (that's the limit in both the v1 and v2 transport protocol), while typical standard library implementation allow this optimization up to 15 bytes.

We could actually test for it exactly though: add a `DynamicUsage(const std::string& s)` function which checks if `s.data() >= &s && s.data() + s.size() - sizeof(s) <= &s`, and if so, return 0.

💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2454719003)
Rebased 3ab9a46ac67964c9d5ad54120b680b5eee7f16f0 -> 05438605ba95df14705bf2f57924455f3b2e40e1 ([`pr/fatalresult.21`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.21) -> [`pr/fatalresult.22`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.22), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.21-rebase..pr/fatalresult.22)) due to various conflicts. Still have not moved InterruptResult from success type to failure type yet (https://github.com/bitcoin/
...

💬 ryanofsky commented on pull request "Add util::ResultPtr class":
(https://github.com/bitcoin/bitcoin/pull/26022#issuecomment-2454732698)
Rebased 5f6f91c5fd848fecdc71db25ce1c5ddbedc78f29 -> c7e73097f4a8336e23d098c2fb4296319aba318e ([`pr/bresult-ptr.8`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-ptr.8) -> [`pr/bresult-ptr.9`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-ptr.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-ptr.8-rebase..pr/bresult-ptr.9)) on top of updated base PR (no changes or conflicts)

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-2454742154)
Rebased 2b0e70295ed6ff4be91cc5775ddaa9351a1ed430 -> caed6aec4241a2e5d849c9446725c889348e1b7e ([`pr/bresult-load.30`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.30) -> [`pr/bresult-load.31`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.31), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-load.30-rebase..pr/bresult-load.31)) due to various conflicts