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.

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

Verify chunk signature #1862

Merged
merged 16 commits into from
Jan 15, 2025
Merged

Conversation

tsachiherman
Copy link
Contributor

What ?

This PR add verification of the received chunk signature.

@tsachiherman tsachiherman self-assigned this Jan 10, 2025
@tsachiherman tsachiherman marked this pull request as ready for review January 10, 2025 18:17
x/dsmr/block.go Show resolved Hide resolved
x/dsmr/block.go Outdated
@@ -16,6 +18,8 @@ import (

const InitialChunkSize = 250 * 1024

var ErrFailedSigVerification = errors.New("failed to verify signature")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this more detailed ie. at least include the signature type and the type of container the signature was over like "failed to verify bls chunk signature"

x/dsmr/p2p.go Outdated
Comment on lines 136 to 142
// TODO:
// check if the expiry of this chunk isn't in the past or too far into the future.

// TODO:
// check if the producer was expected to produce this chunk.

// check to see if this chunk was already accepted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can these comments be moved into the verifier?

From p2p perspective, it should only need to know that it must verify the chunk in some context, but doesn't need to know what verification takes place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a TODO for rate limiting chunk production from the given producer?

x/dsmr/p2p.go Outdated
@@ -127,10 +128,18 @@ func (c ChunkSignatureRequestVerifier[T]) Verify(
}
}

// verify that the signature is a valid signature over the unsigned chunk.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we remove obvious comments that match the function name ie. parse and verify unless the comment provides additional information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

x/dsmr/storage.go Outdated Show resolved Hide resolved
x/dsmr/p2p.go Outdated
@@ -119,6 +119,7 @@ func (c ChunkSignatureRequestVerifier[T]) Verify(
_ *warp.UnsignedMessage,
justification []byte,
) *common.AppError {
// parse the chunk
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove obvious comments that don't add new information?

…va-labs/hypersdk into tsachi/configurable-chunk-rate-limiter
@aaronbuchwald aaronbuchwald merged commit d194b71 into main Jan 15, 2025
17 checks passed
@aaronbuchwald aaronbuchwald deleted the tsachi/configurable-chunk-rate-limiter branch January 15, 2025 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants