Skip to content

fix(std/compress): correctly widen NumReader window in SetNumNbBits - #1865

Open
somasoil wants to merge 1 commit into
Consensys-Incorporated:masterfrom
somasoil:fix/num-reader-setnumnbbits
Open

somasoil wants to merge 1 commit into
Consensys-Incorporated:masterfrom
somasoil:fix/num-reader-setnumnbbits

Conversation

@somasoil

@somasoil somasoil commented Oct 5, 2026 •

Copy link
Copy Markdown

Description

NumReader.SetNumNbBits widens the sliding window of an already-running reader. The
current code computed the widened value incorrectly whenever wordNbBits > 1 or
the reader had advanced past the first window, and it also truncated the remaining
input, which silently corrupted all subsequent reads.

This supersedes #1609, whose fix was closed for "Cannot quickly evaluate correctness
of the PR. Lacking regression tests."
This PR contains the same core fix plus the
missing regression tests, so the change is now verifiable.

The fix:

  • scale the old number by radix^(n'-n) instead of 2^(n'-n),
  • read the appended words from right after the current window
    (nr.toRead[n:n']) instead of from the head,
  • do not mutate nr.toRead — next() relies on a stable start index.

The change adds no constraints relative to the buggy version (see Benchmarking).

Fixes #1864

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

  • TestSetNumNbBitsAfterNext: a new table-driven test covering wordNbBits ∈ {1,2,4,8}
    and several resize schedules, asserting against an independent native oracle
    (numReaderNums). It fails on master and passes with the fix.
  • TestSetNumNbBits: removed the leftover words = []byte{1,0}; increases = []byte{1}
    override that disabled the randomised test (and guarded the index into buf). It now
    actually exercises the nr.last != nil resize path and fails on master.
  • Full ./std/compress/... suite passes (go test ./std/compress/... -count=1).

How has this been benchmarked?

Constraint count is unchanged: the resize path still performs one api.Mul + one
api.Add (plus a second api.Mul only when the window runs past the end of the
input), identical to the original code. The no-resize path (wordsPerNum unchanged)
is now skipped entirely. No benchmark run required; -short safe.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (inline godoc comments clarified)
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally (gofmt + go vet clean)
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Note

Medium Risk
Corrects constraint-circuit number packing used in compress paths; wrong values could have silently broken proofs, but the fix is localized with new regression tests and unchanged constraint shape on the resize path.

Overview
Fixes NumReader.SetNumNbBits when the reader is already running and the bit window is widened. The old path scaled the held value with 2^(n'-n) instead of radix^(n'-n), pulled new words from the wrong offset in toRead, and truncated toRead, which broke later Next() reads whenever wordNbBits > 1 or the head had advanced.

The resize logic now widens nr.last in place (head unchanged): multiply by r^(n'-n), append words from toRead[n:n'], zero-pad past the buffer end, and leave toRead untouched so next() still shifts from index 0.

Tests: adds TestSetNumNbBitsAfterNext (several resize schedules and wordNbBits ∈ {1,2,4,8}) against a native oracle numReaderNums; restores randomized TestSetNumNbBits (drops the hard-coded override, bounds increases indexing); test circuit takes configurable wordNbBits.

Reviewed by Cursor Bugbot for commit a6cc814. Bugbot is set up for automated code reviews on this repo. Configure here.

SetNumNbBits corrupted the sliding window when called on a running reader
(nr.last != nil) whenever wordNbBits > 1 or the reader had advanced:
  - scaled by 2^(n'-n) instead of radix^(n'-n) = 2^((n'-n)·wordNbBits),
  - read the appended words from the head of toRead instead of after the
    current window,
  - truncated toRead, discarding input used by subsequent Next() calls.

Widen the window in place (scale by radix^(delta words), read the appended
words from the correct slice, do not mutate toRead) and add regression tests.

This supersedes Consensys-Incorporated#1609, which was closed for lacking regression tests.

Tests: add TestSetNumNbBitsAfterNext (wordNbBits in {1,2,4,8}) and restore the
randomised TestSetNumNbBits, which previously overwrote its generated data with
words={1,0};increases={1}, skipping the nr.last != nil path entirely.
@somasoil
somasoil requested a review from a team as a code owner October 5, 2026 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: NumReader.SetNumNbBits corrupts the sliding window when called after Next

1 participant