Skip to content

response memory pools are not authoritative: several allocation owners never charge them #1002

Description

@czarcas7ic

The shared response-memory pools, 128 MiB per node and 16 MiB per connection, are presented as a cross-service ceiling, but several allocation owners never charge them. The ceiling is therefore not authoritative, and a full pool does not prevent the next allocation.

Verified

The request-write registry is a plain HashMap created empty, and the funding plan that covers a request's metadata does not include it:

pub(in crate::zakura::block_sync) fn metadata_bytes(max_count: usize) -> Option<u64> {

The plan charges the taken-work buffer and two shared allocations. Publication then inserts one BodyWorkOwner to RequestWriteRegistration entry per request into that map, so both the first allocation and every growth step happen outside the pools, and the map keeps its peak backing capacity after entries are removed. At the in-flight request cap this is single-digit megabytes node-wide against a 128 MiB pool: modest, but outside the bound that is advertised as covering it.

The map predates the funding plan, arriving with the request-write ownership work, so this is an incomplete bound rather than a regression.

Reported, not yet verified

An audit of the same area lists five more owners that receive the connection's memory handle and never reserve from it: header sync session and command-channel setup, discovery exchange progress, GetBlocks serving's work resources and execution owners, legacy responder bookkeeping, and legacy outbound response receivers. It also notes that an external Service implementation has no way to charge the pool at all, since the accessor and the regulator types are not public. Each needs its own check before anyone acts on it; the pattern is consistent enough to treat as one piece of work.

Two shapes for a fix

  1. Charge the marginal cost at admission. For the registry that is roughly three lines, one entry's worth per request. It is cheap and covers the steady state, but it understates a hash map's real per-entry cost and does not account for retained peak capacity, so the number would imply more precision than it has.
  2. Move the owner into a funded container, as the outstanding window and response indexes already are. Correct, and the only option that covers growth and retention, but it needs a funded map, which does not exist yet.

Deciding which owners must charge before 1.0, and correcting the documentation for the ones that will not, is part of the work. Claiming a ceiling that several paths ignore is worse than a smaller documented one.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    blocksyncanything related to blocksyncp2pLegacy Zcash P2P stack

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions