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.

Sign up for GitHub

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

Jump to bottom

chore(core, proto): migrate byte slices from Vec to Bytes #1319

Merged
merged 8 commits into from
Aug 12, 2024
Merged

chore(core, proto): migrate byte slices from Vec to Bytes #1319

merged 8 commits into from
Aug 12, 2024

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Jul 31, 2024 •
edited
Loading

Summary

Changed protobuf, astria-core, and dependent code to utilize generic Bytes instead of the default Vec<u8> to utilize zero-copy deserialization from Protobuf and make type conversion cheaper.

Background

Previously, some components were utilizing Bytes and some were utilizing Vec<u8> as the generated Rust types when converting from Protobuf bytes.

Changes

  • Adjusted Protobuf compiler to use Bytes for all Astria paths.
  • Edited conversions to/from Raw in astria-core to utilize prost::bytes::Bytes.
  • Adjusted code which relied on Vec<u8>.

Testing

Passing all tests.

Related Issues

closes #674

@github-actions github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels Jul 31, 2024
@ethanoroshiba ethanoroshiba changed the title chore(core, proto)!: migrate byte slices from Vec to Bytes chore(core, proto): migrate byte slices from Vec to Bytes Jul 31, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review July 31, 2024 16:18
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner July 31, 2024 16:18
Fraser999
Fraser999 reviewed Aug 1, 2024
View reviewed changes
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Couple of minor suggestions, but I'd like to see if we can replace the handful of remaining Vec<Vec<u8>>s in astria-core and astria-conductor with Vec<Bytes> too.

crates/astria-conductor/src/celestia/reconstruct.rs Outdated Show resolved Hide resolved
crates/astria-core/src/protocol/bridge/v1alpha1/mod.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/block.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/block.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/block.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/block.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/celestia.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/celestia.rs Outdated Show resolved Hide resolved
Fraser999
Fraser999 approved these changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

One call to chunk() has been missed (I unresolved the comment to flag it - it's in celestia.rs). Otherwise LGTM - nice one.

SuperFluffy
SuperFluffy reviewed Aug 8, 2024
View reviewed changes
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

I would like to see a couple of changes before this is merged: a few allocations can be avoided and a few function signatures shouldn't be changed

crates/astria-core/src/primitive/v1/mod.rs Outdated Show resolved Hide resolved
crates/astria-composer/src/executor/tests.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/executor/client.rs Outdated Show resolved Hide resolved
crates/astria-core/src/protocol/bridge/v1alpha1/mod.rs Outdated Show resolved Hide resolved
crates/astria-core/src/protocol/bridge/v1alpha1/mod.rs Outdated Show resolved Hide resolved
crates/astria-core/src/protocol/test_utils.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/block.rs Outdated
@@ -196,7 +199,7 @@ impl SequencerBlockError {
Self(SequencerBlockErrorKind::NoRollupTransactionsRoot)
}

fn incorrect_rollup_transactions_root_length(len: usize) -> Self {
fn incorrect_rollup_tx_root_length(len: usize) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

This should match the name of the error variant. Change back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to avoid a clippy warning of too many lines in SequencerBlock::try_from_block_info_and_data, as the lengthened function name pushes the formatter to break the map_err in which it is called into 3 lines instead of one. Would it be best for me to try to refactor in this case and avoid a clippy exception? Thanks for the detailed feedback :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that seems to a hilarious way to appease the linter with the downside of breaking symmetry with all other methods. :-) The lint hitting that other function is probably correct. But I'm happy with you adding an allow directive on top and address it in a future PR.

crates/astria-core/src/sequencerblock/v1alpha1/celestia.rs Outdated Show resolved Hide resolved
crates/astria-core/src/sequencerblock/v1alpha1/celestia.rs Outdated Show resolved Hide resolved
@ethanoroshiba ethanoroshiba mentioned this pull request Aug 8, 2024
Closed
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Aug 12, 2024
Merged via the queue into main with commit f3ea62e Aug 12, 2024
42 checks passed
@ethanoroshiba ethanoroshiba deleted the ENG-106/migrate_to_bytes branch August 12, 2024 17:54
ethanoroshiba added a commit that referenced this pull request Aug 13, 2024
@ethanoroshiba
chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
00714ab 
## Summary
Changed protobuf, astria-core, and dependent code to utilize generic
`Bytes` instead of the default `Vec<u8>` to utilize zero-copy
deserialization from Protobuf and make type conversion cheaper.

## Background
Previously, some components were utilizing `Bytes` and some were
utilizing `Vec<u8>` as the generated Rust types when converting from
Protobuf `bytes`.

## Changes
- Adjusted Protobuf compiler to use `Bytes` for all Astria paths.
- Edited conversions to/from `Raw` in `astria-core` to utilize
`prost::bytes::Bytes`.
- Adjusted code which relied on `Vec<u8>`.

## Testing
Passing all tests.

## Related Issues
closes #674
ethanoroshiba added a commit that referenced this pull request Aug 14, 2024
@ethanoroshiba
chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
d62a273 
## Summary
Changed protobuf, astria-core, and dependent code to utilize generic
`Bytes` instead of the default `Vec<u8>` to utilize zero-copy
deserialization from Protobuf and make type conversion cheaper.

## Background
Previously, some components were utilizing `Bytes` and some were
utilizing `Vec<u8>` as the generated Rust types when converting from
Protobuf `bytes`.

## Changes
- Adjusted Protobuf compiler to use `Bytes` for all Astria paths.
- Edited conversions to/from `Raw` in `astria-core` to utilize
`prost::bytes::Bytes`.
- Adjusted code which relied on `Vec<u8>`.

## Testing
Passing all tests.

## Related Issues
closes #674
ethanoroshiba added a commit that referenced this pull request Aug 14, 2024
@ethanoroshiba
chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
95eeca9 
## Summary
Changed protobuf, astria-core, and dependent code to utilize generic
`Bytes` instead of the default `Vec<u8>` to utilize zero-copy
deserialization from Protobuf and make type conversion cheaper.

## Background
Previously, some components were utilizing `Bytes` and some were
utilizing `Vec<u8>` as the generated Rust types when converting from
Protobuf `bytes`.

## Changes
- Adjusted Protobuf compiler to use `Bytes` for all Astria paths.
- Edited conversions to/from `Raw` in `astria-core` to utilize
`prost::bytes::Bytes`.
- Adjusted code which relied on `Vec<u8>`.

## Testing
Passing all tests.

## Related Issues
closes #674
@ethanoroshiba ethanoroshiba mentioned this pull request Aug 15, 2024
Merged
noot pushed a commit that referenced this pull request Aug 19, 2024
@ethanoroshiba
refactor(core): shorten try_from_block_info_and_data() (#1371)
a1432ba 
## Summary
Shortened `try_from_block_info_and_data()` to remove clippy exception.

## Background
Migration from `Vec<u8>` to `Bytes` in #1319 introduced some new code
which put `SequencerBlock::try_from_block_info_and_data` over the line
limit.

## Changes
- Created helper function to return `rollup_transactions_root` and
`rollup_ids_root` from a iterator on `data`.

## Testing
Passing all tests

## Related Issues
closes #1357
steezeburger added a commit that referenced this pull request Aug 21, 2024
@steezeburger
Merge branch 'main' into fix/ics20-withdrawals
02acb2e 
* main:
  Chore: Upgrade celestia-node to v0.14.1 (#1360)
  chore(charts): fix charts production templates (#1359)
  chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
steezeburger added a commit that referenced this pull request Aug 22, 2024
@steezeburger
Merge branch 'main' into feature/ibc-e2e-smoke-test
6766220 
* main:
  refactor(core, proto)!: define app genesis state in proto (#1346)
  fix(sequencer): bump penumbra dep to fix ibc state access bug (#1389)
  feat(conductor)!: support disabled celestia auth (#1372)
  fix(sequencer)!: fix block fees (#1343)
  perf(sequencer): add benchmark for prepare_proposal (ENG-660) (#1337)
  fix(proto): fix import name mismatch (#1380)
  fix(ci): enable bridge withdrawer building with tag (#1374)
  feat(sequencer): rewrite memool to have per-account transaction storage and maintenance  (#1323)
  refactor(core, sequencer)!: require that bridge unlock address always be set (#1339)
  fix(sequencer)!: take funds from bridge in ics20 withdrawals (#1344)
  fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332)
  fix: abci error code (#1280)
  refactor(core): shorten `try_from_block_info_and_data()` (#1371)
  fix(relayer): change `reqwest` for `isahc` in relayer blackbox tests (ENG-699) (#1366)
  fix(conductor): update for celestia-node v0.15.0 (#1367)
  Chore: Upgrade celestia-node to v0.14.1 (#1360)
  chore(charts): fix charts production templates (#1359)
  chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperFluffy SuperFluffy SuperFluffy left review comments

@Fraser999 Fraser999 Fraser999 approved these changes

@joroshiba joroshiba Awaiting requested review from joroshiba

@noot noot Awaiting requested review from noot

Assignees
No one assigned
Labels
composer pertaining to composer conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

core: migrate byte slices from Vec to Bytes
3 participants
@ethanoroshiba @Fraser999 @SuperFluffy