📝 LarryRuane opened a pull request: "bugfix: account for system-allocated memory in pool resource"
(https://github.com/bitcoin/bitcoin/pull/27748)
Follow-on to PR #25325, "Add pool based memory resource"

The `DynamicUsage()` function for the version of `unordered_map` that uses the `PoolAllocator` returns a close approximation of the amount of physical memory used by the map. (This is the map used for the dbcache.) It accounts for several of the allocator's internal data structures, such as the memory chunks, the freelist, and the unordered_map's bucket list. But it doesn't account for memory that had to be obtained from the system allo
...

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204957663)
This test may be somewhat fragile; it depends on `MallocUsage(8) == 32`. Maybe the test condition here should be `8 <= resource.SystemBytes()`, but I think that doesn't test enough.

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204956602)
Copied this from https://github.com/bitcoin/bitcoin/blob/a13f3746dccd9c4ec16d6bfe9b33ebd26e3238e1/src/memusage.h#L50 because including that file would create a circular dependency. Is there a better way to do this?

💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1562231512)
> You can leave this open and rebase it onto the main repo PR, just mention it in the description.

All right, I'll start working on this refactor.
Btw, do we have some docs on how to setup git locally so I can have dependent changes across the "bitcoin" and the "gui" repos? So far I was moving changes via patches but that doesn't seem too convenient.

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562239541)
How much of a difference does this make? I ran IBD on Ubuntu using the default `dbcache` (450) and `maxmempool` (300), which gives a total cache size (during IBD) of about 750 MB, and this (missing) system allocation amount was 97 MB (96941536 bytes).

With this amount of allocation omitted from the memory usage estimate (that is, current master), nodes will use 97 MB more memory than expected, or about 13% more than configured, a pretty significant error. This regression could cause surprises
...

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204976258)
CI failed here (on 32-bit), so I force-pushed to make the test more precise.

💬 martinus commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204991822)
I don't think that calculation is now more correct, in fact this now counts the the index array twice; once in `usage_system` and once in the estimated `usage_buckets`.

The most correct way is probably to just let the pool do all the malloc & free accounting, and don't do any other estimation. Then the `DynamicUsage` method boils down to something like `return m.get_allocator().resource().memory_usage();`. That would give more theoretically correct numbers, but in practice I doubt that this i
...

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205009503)
I see, yes, it's counting the buckets twice, thanks. I agree that either way gives the same answer; the way I'm suggesting here would isolate this pool code a little more from the details of `std::unordered_map` (i.e., the existence of the `m.bucket_count()` method), which would be a little more general, but I think the way it is now is fine. I guess if we use this pool resource for a different kind of container (for example, an ordered map), then this PR's approach would be better since we woul
...

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562291221)
Closing at least for now, see the [comment](https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205009503) above.

✅ LarryRuane closed a pull request: "bugfix: account for system-allocated memory in pool resource"
(https://github.com/bitcoin/bitcoin/pull/27748)

💬 martinus commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562304483)
Also related: #26614

💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1205026761)
> And in the wallet, we use the clock time to set the descriptors/keys creation time.

Ok, thanks for the background. However, I think the wallet uses mock time (NodeClock) for the times, so a much simpler fix would be just a single line patch to set the mock time to the genesis block time in the wallet bench that needs it?

💬 MarcoFalke commented on pull request "Use 'byte'/'bytes' for bech32(m) validation error message - 27727 follow up.":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205031671)
```suggestion
std::string_view byte_str{data.size() == 1 ? "byte" : "bytes"};
```

nit: snake_case per dev notes and string_view to avoid a copy?

👍 MarcoFalke approved a pull request: "Use 'byte'/'bytes' for bech32(m) validation error message - 27727 follow up."
(https://github.com/bitcoin/bitcoin/pull/27747#pullrequestreview-1443136434)
lgtm

💬 martinus commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205035430)
I agree, also with current memory accounting code its not possible to use the same memory resource for multiple containers. So it would be nice to add full accounting to the pool, and then remove the special handling of std::unordered_map with pool and just count the ressource's total usage.

💬 MarcoFalke commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1205038328)
I don't think this is correct. Dereferencing an end-iterator is UB, so there is just no way to safely proceed in production, unless you add an early return (no idea if that is possible or even safe)

💬 russeree commented on pull request "rpc: Use 'byte'/'bytes' for bech32(m) validation error message":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205046324)
Applied, committed, squashed, and pushed.

- Is there any need for `const` keyword with std::string_view since it is read only?

- Snake case has been remedied.

💬 MarcoFalke commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1205046358)
It's removed, no?

💬 MarcoFalke commented on pull request "rpc: Use 'byte'/'bytes' for bech32(m) validation error message":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205048572)
```suggestion
std::string_view byte_str{data.size() == 1 ? "byte" : "bytes"};
```

💬 russeree commented on pull request "rpc: Use 'byte'/'bytes' for bech32(m) validation error message":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205053788)
My apologies for not being attentive to detail. This has now been remedied.