chore: block proposer fixtures become bls valid#627
Conversation
Greptile SummaryThis PR replaces sequential-byte BLS placeholder values in the Capella and Deneb block fixtures with canonical valid compressed BLS12-381 points, fixing
Confidence Score: 5/5Safe to merge — the fixture data change is additive and self-contained, and the //go:embed refactor is mechanical. The changed code replaces two large inline string literals with embedded JSON files that now carry valid BLS12-381 compressed points. The substituted values have the correct byte lengths (48-byte G1 for pubkeys, 96-byte G2 for signatures) and correct compression-bit prefixes. All non-BLS fields are preserved from upstream. The embed import and directives are syntactically correct. No logic paths, exports, or interfaces are altered. No files require special attention; the only gap is the BLS-decodability regression test described in the PR summary but absent from the diff. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[beacon_node_consts.go] -->|go:embed| B[testdata/capella_block.json]
A -->|go:embed| C[testdata/deneb_block_contents.json]
B --> D[capellaBlock]
C --> E[denebBlockContents]
D --> F[TestingBeaconBlockCapella]
E --> G[TestingBlockContentsDeneb]
F --> H[TestingBeaconBlockV]
G --> H
B -. BLS fields replaced .-> I[Valid G1 pubkey + G2 signature]
C -. BLS fields replaced .-> I
Reviews (2): Last reviewed commit: "refactor: cappela and deneb block data j..." | Re-trigger Greptile |
| rewritten := bytes.Clone(literal) | ||
| for v := range pubkeys { | ||
| rewritten = bytes.ReplaceAll(rewritten, []byte(`"`+v+`"`), []byte(`"`+validPubkey+`"`)) | ||
| } | ||
| for v := range sigs { | ||
| rewritten = bytes.ReplaceAll(rewritten, []byte(`"`+v+`"`), []byte(`"`+validSig+`"`)) | ||
| } | ||
| if len(rewritten) != contentEnd-contentStart { | ||
| log.Fatalf("%s: substitution changed literal length", name) | ||
| } | ||
| copy(out[contentStart:contentEnd], rewritten) |
There was a problem hiding this comment.
ReplaceAll can silently corrupt non-BLS fields that share a hex value
discoverBLSValues collects the hex strings found in BLS-typed positions, then bytes.ReplaceAll swaps every quoted occurrence of those strings anywhere inside the literal. A Deneb block contains KZG commitments (48 bytes, same width as a BLS pubkey) and KZG proofs (96 bytes, same width as a BLS signature). If any KZG field happens to carry the same sequential byte pattern as a discovered BLS field, it will be silently replaced with a valid BLS point—corrupting a field the PR explicitly says should be "preserved byte-for-byte from upstream". The current fixture data is safe (verified: the 4 unique 48-byte values in denebBlockContents are all distinct), but this invariant is fragile: a go-eth2-client test-data bump that changes byte offsets could silently introduce the collision without the length-check at line 95 catching it. Consider replacing the values at the byte offsets discovered during the walk (e.g., by encoding their positions while unmarshalling) rather than doing a content-addressed search-and-replace.
There was a problem hiding this comment.
Good catch, fixed in 3e2f851. discoverBLSValues now returns per-value counts, and rewriteLiteral asserts that each BLS hex appears in the literal exactly as many times as the walker found at BLS-typed positions before substituting. If a future upstream bump puts a KZG commitment/proof byte-identical to a discovered BLS placeholder, the count check fails and the script aborts before any replacement happens.
Didn't go all the way to offset-tracked replacement since the count check covers the only practical corruption mode (literal count strictly greater than walker count, i.e., a non-BLS field shares the value) without re-marshaling JSON.
|
On the Comments Outside Diff item about |
|
Thanks for putting this together. I think the core fix is right: After looking through the PR, I think we can make the shape simpler and easier to review. The current walker and regen script make sense if we expect to refresh these upstream fixtures often, but for #622 the main issue is the checked-in fixture data. I would prefer fixing the source fixtures directly and keeping the change focused. I also think this PR is a good chance to move the giant Go raw strings into With that shape, I do not think we need a separate fixture validation test or a regeneration framework in this PR. The Go tests do not currently catch invalid compressed BLS points because So my preferred version would be:
|
|
Good idea! Pushed the reshape in ab5d985 Mapped to your bullets:
Removed: walker, regen script, |
|
The Lint failure on this PR is coming from the golangci-lint installer URL, not from this diff. I opened #629 to switch the repo from the retired The Sync failure is separate and still needs to be handled independently. |
|
Superseded by #630 — same branch pushed directly to ssvlabs/ssv-spec so CI runs without fork restrictions. |
Summary
The
capellaBlockanddenebBlockContentsfixtures intypes/testingutils/beacon_node_consts.gowere copied from go-eth2-client's codec tests, where BLS-typed fields are sequential-byte placeholders (0x000102…, etc.). Byte lengths are correct but the bytes are not valid compressed BLS12-381 points, so strict decoders (Lighthouse'sblscrate viablst) reject them withBLST_BAD_ENCODING. This blocks cross-client consumers like Anchor'sspec_testsfrom running real BLS validation on these fixtures.Replace every BLS-typed value in those two literals with a canonical valid pubkey + signature derived from
Testing4SharesSet().ValidatorSK. Non-BLS fields (KZG commitments/proofs, transactions, blobs, roots, etc.) are preserved byte-for-byte from upstream. Closes #622.Changes
capellaBlockanddenebBlockContentswith a canonical valid pubkey + signature; everything else byte-identical to upstream.TestProposerFixturesBLSDecodable(capella + deneb subtests) asserting every BLS field strict-decodes via herumi. Electra/Fulu use real Pectra-devnet-6 bytes and already strict-decode; not re-tested.internal/proposerbls/with a sharedVisitorwalker over Capella/Deneb beacon block bodies, as single source of truth for "which fields are BLS-typed", consumed by both the test and the regen script.//go:build ignoreregen script that locates each named literal by prefix match, JSON-decodes into the typed go-eth2-client struct, walks BLS fields via the shared visitor, and substitutes values in-place inside the literal range. Length-preserving and no JSON re-marshalmake generate-jsonsto be read-only on source: runs the BLS-decodability test as a precondition, then regenerates JSONs. Addmake regen-proposer-fixturesas the explicit fix tool. Source mutation is developer-driven, never an implicit side-effect of JSON regen.When to regenerate (after a go-eth2-client bump)
make regen-proposer-fixtures # rewrites beacon_node_consts.go in place
review the diff, commit the source fix
make generate-jsons # precondition test passes; regenerates JSONs
Limitations
internal/proposerbls/walk.goneeds updating. Both the script and the regression test consume it.