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

Associate block with metadata and fix transaction encoding #2045

Merged
merged 14 commits into from
Nov 16, 2023

Conversation

shenkeyao
Copy link
Member

@shenkeyao shenkeyao commented Nov 10, 2023

  • Updates the BlockPayload trait.
    • Add a Metadata type and a constructor to build the payload and metadata.
    • Add transaction encoding and decoding functions.
  • Updates the BlockHeader constructor to take metadata as an input.
  • Implements the new BlockPayload functions for the VIDBlockPayload struct.
  • Fixes transaction flattening to include the length of each transaction.

Closes #2024. Closes #2025.

crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/block_impl.rs Outdated Show resolved Hide resolved
crates/task-impls/src/consensus.rs Outdated Show resolved Hide resolved
crates/task-impls/src/consensus.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

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

Some quick comments. Perhaps I'll post more later when I have time. We'll definitely need feedback from @jbearer here.

crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Show resolved Hide resolved
crates/types/src/block_impl.rs Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
@shenkeyao
Copy link
Member Author

shenkeyao commented Nov 10, 2023

All the task implementations (for consensus, transactions, etc.) are still using VIDBlockPayload which used to be the only payload struct. I should have replaced all of them in the production code with the app-specified payload type.

Will update this. Thanks for the comments! @jbearer @ggutoski

Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

I think the overall shape of things is right now. All of my comments are just details

crates/task-impls/src/da.rs Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/types/src/block_impl.rs Outdated Show resolved Hide resolved
crates/types/src/block_impl.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
jbearer
jbearer previously approved these changes Nov 15, 2023
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

I think it looks good, except see this thread about possibly removing the commit() method of BlockPayload and having HotShot keep track of the commit on its own. WDYT about that @shenkeyao @ggutoski

@shenkeyao
Copy link
Member Author

I think it looks good, except see this thread about possibly removing the commit() method of BlockPayload and having HotShot keep track of the commit on its own. WDYT about that @shenkeyao @ggutoski

@jbearer As mentioned in that thread, I think it makes sense to remove the Commitable bound. I've made an issue and will do that after merging this PR.

@shenkeyao shenkeyao merged commit f45acbc into develop Nov 16, 2023
4 of 6 checks passed
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.

Fix transaction flattening Associate block header and payload with metadata type
3 participants