Stop unfairly penalize peers #15821

nalepae · 2025-10-08T09:16:06Z

What type of PR is this?
Bug fix

What does this PR do? Why is it needed?
Before this PR, Prysm used to return ValidationReject when hitting an internal error in the validator of a message received via gossip. In this case, the corresponding peer is unfairly punished, and possibly disconnected.

This PR moves internal errors from ValidationReject to ValidationIgnore. As a consequence, we ignore the incoming message without punishing the peer.

Other notes for review
Please read commit by commit, with commit messages.

Choosing if the peer has a minimum of responsibility in an "internal error" is not always so straightforward.
Sometimes, there is no doubt. Sometimes, things are more complicated.

Ideally, the reviewer should check all the ValidationReject in the codebase and confirm that Prysm reject/ignore messages when it should.

The following part explains some questionable cases.

`decodePubsubMessage`

The only case where I found it unfair to penalize the peer is when

	m, ok := base.(ssz.Unmarshaler)
	if !ok {
		return nil, errors.Errorf("message of %T does not support marshaller interface", base)
	}

fails.

However, I think reaching this case shows an important bug in our node and the node needs a fix. So I did not write an exception for it.

`kzg.VerifyCellKZGProofBatch`

I initially thought that ckzg4844.VerifyCellKZGProofBatch would return false and no error if at least one proof in the batch is invalid, and an error if the function itself has an issue.
However, giving to this function a sidecar modified by createInvalidTestDataColumns will error the function as well.
(Maybe it's a bug of ckzg4844.VerifyCellKZGProofBatch). As a consequence, we want to reject all messages resulting of an error of ckzg4844.VerifyCellKZGProofBatch.

Seen with @jtraglia : The createInvalidTestDataColumns flips the first byte of the field element in the cell. This will make the field element invalid (not just incorrect) & that’s why it errors.

==> We need to REJECT if ckzg4844.VerifyCellKZGProofBatch errors.

Acknowledgements

I have read CONTRIBUTING.md.
I have included a uniquely named changelog fragment file.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

…Tracef`.

Rationale: The only way `set.Verify` returns an error is if there is a mismatch in `VerifyMultipleSignatures` arguments sizes, which is not the reponsability of the peer. ==> It is unfair to penalize the peer in such a case.

jtraglia · 2025-10-08T12:44:04Z

I initially thought that ckzg4844.VerifyCellKZGProofBatch would return false and no error if at least one proof in the batch is invalid, and an error if the function itself has an issue. However, giving to this function a sidecar modified by createInvalidTestDataColumns will error the function as well. (Maybe it's a bug of ckzg4844.VerifyCellKZGProofBatch). As a consequence, we want to reject all messages resulting of an error of ckzg4844.VerifyCellKZGProofBatch.

VerifyCellKZGProofBatch will return false and no error if an input (eg proof) is incorrect, but it will return false with an error if anything is invalid (malformed). There is no bug here. This is how it was designed.

…th used in blob sidecars and data column sidecars.

…ror.

nalepae · 2025-10-10T14:43:16Z

beacon-chain/verification/blob.go

 	if err != nil {
 		log.WithFields(logging.BlobFields(bv.blob)).WithError(err).Debug("Could not replay parent state for blob signature verification")
-		return ErrInvalidProposerSignature
+		return errors.Wrap(err, "parent state")


Because of this change, TestValidProposerSignature_CacheMiss now fails.
Is it fair to penalize the peer in such a case?

nalepae · 2025-10-10T14:44:22Z

beacon-chain/verification/blob.go

 	parentSlot, err := bv.fc.Slot(bv.blob.ParentRoot())
 	if err != nil {
-		return errors.Wrap(errSlotNotAfterParent, "Parent root not in forkchoice")
+		return errors.Wrap(err, "slot")


Because of this change, TestSidecarParentSlotLower starts to fail.
Is it fair to penalize the peer in such a case?

nalepae · 2025-10-10T14:51:14Z

beacon-chain/verification/blob.go

 		if err != nil {
 			log.WithError(err).WithFields(logging.BlobFields(bv.blob)).Debug("State replay to parent_root failed")
-			return errSidecarUnexpectedProposer
+			return errors.Wrap(err, "parent state")


Because of this change, TestSidecarProposerExpected - "not cached, state lookup failure" starts to fail.
Is it fair to penalize the peer in such a case?

nalepae · 2025-10-10T14:54:27Z

beacon-chain/verification/blob.go

 		if err != nil {
 			log.WithError(err).WithFields(logging.BlobFields(bv.blob)).Debug("Error computing proposer index from parent state")
-			return errSidecarUnexpectedProposer
+			return errors.Wrap(err, "compute proposer")


Because of this change, TestSidecarProposerExpected - "not cached, ComputeProposer fails start to fail.
Is it fair to penalize the peer in such a case?

nalepae added 2 commits October 8, 2025 10:08

validateWithBatchVerifier: Use Trace and WithField instead of `…

f1ea5e6

…Tracef`.

validateWithBatchVerifier: Move a reject into an ignore.

c76ca41

Rationale: The only way `set.Verify` returns an error is if there is a mismatch in `VerifyMultipleSignatures` arguments sizes, which is not the reponsability of the peer. ==> It is unfair to penalize the peer in such a case.

nalepae added the networking label Oct 8, 2025

nalepae force-pushed the stop-penalize-peers-for-nothing branch 3 times, most recently from a52e57e to c449ca0 Compare October 10, 2025 13:00

nalepae changed the title ~~Stop penalize peers for nothing~~ Stop unfairly penalize peers Oct 10, 2025

nalepae added 12 commits October 10, 2025 16:27

IsAggregator: Remove unused error.

d9ac483

validateSelectionIndex: Reject or ignore depending on the error.

5abd3b4

Ignore if NewROBlob fails instead of rejecting.

6c26435

Rename blob specific errors into sidecars errors, because they are bo…

2731720

…th used in blob sidecars and data column sidecars.

Sidecar verification errors: Always join with ErrSidecarInvalid.

f4bb33e

Blob verification: Stop returning verification error when internal er…

15f2f3e

…ror.

validateBlob: Reject only if real validation error, else ignore.

fd4c210

Do not reject on NewRODataColumn failure.

67d512f

Data column sidecars: Do not reject if internal error.

b0e947d

validateProposerSlashing: Do not reject on internal error

e62be1f

rejectInvalidSelectionProof: Do not reject on internal error.

f389a45

Add changelog.

90ef02c

nalepae force-pushed the stop-penalize-peers-for-nothing branch from f921d23 to 90ef02c Compare October 10, 2025 14:27

nalepae commented Oct 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stop unfairly penalize peers #15821

Stop unfairly penalize peers #15821

Uh oh!

nalepae commented Oct 8, 2025 •

edited

Loading

Uh oh!

jtraglia commented Oct 8, 2025

Uh oh!

nalepae Oct 10, 2025

Uh oh!

nalepae Oct 10, 2025

Uh oh!

nalepae Oct 10, 2025

Uh oh!

nalepae Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Stop unfairly penalize peers #15821

Are you sure you want to change the base?

Stop unfairly penalize peers #15821

Uh oh!

Conversation

nalepae commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

decodePubsubMessage

kzg.VerifyCellKZGProofBatch

Uh oh!

jtraglia commented Oct 8, 2025

Uh oh!

nalepae Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

nalepae Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

nalepae Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

nalepae Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nalepae commented Oct 8, 2025 •

edited

Loading

`decodePubsubMessage`

`kzg.VerifyCellKZGProofBatch`