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

TransactionView: Message Header Meta #2409

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

apfitzge
Copy link

@apfitzge apfitzge commented Aug 2, 2024

Problem

Summary of Changes

  • Parse a message version and header

Fixes #

@apfitzge apfitzge mentioned this pull request Aug 2, 2024
14 tasks
@apfitzge apfitzge force-pushed the message_header_meta branch from 08c9832 to 925a947 Compare August 2, 2024 15:25
@apfitzge apfitzge marked this pull request as ready for review August 2, 2024 16:39
@apfitzge apfitzge self-assigned this Aug 2, 2024
match version {
// add one byte for prefix byte. Do not need to check for
// overflow here, because we have already checked.
0 => (TransactionVersion::V0, message_offset.wrapping_add(1)),
Copy link

Choose a reason for hiding this comment

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

You could have just done *offset += 1 here, instead of creating a new variable.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I don't think we need to have new variable here. We've already advanced the byte, we actually need to "go back" in the case of legacy instead of v1 since we read the byte here:

        let message_prefix = read_byte(bytes, offset)?;

Copy link
Author

Choose a reason for hiding this comment

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

let me know what you think of this: 2b30a84

Instead of adjusting the offset I just adjusted how we read the number of required signatures.

Legacy txs it will just be the byte we tried reading for the message prefix.

Copy link
Author

Choose a reason for hiding this comment

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

that should actually save us from re-reading the byte; though i'm not sure that would significantly affect peformance.

transaction-view/src/message_header_meta.rs Show resolved Hide resolved
@apfitzge
Copy link
Author

apfitzge commented Aug 5, 2024

Gonna hold off on merging until after #2408 so I can rebase and pick up the bytes::* function renames.

Killing CI since I don't really need it before that rebase.

Edit: wait I am an idiot. This doesn't use those functions 🤦‍♂️ restarting CI

@apfitzge apfitzge merged commit 42422f2 into anza-xyz:master Aug 5, 2024
42 checks passed
@apfitzge apfitzge deleted the message_header_meta branch August 5, 2024 16:57
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
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