fix: address book bucket overflow, decoder panic, stale rollback caches, protoio byte reader, negative-number queries - #6043

What it says on the box

Five independent, small correctness bugs found while auditing peer-discovery bookkeeping and a handful of internal libraries — none of these are crashes or DoS vectors on their own. (A separate, genuinely security-sensitive nil-pointer-dereference finding in the mempool recheck path came out of the same audit; it is intentionally not in this PR and is being routed through coordinated disclosure instead.)

p2p/pex — address book bucket capacity + overflow-recovery losing addresses

addToNewBucket/addToOldBucket used > instead of >= against their bucket-size constants (newBucketSize/oldBucketSize, both 64), so a bucket already at capacity would still admit a 65th entry.

Worse: when moveToOld's overflow-recovery path demotes the oldest entry of a full old bucket back to a new bucket, removeFromBucket never resets the entry's BucketType. addToNewBucket's isOld() consistency guard then rejects the demotion outright — silently, only logged — and since removeFromBucket had already dropped the address from addrLookup, it's gone from the book entirely.

E[...] Error adding peer to old bucket  err="failed consistency check! Cannot add pre-existing address ... into new bucket 221"

Receipts (p2p/pex/addrbook_bucket_overflow_test.go):

$ go test ./p2p/pex/... -v -run 'TestOldBucketRejectsEntryAtCapacity|TestMoveToOldDemotionDoesNotLoseAddress'
--- PASS: TestOldBucketRejectsEntryAtCapacity (0.01s)
--- PASS: TestMoveToOldDemotionDoesNotLoseAddress (0.01s)

Both fail with the predicted symptoms when run against the unfixed source (verified via git stash before writing the fix — not just written and assumed correct).

A Codex adversarial review of the first pass caught that this fix was itself incomplete for legacy state: a book persisted by a pre-fix build can already have a bucket at 65+ entries. Against that state, a single demotion only brings the bucket down to exactly 64 — one short of the room the original promotee still needs on its final re-add attempt — so the promotee itself ended up orphaned (BucketType=old, zero bucket membership, uncounted, unreachable). Fixed by looping the demotion until there's actually room, with a last-resort fallback that keeps the address reachable as new rather than losing it if that still somehow fails. The identical shape existed in addToNewBucket's single-expiry-then-add pattern for a legacy oversized new bucket (would stay at 65 forever); fixed the same way. Both have dedicated regression tests, independently verified red against the Codex-reviewed first-pass fix and green against the final one:

$ go test ./p2p/pex/... -v -run 'TestMoveToOldSurvivesLegacyOverfullOldBucket|TestAddToNewBucketShrinksLegacyOverfullBucket'
--- PASS: TestMoveToOldSurvivesLegacyOverfullOldBucket (0.01s)
--- PASS: TestAddToNewBucketShrinksLegacyOverfullBucket (0.00s)

Full package: go test ./p2p/pex/... → ok (11.1s, 25 tests).

libs/json — 1-byte input panics the 64-bit-integer decode guard

decodeReflect's int64/uint64 branch checks bz[0] != '"' || bz[len(bz)-1] != '"' without first checking len(bz) >= 2. For a 1-byte input this reads the same byte twice, the guard passes (both comparisons happen to be false when that byte is "), and the following bz[1:len(bz)-1] slice becomes bz[1:0] — panic. The sibling time-value guard 30 lines up already has the len(bz) < 2 check this one is missing.

panic: runtime error: slice bounds out of range [1:0]

Reachable via unauthenticated RPC (e.g. GET /block?height="); caught by RecoverAndLogHandler, so this is a wrong-status-code + log-noise bug, not a node crash — stated plainly, not oversold.

Fix: add the missing len(bz) < 2 ||. Receipt:

$ go test ./libs/json/... -v -run 'TestUnmarshal/int64_single_char'
--- PASS: TestUnmarshal (0.00s)
    --- PASS: TestUnmarshal/int64_single_char (0.00s)

Fails with the exact panic above against the unfixed source. Codex found no issue in this fix or its test.

store — DeleteLatestBlock (the cometbft rollback path) leaks the extended-commit row and never evicts commit caches

DeleteLatestBlock deletes the block/commit/seen-commit/meta rows but never deletes the EC: (extended-commit) row, and never evicts any of the three LRU commit caches (blockCommitCache, seenCommitCache, blockExtendedCommitCache). After a rollback + resync with a different block landing at the same height, LoadBlockExtendedCommit can return the stale pre-rollback value straight from cache — the DB itself gets correctly overwritten by the resync, but the cache doesn't know that.

Admin-triggered (requires an operator running cometbft rollback), not remotely reachable — stated as such.

Fix: delete the EC: key in the batch, and evict all three caches for the height. Receipts:

$ go test ./store/... -v -run TestDeleteLatestBlock
--- PASS: TestDeleteLatestBlockEvictsExtendedCommitCache (0.00s)
--- PASS: TestDeleteLatestBlockDeletesExtendedCommitRow (0.00s)
--- PASS: TestDeleteLatestBlockEvictsCommitAndSeenCommitCaches (0.00s)

The first test proves the cache-staleness scenario end to end via LoadBlockExtendedCommit. Codex correctly flagged that this alone doesn't prove the DB row is actually deleted (a resave overwrites it regardless of whether the delete ran) — added TestDeleteLatestBlockDeletesExtendedCommitRow, which reads the raw DB key directly with no resync involved, and extended coverage to LoadSeenCommit's cache too. All three independently verified red against upstream main (the real pre-fix source, not just the first-pass commit) and green after.

Disclosed, not fixed — flagging for a maintainer rather than expanding scope: Load*Commit's DB-read-then-cache-Add sequence isn't performed under bs.mtx. A concurrent DeleteLatestBlock racing an in-flight Load*Commit call could theoretically repopulate a cache entry just after this fix evicts it. This race predates this PR — every existing cache-eviction call site in this file (e.g. PruneBlocks's own blockExtendedCommitCache.Remove) has the identical gap — and closing it properly means broader locking discipline changes across the whole store package, out of scope for a housekeeping PR like this one.

Full package: go test ./store/... → ok (2.1s, 15 tests).

libs/protoio — byteReader mishandles two legal io.Reader return shapes

(0, nil) — legal per the io.Reader doc ("nothing happened yet") — surfaced a stale or fabricated zero byte instead of retrying. (n>0, io.EOF) — also legal, explicitly documented — discarded an already-successfully-read byte along with the error. All existing tests use bytes.Buffer, which happens to never produce either shape.

Fix: loop on (0, nil); return the byte immediately whenever n == 1 regardless of the accompanying error (deferring any non-EOF error to the next call). This exactly matches bufio.Reader's own fill()/readErr() pattern in the standard library — verified by reading the real stdlib source, not assumed. Receipts:

$ go test ./libs/protoio/... -v -run TestByteReader
--- PASS: TestByteReaderIgnoresZeroByteNilErrorReturn (0.00s)
--- PASS: TestByteReaderDoesNotDropByteReturnedAlongsideEOF (0.00s)
--- PASS: TestByteReaderGivesUpOnPersistentZeroByteReader (0.00s)

Codex flagged (correctly) that an unbounded (0, nil) retry loop could hang forever against a pathological reader. Bounded it with the identical maxConsecutiveEmptyReads = 100 → io.ErrNoProgress pattern bufio.Reader itself uses. TestByteReaderGivesUpOnPersistentZeroByteReader uses a goroutine + 5s test timeout to directly prove the unbounded version genuinely hangs (confirmed by temporarily reverting just the bound and watching the test time out) and the bounded version returns cleanly.

Full package: go test ./libs/protoio/... → ok (1.3s, 9 tests).

libs/pubsub/query — numeric operators never match a negative event attribute value

extractNum's regex was ^\d+(\.\d+)? — no support for a leading -. A negative event attribute (e.g. "-5") extracted an empty string, parseNumber returned an error, and =/</<=/>/>= all share the same err == nil && ... guard — so every comparison against a negative attribute silently evaluated to false.

Fix: ^-?\d+(\.\d+)?. Receipts:

$ go test ./libs/pubsub/query/... -v -run TestNegativeNumbers
--- PASS: TestNegativeNumbers (0.00s)
    --- PASS: TestNegativeNumbers/01 .. /08

Note on scope: the query literal itself still can't be negative — the grammar's own number scanner doesn't accept a leading - in query text at all, a separate pre-existing limitation this PR doesn't touch. Tests compare only against a non-negative literal (0), which is sufficient to isolate this fix. Codex correctly caught that an earlier draft of this test included a delta.value = 0 case that was false both before and after the fix (-5 != 0 either way) — removed it and left a comment explaining why = isn't independently provable through the query-syntax path even though the fix covers it via the same parseNumber the ordering-operator tests do exercise.

Full package: go test ./libs/pubsub/... → ok (all three subpackages).

Review process

Ran Codex (gpt-5.6-sol) as a synchronous adversarial reviewer against the first-pass commit before pushing. It surfaced six concrete, real findings — two led to genuine additional fixes (the legacy-state bucket handling, the busy-spin bound), three led to test-quality improvements (the DB-row-vs-cache distinction, the misleading = subtest, the seen-commit coverage), and one (the byteReader n=1+err handling) was investigated against the real Go stdlib and confirmed to already be correct, matching bufio.Reader's own idiom — kept as-is with the reasoning disclosed above rather than silently ignored or blindly changed.

What's not in this PR

A separate, genuinely security-sensitive finding from the same audit pass — an unrecovered nil-pointer dereference in the mempool recheck path (findNextEntryMatching), reachable under a specific recheck-timeout + stale-response timing condition — is deliberately excluded. Publishing the exact trigger conditions in a public diff before a fix ships would be irresponsible for a consensus node's mempool; it's being routed through coordinated disclosure instead.