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

Fix narwhal dependency with net updates #1677

Merged
merged 16 commits into from
Apr 30, 2022
Merged

Fix narwhal dependency with net updates #1677

merged 16 commits into from
Apr 30, 2022

Conversation

asonnino
Copy link
Contributor

@asonnino asonnino commented Apr 29, 2022

It seems we had the following problem twice:

  • We make updates on the narwhal repo that break shared object support on Sui. We have no way to check that on the Narwhal side since the narwhal repo does not import the sui repo
  • We make updates on the Sui repo that break shared object but without systematic test failures, the unit tests only become flaky.
  • We eventually disable the flaky unit tests
  • We add new features to narwhal that we want to import on Sui, so we update the narwhal dependency
  • Shared object support is now entirely broken

This PR fixes shared object support (on both the Narwhal and Sui side) while preserving all the new Narwhal features. It also provides tests that (should) systematically breaks when we make a mistake on Sui.

This PR however does not prevent us from making updates on Narwhal that will break shared object support on Sui. @lxfind suggested that I give a tech presentation about what we need from narwhal to support shared objects on Sui, so that we can keep it in mind while updating Narwhal.

@asonnino asonnino marked this pull request as draft April 29, 2022 18:30
@asonnino asonnino self-assigned this Apr 29, 2022
@asonnino asonnino linked an issue Apr 29, 2022 that may be closed by this pull request
@asonnino asonnino added Type: Bug Something isn't working Type: Major Feature Major new functionality or integration, which has a significant impact on the network Priority: High Very important task, not blocking but potentially delaying milestones or limiting our offering Status: In Progress Issue is actively being worked on sui-node labels Apr 29, 2022
@asonnino asonnino added this to the DevNet milestone Apr 29, 2022
@asonnino asonnino marked this pull request as ready for review April 29, 2022 21:22
@asonnino asonnino changed the title WIP Fix narwhal dependency with net updates Apr 29, 2022
@asonnino asonnino enabled auto-merge (squash) April 29, 2022 21:30
@asonnino asonnino added Status: Needs Review PR is ready for review and removed Status: In Progress Issue is actively being worked on labels Apr 29, 2022
@asonnino asonnino requested a review from LefKok April 30, 2022 00:06
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks a lot!

@lxfind suggested that I give a tech presentation about what we need from narwhal to support shared objects on Sui, so that we can keep it in mind while updating Narwhal.

That's an excellent idea!

@asonnino asonnino merged commit 2b57818 into main Apr 30, 2022
@asonnino asonnino deleted the so-fix branch April 30, 2022 02:07
longbowlu pushed a commit that referenced this pull request May 12, 2022
Fix narwhal & shared object support
punwai pushed a commit that referenced this pull request Jul 27, 2022
Fix narwhal & shared object support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Very important task, not blocking but potentially delaying milestones or limiting our offering Status: Needs Review PR is ready for review sui-node Type: Bug Something isn't working Type: Major Feature Major new functionality or integration, which has a significant impact on the network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[consensus] Shared object transaction unit test failure
2 participants