Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(state-transition): Skip double payload verification on verified and finalized blocks #2145

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

perf(state-transition): Skip double payload verification on verified and finalized blocks #2145

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
33f42fe
let consensus control payload verification
abi87 Nov 11, 2024
aed28ab
switched off payload verify upon block finalization
abi87 Nov 11, 2024
1e120b3
skipped execution payload timestamp validator on Bartio
abi87 Nov 12, 2024
5d2cd2c
add logging to state transitions
abi87 Nov 12, 2024
675abd4
logged consensus height along with payload height
abi87 Nov 12, 2024
d32eae0
wrong test sign
abi87 Nov 12, 2024
73b7780
nits
abi87 Nov 12, 2024
94e55ee
nit typo
abi87 Nov 12, 2024
103be19
Merge branch 'feature-flag-timestamp-verification' into consensus-con…
abi87 Nov 12, 2024
66a8ca1
Merge branch 'main' into consensus-controls-payload-validation
abi87 Nov 15, 2024
30d5272
Merge remote-tracking branch 'origin/main' into consensus-controls-pa…
abi87 Nov 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 3 additions & 17 deletions mod/beacon/blockchain/process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,23 +113,9 @@ func (s *Service[
// bad peer, and we would likely AppHash anyways.
OptimisticEngine: true,

// When we are NOT synced to the tip, process proposal
// does NOT get called and thus we must ensure that
// NewPayload is called to get the execution
// client the payload.
//
// When we are synced to the tip, we can skip the
// NewPayload call since we already gave our execution client
// the payload in process proposal.
//
// In both cases the payload was already accepted by a majority
// of validators in their process proposal call and thus
// the "verification aspect" of this NewPayload call is
// actually irrelevant at this point.
SkipPayloadVerification: false,

ProposerAddress: blk.GetProposerAddress(),
NextPayloadTimestamp: blk.GetNextPayloadTimestamp(),
SkipPayloadVerification: true, // TODO: tmp debuggin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Remove hardcoded skip of payload verification

Hardcoding SkipPayloadVerification to true with a TODO comment is concerning:

  1. It unconditionally skips verification for all blocks
  2. It contradicts the PR's objective of letting consensus control verification
  3. The comment suggests this is temporary debugging code that shouldn't be merged

Replace the hardcoded value with the intended implementation:

-           SkipPayloadVerification: true, // TODO: tmp debuggin
+           SkipPayloadVerification: !blk.GetVerifyPayload(),

This change:

  • Aligns with PR objectives by letting consensus control verification
  • Uses the new GetVerifyPayload method as intended
  • Maintains security by only skipping verification during replay
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
SkipPayloadVerification: true, // TODO: tmp debuggin
SkipPayloadVerification: !blk.GetVerifyPayload(),

ProposerAddress: blk.GetProposerAddress(),
NextPayloadTimestamp: blk.GetNextPayloadTimestamp(),
},
st,
blk.GetBeaconBlock(),
Expand Down
5 changes: 5 additions & 0 deletions mod/beacon/blockchain/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ type ConsensusBlock[BeaconBlockT any] interface {
// consensus for the next payload to be proposed. It is also
// used to bound current payload upon validation
GetNextPayloadTimestamp() math.U64

// GetVerifyPayload signals whether execution payload should be
// verified or not. We should always verify it except when finalizing
// replayed blocks.
GetVerifyPayload() bool
}

// BeaconBlock represents a beacon block interface.
Expand Down
6 changes: 6 additions & 0 deletions mod/consensus/pkg/cometbft/service/middleware/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ func (h *ABCIMiddleware[
blk,
req.GetProposerAddress(),
req.GetTime().Add(h.minPayloadDelay),
true, // verify payload
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider making payload verification configurable

Instead of hardcoding true, consider making this value configurable through middleware initialization or configuration. This would provide more flexibility for different scenarios and make the code more maintainable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add documentation for the payload verification parameter

The addition of true for payload verification is correct for the proposal processing flow. However, consider adding a comment explaining why payload verification is always enabled during proposal processing.

-		true, // verify payload
+		true, // Always verify payload during proposal processing to ensure block validity
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
true, // verify payload
true, // Always verify payload during proposal processing to ensure block validity

)
blkEvent := async.NewEvent(ctx, async.BeaconBlockReceived, consensusBlk)
if err = h.dispatcher.Publish(blkEvent); err != nil {
Expand Down Expand Up @@ -346,6 +347,11 @@ func (h *ABCIMiddleware[
blk,
req.GetProposerAddress(),
req.GetTime().Add(h.minPayloadDelay),

// Finalize may be called while syncing. In such a case
// we can skip payload verification since block is guaranteed
// to have been accepted by a super majority of validators.
req.SyncingToHeight == req.Height,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Enhance error handling for payload verification control

The current implementation would benefit from explicit error handling when payload verification is skipped or fails. Consider:

  1. Adding specific error types for verification skipped vs failed
  2. Including verification status in metrics
  3. Adding structured logging for verification decisions

This would improve observability and debugging capabilities.

Also applies to: 224-224

🧹 Nitpick (assertive)

Consider additional safety checks for sync detection

While the current condition req.SyncingToHeight == req.Height works for basic sync detection, consider adding additional safety checks:

  1. Ensure both heights are non-zero
  2. Consider adding a minimum height difference threshold
  3. Add logging when skipping verification during sync

This would make the sync detection more robust and aid in debugging.

Example enhancement:

-       req.SyncingToHeight == req.Height,
+       func() bool {
+           isSyncing := req.SyncingToHeight > 0 && req.Height > 0 && 
+               req.SyncingToHeight == req.Height
+           if isSyncing {
+               h.logger.Debug("Skipping payload verification during sync",
+                   "height", req.Height,
+                   "syncingToHeight", req.SyncingToHeight)
+           }
+           return isSyncing
+       }(),

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM! Consider extracting the sync check to a named variable

The logic for skipping payload verification during syncing is correct and well-documented. However, consider extracting the comparison to a named variable for better readability.

+		isSyncing := req.SyncingToHeight == req.Height
 		// Finalize may be called while syncing. In such a case
 		// we can skip payload verification since block is guaranteed
 		// to have been accepted by a super majority of validators.
-		req.SyncingToHeight == req.Height,
+		isSyncing,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Finalize may be called while syncing. In such a case
// we can skip payload verification since block is guaranteed
// to have been accepted by a super majority of validators.
req.SyncingToHeight == req.Height,
isSyncing := req.SyncingToHeight == req.Height
// Finalize may be called while syncing. In such a case
// we can skip payload verification since block is guaranteed
// to have been accepted by a super majority of validators.
isSyncing,

)
blkEvent := async.NewEvent(
ctx,
Expand Down
8 changes: 8 additions & 0 deletions mod/consensus/pkg/types/consensus_block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,32 @@ type ConsensusBlock[BeaconBlockT any] struct {

// some consensus data useful to build and verify the block
*commonConsensusData

verifyPayload bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add documentation for the verifyPayload field

Consider adding a comment explaining the purpose of this field and its relationship to payload verification during block replay.

 	*commonConsensusData

+	// verifyPayload determines whether payload verification should be performed
+	// during block processing. This is typically false during block replay.
 	verifyPayload bool
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
verifyPayload bool
*commonConsensusData
// verifyPayload determines whether payload verification should be performed
// during block processing. This is typically false during block replay.
verifyPayload bool

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add documentation for the verifyPayload field

Consider adding a comment explaining the purpose and usage of this field, particularly its role in controlling payload verification during block processing.

 	*commonConsensusData

+	// verifyPayload determines whether the payload should be verified during block processing.
+	// When false, payload verification is skipped (e.g., during block replay).
 	verifyPayload bool
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
verifyPayload bool
*commonConsensusData
// verifyPayload determines whether the payload should be verified during block processing.
// When false, payload verification is skipped (e.g., during block replay).
verifyPayload bool

}

// New creates a new ConsensusBlock instance.
func (b *ConsensusBlock[BeaconBlockT]) New(
beaconBlock BeaconBlockT,
proposerAddress []byte,
nextPayloadTimestamp time.Time,
verifyPayload bool,
) *ConsensusBlock[BeaconBlockT] {
b = &ConsensusBlock[BeaconBlockT]{
blk: beaconBlock,
commonConsensusData: &commonConsensusData{
proposerAddress: proposerAddress,
nextPayloadTimestamp: math.U64(nextPayloadTimestamp.Unix()),
},
verifyPayload: verifyPayload,
}
return b
}

func (b *ConsensusBlock[BeaconBlockT]) GetBeaconBlock() BeaconBlockT {
return b.blk
}

func (b *ConsensusBlock[BeaconBlockT]) GetVerifyPayload() bool {
return b.verifyPayload
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add documentation for the GetVerifyPayload method

Consider adding a comment explaining the method's purpose and return value semantics.

+// GetVerifyPayload returns whether payload verification should be performed during
+// block processing. A return value of false typically indicates that the block
+// is being replayed.
 func (b *ConsensusBlock[BeaconBlockT]) GetVerifyPayload() bool {
 	return b.verifyPayload
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (b *ConsensusBlock[BeaconBlockT]) GetVerifyPayload() bool {
return b.verifyPayload
}
// GetVerifyPayload returns whether payload verification should be performed during
// block processing. A return value of false typically indicates that the block
// is being replayed.
func (b *ConsensusBlock[BeaconBlockT]) GetVerifyPayload() bool {
return b.verifyPayload
}

5 changes: 5 additions & 0 deletions mod/node-core/pkg/components/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ type (
// consensus for the next payload to be proposed. It is also
// used to bound current payload upon validation
GetNextPayloadTimestamp() math.U64

// GetVerifyPayload signals whether execution payload should be
// verified or not. We should always verify it except when finalizing
// replayed blocks.
GetVerifyPayload() bool
}

// BeaconBlock represents a generic interface for a beacon block.
Expand Down
Loading