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

Generate packet offsets for versioned messages #19138

Merged
merged 3 commits into from
Aug 17, 2021

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Aug 10, 2021

Problem

Sigverify does not support getting the correct packet offsets for the new versioned transactions.

Summary of Changes

Add detection for the message version prefix to properly handle versioned transactions in sigverify

Fixes #

@jstarry jstarry changed the title Add support for generating packet offsets for new versioned message Generate packet offsets for versioned messages Aug 10, 2021
@jstarry jstarry requested review from t-nelson and sakridge August 10, 2021 17:23
@jstarry
Copy link
Member Author

jstarry commented Aug 10, 2021

downstream failure is unrelated to this change, this is ready for review!

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #19138 (12fc4cd) into master (8817f59) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head 12fc4cd differs from pull request most recent head 691cf1a. Consider uploading reports for the commit 691cf1a to get more accurate results

@@            Coverage Diff            @@
##           master   #19138     +/-   ##
=========================================
- Coverage    82.9%    82.8%   -0.1%     
=========================================
  Files         453      453             
  Lines      128700   128723     +23     
=========================================
- Hits       106715   106673     -42     
- Misses      21985    22050     +65     

t-nelson
t-nelson previously approved these changes Aug 13, 2021
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm! It might be nice to adjust this a bit so the inevitable v1 in a smaller change set. I won't block on that though

@jstarry
Copy link
Member Author

jstarry commented Aug 13, 2021

It might be nice to adjust this a bit so the inevitable v1 in a smaller change set. I won't block on that though

Can you elaborate on what you mean here? I intend to have versioning start at 0

@t-nelson
Copy link
Contributor

Can you elaborate on what you mean here? I intend to have versioning start at 0

Sure! My idea was just to set this code up so the addition of future versions is obvious. Basically just a minor rework to a match over the version number and v0's implementation stuck under the corresponding arm

@jstarry
Copy link
Member Author

jstarry commented Aug 13, 2021

Sure! My idea was just to set this code up so the addition of future versions is obvious. Basically just a minor rework to a match over the version number and v0's implementation stuck under the corresponding arm

Ah, got it. Good call, updated.

@mergify mergify bot dismissed t-nelson’s stale review August 13, 2021 04:18

Pull request has been modified.

@jstarry jstarry force-pushed the versioned-sigverify branch from 13bacbb to 691cf1a Compare August 13, 2021 22:13
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

re-re-lgtm!

@jstarry jstarry merged commit fd33f52 into solana-labs:master Aug 17, 2021
@jstarry jstarry deleted the versioned-sigverify branch August 17, 2021 00:55
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