perf(shrex/client): reduce memory usage by removing the buffer

[vgonkivs]

Resolves PROTOCO-2384
Reduce memory usage by removing an internal buffer. And stream the EDS directly.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[greptile-apps]

Greptile Summary

GetEDS now consumes ODS shares directly from the SHREX stream and reconstructs the EDS after the transfer completes. Focused checks confirmed successful streamed retrieval, cancellation and not-found handling, and rejection of a cleanly truncated response through final integrity verification. One security issue remains in the Dependabot tidy workflow: mutable action tags run before a write-capable branch push.

Confidence Score: 3/5

The streamed EDS behavior exercised by focused tests is safe, but the write-capable Dependabot workflow remains exposed to mutable third-party action references.

There is exactly one actionable P1 finding, and it is a security finding, so the confidence score is 3.

Files Needing Attention: .github/workflows/dependabot-gomod-tidy.yml

T-Rex Logs

What T-Rex did

• T-Rex produced a proof for the posted P1 finding and linked it to the corresponding review comment for details.

_{Ran code and verified through T-Rex}

Comments Outside Diff (1)

1. .github/workflows/dependabot-gomod-tidy.yml, line 34 (link)

   An executed workflow-source extraction captured the job permission, both action referen...

   • Bug
     • An executed workflow-source extraction captured the job permission, both action references, and the authenticated push. The job grants contents: write, runs actions/checkout@v7 and actions/setup-go@v7 through mutable tags, then commits tracked changes and pushes with GITHUBTOKEN. A retargeted action can therefore execute in the write-capable job before publication and alter the Dependabot branch.
   • Cause
     • T-Rex reproduced this while running the changed behavior, but it did not return a separate root-cause sentence.
   • Fix
     • Update the changed code so this failing path is handled, then rerun the same T-Rex check to confirm it passes.
   Artifacts

   
   

   Command output from the check

   • Executed source extraction captures exact lines 21-22, 34-41, and 52-65 showing the write-capable token, mutable action tags, and subsequent authenticated branch push.

   

   _{Ran code and verified through T-Rex}

_{Reviews (6): Last reviewed commit: "defer EDS reconstruction off the libp2p ..." | Re-trigger Greptile}

[renaynay]

Do we really need countingReader ? I get that we want real bytes tx'd over the wire but we can get these metrics from libp2p bandwidth reporter (for the shrex-eds protocol) and even get per-peer metrics as well.

I won't die on the hill but 🤷🏻‍♀️

[vgonkivs]

Good call. For this PR I'll keep countingReader bc it just preserves the existing per-request "bytes received" log/trace that the removed bytes.Buffer gave via buff.Len(), so no behaviour change. Wiring shrex-eds traffic to the libp2p bandwidth reporter (per-peer/per-protocol) and dropping countingReader is a self-contained metrics change, so I'd propose to do it in a follow-up pr

[renaynay]

This solution will block libp2p stream til square is reconstructed + hashed.

Rather do something like this:

  type edsResponse struct {
      shares []libshare.Share
      eds    *eds.Rsmt2D
  }

  func (r *edsResponse) ReadFrom(src io.Reader) (int64, error) {
      cr := &countingReader{r: src}

      shares, err := eds.ReadShares(cr, libshare.ShareSize, r.odsSize)
      if err != nil {
          return cr.n, err
      }

      r.shares = shares
      return cr.n, nil
  }

leave reconstruction logic to build func that's also passed in to executeRequest

[vgonkivs]

This solution will block libp2p stream til square is reconstructed + hashed.

Good catch. Fixed

[renaynay]

Ah sorry, last request hopefully last back and forth:

let's not read too much from the stream (1x per share read), you can wrap the stream in a smaller (like 64kib) buffered reader and read the shares from that instead. So there will be tiny allocation but better than reading from stream num_shares amount of times.

[vgonkivs]

Let me clarify smth:

1. adding and internal buffer per share will add a copy and not just allocation: bufio.Reader read at 512B (< bufsize) always goes through its internal buffer so we'd get two copies per byte. Currently io.ReadFull(stream, shr) copies once, straight into the share's backing.
2. stream.Read is not a syscall here. IT's not a per-socket read and num sahres read != network roundtrips, rthaer than a per-read muxer overhead. And the window-update frames are already throttled (GrowTo/needed gate), so the win is really just saving mutex/call overhead
3. Today it's one EDS per stream, framed implicitly by exact byte length and per-share ReadFull stops right on the boundary. If we ever stream a range of EDSs back-to-back over one stream, a bufio created inside ReadFrom would pull a 64KiB chunk containing the next EDS's bytes, then get discarded on return. These bytes are lost and the next square is corrupted. Buffering inside ReadFrom bakes in "one EDS per stream."

My suggestion is to keep the solution as you proposed initailly w/o another buffer. Adn do buffering at the stream level when we will be implementing range-eds(we could have io.LimitReader per eds per stream)