[QuantumExplorer]

Basic explanation

Dash Platform wants fair same-block randomness for games: a card's rarity or a token payout that nobody can predict or pick, set in the block that asks for it. The random value comes from the validators: when enough of them prevote for a block, their BLS signature shares combine into one threshold signature that nobody could compute in advance. That signature is the random beacon.

This PR adds the ABCI method that hands the beacon to the application, and the commit fields that will carry it. Nothing calls the method yet; a follow-up PR switches it on at consensus version 2.

Before, the application had no way to learn the beacon, and a commit carried only its threshold signature:

Commit { height, round, block_id, quorum_hash, threshold_block_signature, threshold_vote_extensions }

After, there is a new call, and commits have room for the beacon and the app hash the precommits sign:

ApplyBlockRandomness { hash, height, round, beacon }  ->  { app_hash }
Commit { ..., beacon, app_hash }
CommitInfo { ..., beacon, app_hash }   (passed to FinalizeBlock)

TL;DR: Add the ApplyBlockRandomness ABCI method and beacon/app_hash fields on commits. Nothing calls the method yet, and nodes keep both fields empty before consensus version 2.

User story

As a Dash Platform developer, I want Tenderdash to give Drive a random beacon for each block, so that game contracts can get unpredictable values in the same block that requests them.

Scenario

Base flow

A Platform contract declares a random property. A user creates a document; the block runs it with a placeholder. Once the prevotes for the block reach the threshold, the beacon exists, and Drive should replace the placeholder with a value derived from it before the precommits sign the final state.

Actual behavior

There is no ABCI call that carries a beacon to the application, and the commit has nowhere to record the beacon or the final app hash.

Expected behavior

The ABCI interface, clients and commit format can carry the beacon and the final app hash. The consensus change that uses them comes in the stacked follow-up PR.

Detailed discussion

What was done

• proto: RequestApplyBlockRandomness {hash, height, round, beacon} and ResponseApplyBlockRandomness {app_hash}, added to the Request/Response oneofs and the ABCIApplication service; CommitInfo.beacon = 5 and CommitInfo.app_hash = 6; Commit.beacon = 7 and Commit.app_hash = 8. Regenerated Go code, mocks and spec/abci++/api.md; documented the method in spec/abci++/abci++_methods.md (and brought its stale CommitInfo table in line with the proto).
• Go ABCI: Application.ApplyBlockRandomness; socket, gRPC, routed and local clients; socket server; proxy client with a timing metric (apply_block_randomness).
• BaseApplication.ApplyBlockRandomness returns ErrApplyBlockRandomnessNotImplemented. Every other base method returns an empty response, but an empty app_hash must never pass as an answer.
• kvstore example: returns, unchanged, the app hash that ProcessProposal left for the requested block hash at that height and round, and refuses a block it did not process there.
• types.Commit: new Beacon and AppHash fields, carried through protobuf and ToCommitInfo (JSON omitempty, so RPC output is unchanged while they are empty). The new MaxCommitRandomnessBytes (132: 96+2 for the beacon and 32+2 for the app hash, including their protobuf tags) is what a commit grows by when it carries them; MaxCommitOverheadBytes is unchanged, so the mempool reserves nothing more until the follow-up adds it at consensus version 2.
• Contract: the docs (proto, abci++_methods.md, the call grammar in abci++_tmint_expected_behavior.md) say that vote extensions and their verification must not depend on the beacon, that the randomness may change only the state behind the returned app hash (transaction results, events and validator set or consensus parameter updates stay those of ProcessProposal), that from consensus version 2 FinalizeBlock commits the candidate with the commit's beacon applied and its root must equal commit.app_hash, and that the method is called on every finalizing path, a validator's own commit included.
• ABCISemVer 1.4.0 → 1.5.0, as RELEASES.md asks for an interface change. Commit.String, StringIndented and the commit's log fields show the beacon and app hash.
• Size bound on the new commit fields. Neither the commit hash nor its threshold signature covers beacon or app_hash, so a peer relaying a validly signed commit could fill them with any bytes. Commit.ValidateBasic now refuses a commit unless both are empty or exactly 96 and 32 bytes (ErrInvalidCommitRandomness), whatever the consensus version. Every path that accepts a commit on its own goes through it (block sync responses, consensus Commit messages, light blocks, stored commits), so a padded commit is refused before it is stored or passed on. A block's LastCommit is not refused for them, because releases before this one skip the fields as unknown and refusing the block would split upgraded nodes from them: BlockFromProto reads it without the malformed fields instead.
• Tolerant block decoding. Releases before this one skip fields 7 and 8 of a commit whatever their wire type, while the generated decoder refuses a non-bytes encoding. types.UnmarshalBlockProto decodes as before and, only when that fails, drops those mistyped occurrences from the block's LastCommit and decodes again; it allocates nothing unless it drops a field, and then one buffer. Consensus part assembly and BlockStore.LoadBlock use it, so during a rolling upgrade an upgraded node decodes every block an older one commits. google.golang.org/protobuf becomes a direct dependency for its wire format writer.
• Beacon and app hash stay empty before consensus version 2. A peer can add a correctly sized beacon and app hash to a validly signed commit it relays, and a proposer can put them in its block's LastCommit. Nodes drop both (Commit.WithoutRandomBeacon) where they adopt a commit (consensus Commit messages, block sync, state sync and its backfill), from the commit a proposal builds on (CreateProposalBlock, so older releases, which drop the fields when they decode a block and rebuild its parts, can still block sync it), and from the last commit PrepareProposal and ProcessProposal pass to the application. The follow-up keeps and verifies them from consensus version 2.
• Docs: the Commit tables in spec/core/data_structures.md, spec/consensus/consensus.md and consensus-commit.md list the new fields and their rules; Requirement 10 in abci++_app_requirements.md, abci++_basic_concepts.md and the adapting-applications list cover ApplyBlockRandomness, which must not modify the committed state; the proto and spec say the commit signs app_hash only through its state ID; UPGRADING.md has an entry for the change, including routing the method with the routed ABCI transport.

Nothing calls the new method and honest nodes never set the new fields, so consensus behaviour is unchanged. The visible effects are that a commit with a wrongly sized beacon or app hash is refused, that a correctly sized one a peer or proposer adds is dropped instead of being stored or passed to the application, and that RequestInfo.abci_version is 1.5.0.

Before the size bound, a block sync peer could serve an honest commit with a 5 MiB beacon:

BlockResponse { block H, commit(H) with beacon = 5 MiB }  ->  signature verifies, commit stored, FinalizeBlock over gRPC fails, node crashes on every restart

After, the response is refused before anything is stored:

BlockResponseFromProto  ->  invalid commit beacon or app hash size: beacon of 5242880 bytes and app hash of 32 bytes, expected both empty or 96 and 32 bytes

Testing

Targeted tests on the touched packages:

go test -run 'TestCommitBeaconFields|TestMaxCommitBytes|TestCommit$|TestBlockMaxDataBytes|TestCommitValidateBasicRandomnessSizes|TestBlockFromProtoMalformedLastCommitRandomness|TestUnmarshalBlockProtoMistypedLastCommitRandomness' ./types/
go test -run 'TestBlockResponseRefusesPaddedCommitBeacon|TestBlockSyncDropsRelayedCommitRandomness' ./internal/blocksync/
go test -run TestCommitMessageRefusesMalformedRandomness ./internal/consensus/
go test -run TestLoadBlockMistypedLastCommitRandomness ./internal/store/
go test -run TestBaseApplicationApplyBlockRandomness ./abci/types/
go test -run 'TestApplyBlockRandomness|TestApplyBlockRandomnessEquivocation|TestClientServer|TestKVStore|TestProcessProposalRoundReprocessing' ./abci/example/...
go test -run 'TestRouting|TestRoutedClientGrpc' ./abci/client/
go test -run 'TestApplyBlockRandomnessMetrics|TestFailureMetrics' ./internal/proxy/
go test -run 'TestTxFilter|TestProposalDropsRelayedCommitRandomness' ./internal/state/
go test -run 'TestMaxProposalBlockSize|TestCreateProposalBlock' ./node/

TestClientServer now calls ApplyBlockRandomness over both the socket and the gRPC transports. Regression tests for the review fixes:

• TestBlockResponseRefusesPaddedCommitBeacon: a block sync response whose signed commit has its beacon padded (25 MiB, 5 MiB) or an app hash without a beacon is refused with ErrInvalidCommitRandomness.
• TestCommitMessageRefusesMalformedRandomness and TestCommitValidateBasicRandomnessSizes: the same for consensus Commit messages and every size combination.
• TestBlockFromProtoMalformedLastCommitRandomness: a block whose LastCommit has malformed fields still decodes, without them, and keeps its hash.
• TestUnmarshalBlockProtoMistypedLastCommitRandomness and TestLoadBlockMistypedLastCommitRandomness: blocks whose LastCommit carries field 7 or 8 as a varint, fixed32 or fixed64 decode (and load from the store) as an older release reads them; truncated bytes still fail.
• TestCommitBeaconFields encodes and decodes Commit and CommitInfo on the wire and compares the whole structs; TestRouting and TestApplyBlockRandomnessMetrics cover the routed client and the proxy client's apply_block_randomness metric.
• TestApplyBlockRandomnessEquivocation: the kvstore answers for the requested block after an equivocating proposer's two blocks in one round, and refuses a block it did not process.
• TestProposalDropsRelayedCommitRandomness: a proposal built from a relayed commit with a correctly sized beacon and app hash drops them and keeps the parts an older release rebuilds (a block that kept them would not), and neither PrepareProposal nor ProcessProposal passes them to the application.
• TestBlockSyncDropsRelayedCommitRandomness: block sync of a commit with a correctly sized beacon and app hash added neither passes them to FinalizeBlock nor stores them in the seen commit.

make proto-gen, make mockery and make lint (new findings against v1.8-dev) are clean.

Breaking changes

• The ABCI Application interface has a new method. Go applications that embed BaseApplication compile unchanged, but get an error if Tenderdash ever calls the method, which it will only do from consensus version 2. Applications must implement it before their chain moves to consensus version 2.
• ABCISemVer is 1.5.0. See UPGRADING.md.
• A commit whose beacon or app_hash is set with any size other than 96 and 32 bytes is refused, at every consensus version. No release sets them before consensus version 2.

Prior work

• Refs [DashVM 4.4-dev] Deliver the native randomness primitive as a separate project platform#4682 (Platform randomness). This design reveals the value when its own block commits, where #4682 asked for a later lookup.
• feat(platform)!: sys.random.uniform document properties platform#5749 builds the random document properties on the Platform side.
• A stacked follow-up PR recovers the beacon from prevotes and makes precommits sign the app hash returned by this method, at consensus version 2.

Attribution

Prepared with Claude Code.

🤖 Generated with Claude Code

PR Hygiene · cd6b4ec

• Bots — thepastaclaw ✓, 1 thread unresolved — resolve it
• Self-review — post /self-reviewed
• Build green
• Approvals
  • files with no dedicated owner (UPGRADING.md, abci/client/grpc_client.go, abci/client/mocks/client.go and 41 more) — lklimek or shumkov

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: dashpay/tenderdash
• Branch: feat/abci-apply-block-randomness
• File: .coderabbit.yaml
• Commit: cd6b4ec
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1010a6be-cc1b-446c-9473-36bae353c60a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[thepastaclaw]

✅ Final review complete — no blockers (commit cd6b4ec) · triage: critical

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The compatibility decoder still rejects crafted block encodings accepted by the base revision, creating a rolling-upgrade consensus acceptance mismatch; proposal-side CommitInfo conversion also omits the new fields. Static verification at c0e8c7f confirms one blocking issue and one non-blocking suggestion; no builds or tests were run. Current CI reports successful build, tests, lint, vet, vulnerability and generation checks, with PR Hygiene still pending.

🔴 1 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve the new fields in proposal-side CommitInfo conversion
internal/state/execution.go:778-783

Commit.ToCommitInfo now copies Beacon and AppHash, but buildLastCommitInfo independently constructs the old four-field representation. Both RequestPrepareProposal.LocalLastCommit and RequestProcessProposal.ProposedLastCommit use this helper, so they omit the new fields even when block.LastCommit contains them. This leaves a sibling ABCI conversion incomplete and duplicates a conversion that already has an owner, contrary to STYLE_GUIDE.md's Don't Repeat Yourself rule. It is non-blocking while the feature remains inactive. Keep the existing initial-height guard, delegate to ToCommitInfo, and add regression coverage for both proposal requests with populated commit fields.

	return block.LastCommit.ToCommitInfo()

_{source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general)}

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — This cross-cutting change adds custom peer-facing block deserialization in types/block_decode.go (UnmarshalBlockProto), wires it into proposal processing, and changes commit validation and malformed-field handling in types/block.go, directly affecting block acceptance and upgrade compatibility.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `types/block_decode.go`:
- [BLOCKING] types/block_decode.go:83-85: Match legacy skip semantics in the compatibility decoder
  The fallback still accepts fewer encodings than the base revision's generated decoder. Append `22 02 3b 44` to an otherwise valid serialized block: this adds another LastCommit occurrence containing a field-7 start-group and field-8 end-group. The legacy Block decoder merges the occurrence, and its Commit decoder skips the unknown group because skipTypes tracks depth without matching group numbers. At this head, strict decoding rejects field 7's wire type, then protowire.ConsumeFieldValue rejects the mismatched end-group, so UnmarshalBlockProto fails. A field-7 varint containing nine 0x80 bytes followed by 0x02 exposes the same problem: the legacy skipper accepts its termination, while protowire rejects uint64 overflow. A Byzantine proposer can therefore propose bytes that legacy validators accept but upgraded validators cannot decode; upgrading a node that already stored such a block also makes BlockStore.LoadBlock panic. Make both fallback scanners preserve the legacy generated decoder's acceptance rules, with bounds checks, and add differential regression cases for these encodings.

In `internal/state/execution.go`:
- [SUGGESTION] internal/state/execution.go:778-783: Preserve the new fields in proposal-side CommitInfo conversion
  Commit.ToCommitInfo now copies Beacon and AppHash, but buildLastCommitInfo independently constructs the old four-field representation. Both RequestPrepareProposal.LocalLastCommit and RequestProcessProposal.ProposedLastCommit use this helper, so they omit the new fields even when block.LastCommit contains them. This leaves a sibling ABCI conversion incomplete and duplicates a conversion that already has an owner, contrary to STYLE_GUIDE.md's Don't Repeat Yourself rule. It is non-blocking while the feature remains inactive. Keep the existing initial-height guard, delegate to ToCommitInfo, and add regression coverage for both proposal requests with populated commit fields.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Preserve authenticated original block bytes through block sync — The base revision already loses unknown fields and noncanonical encodings when prepareBlockResponse serves LoadBlock(...).ToProto(). The receiving blockApplier.Apply rebuilds the part set from that representation, and ValidatorSet.commitSignData rejects the resulting BlockID when its PartSetHeader differs from the authentic commit. A Byzantine proposer can therefore get a noncanonical block committed that honest peers cannot subsequently serve successfully through block sync. This is a concrete pre-existing availability issue, independent of the new randomness fields.
  • Follow-up: Track a separate change to retain and verify original authenticated block bytes or parts through block sync, with a regression covering a committed block containing ignored protobuf wire data.

[github-actions]

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Independent static verification of the complete diff at 2c9287d found no actionable in-scope defects, and all three prior findings are addressed. Current CI reports successful build, unit, integration, lint, protobuf, mock, and vulnerability checks; only PR Hygiene remains pending, and no local builds or tests were run. The base-only CBOR dependency update does not supersede this work, and the supplied merge evidence reports no conflicts.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The large, cross-cutting diff adds intricate peer-facing block deserialization in types/block_decode.go (UnmarshalBlockProto and custom protobuf wire parsing) and changes commit validation in types/block.go, directly affecting block acceptance and compatibility.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Static verification of the complete diff found no actionable in-scope defects; all three prior findings are addressed. Current CI shows the build, all six unit-test shards, ABCI tests, lint, generated-artifact checks, and both e2e variants passing; only PR Hygiene remains pending. No local builds or tests were run, and the working tree is unchanged.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The cross-cutting change directly alters peer-facing block deserialization in types/block_decode.go (UnmarshalBlockProto and its custom protobuf wire parser) and commit validation in types/block.go, making backward-compatible block acceptance an intricate critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Static verification at 9e4b04d confirms one blocking omission: proposer-supplied randomness survives canonical commit persistence and is published through commit APIs before activation. The three prior findings are fixed, and the block-sync wire-preservation issue is pre-existing and belongs in a separate follow-up. No local builds or tests were run; current CI reports successful code checks, with PR Hygiene pending.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The cross-cutting change adds intricate peer-facing block deserialization in types/block_decode.go (UnmarshalBlockProto and its wire-parsing helpers), altering acceptance of commit encodings across protocol versions.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `types/block.go`:
- [BLOCKING] types/block.go:1079-1080: Normalize randomness before persisting the canonical last commit
  The new serialization exposes a persistence path that bypasses pre-version-2 normalization. A Byzantine proposer can include an otherwise valid LastCommit with arbitrary 96-byte Beacon and 32-byte AppHash values: BlockFromProto retains correctly sized fields, and LastCommit validation does not authenticate either value. internal/store/store.go:605-607 then serializes block.LastCommit directly into blockCommitKey(height-1). LoadBlockCommit returns those injected fields, /commit publishes them as canonical commit metadata, and statesync.fetchLightBlock serves them to peers. Stripping received standalone commits and proposal-construction inputs does not sanitize a Byzantine proposer's LastCommit. Keep the generic ToProto conversion, but apply WithoutRandomBeacon when persisting the separate canonical commit record, without modifying the block or its committed parts. Add a regression that saves a block containing correctly sized injected fields and verifies that LoadBlockCommit returns empty randomness fields while the original block parts remain unchanged.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Preserve committed block bytes throughout block sync — Block-sync serving decodes stored parts with LoadBlock and reserializes through ToProto; the receiving applier reconstructs parts with MakePartSet. Unknown protobuf fields or noncanonical field ordering accepted during consensus are lost or normalized, producing a different PartSetHeader from the committed one and preventing block sync past that height. Both behaviors already exist at the exact review base, independently of this PR.
  • Follow-up: Address this in a separate follow-up PR by preserving the committed wire representation through block-sync serving and verification, or introducing an upgrade-gated canonical-encoding rule. Include regressions for unknown fields and reordered protobuf fields.

[thepastaclaw]

✅ Resolved at cd6b4ec8; see the replies below.

🔴 Blocking: Normalize randomness before persisting the canonical last commit

The new serialization exposes a persistence path that bypasses pre-version-2 normalization. A Byzantine proposer can include an otherwise valid LastCommit with arbitrary 96-byte Beacon and 32-byte AppHash values: BlockFromProto retains correctly sized fields, and LastCommit validation does not authenticate either value. internal/store/store.go:605-607 then serializes block.LastCommit directly into blockCommitKey(height-1). LoadBlockCommit returns those injected fields, /commit publishes them as canonical commit metadata, and statesync.fetchLightBlock serves them to peers. Stripping received standalone commits and proposal-construction inputs does not sanitize a Byzantine proposer's LastCommit. Keep the generic ToProto conversion, but apply WithoutRandomBeacon when persisting the separate canonical commit record, without modifying the block or its committed parts. Add a regression that saves a block containing correctly sized injected fields and verifies that LoadBlockCommit returns empty randomness fields while the original block parts remain unchanged.

_{source: gpt-6.1-sol (phase2-reviewer: general)}

[thepastaclaw]

Resolved (re-reviewed at cd6b4ec8): Your saveBlockToBatch change strips both fields before writing the previous height's canonical commit, while preserving the original block parts. The new regression verifies canonical reload and confirms that saving does not mutate the supplied block.

[github-actions]

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Static verification of the complete diff at cd6b4ec found no actionable in-scope defects. Three prior findings are fixed; the proposal-side field-preservation request is withdrawn because forwarding unauthenticated randomness before consensus version 2 would violate the intended boundary. Current CI code checks passed, with PastaClaw review and PR Hygiene pending; no local builds or tests were run.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The cross-cutting diff adds intricate peer-facing block deserialization in types/block_decode.go (UnmarshalBlockProto and custom protobuf field parsing) and changes commit validation in types/block.go, directly affecting acceptance compatibility between node versions.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.