💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454613258)
> `getInt` is used in many RPCs. What makes this call to `getInt` in this RPC special?

Thank you, @maflcko, for your feedback. I believe that `getInt` in `lockunspent` is special because the vout param specifically represents transaction output indexes, which are always whole numbers. Since this value comes directly from user input, providing a descriptive error message would be very helpful.

I also updated the PR description to add motivation for this change.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827415495)
Nit: Given that `MallocUsage` warns that it `is *not* recursive`, would have been great to add a
```c++
requires std::is_trivial_v<X>
```
here, but apparently it *is* used for nested structures - nothing to do here, just documenting my journey.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827342209)
Q: `v.capacity()` can be significantly more here than `v.size()`, right?
Is it because we can't size them properly in https://github.com/bitcoin/bitcoin/blob/master/src/streams.h#L82?

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