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

fault_proving(compression): include commitment to registry in compressed block header #2571

Closed
wants to merge 1 commit into from
Closed

fault_proving(compression): include commitment to registry in compressed block header #2571

wants to merge 1 commit into from

Conversation

rymnc
Copy link
Member

@rymnc rymnc commented Jan 15, 2025 •
edited
Loading

Linked Issues/PRs

Description

We implement a function id for RegistrationsPerTable which yields a RegistryId that is included within the CompressedBlockHeader

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@rymnc rymnc self-assigned this Jan 15, 2025
@rymnc rymnc force-pushed the proving/compressed-header-changes-p2 branch from 6e6e82d to 2594aa0 Compare January 15, 2025 10:11
@rymnc rymnc linked an issue Jan 15, 2025 that may be closed by this pull request
fault_proving(compression): add hash of registry to compressed block header #2568
Open
Base automatically changed from proving/compressed-header-changes to master January 15, 2025 11:28
@rymnc rymnc force-pushed the proving/compressed-header-changes-p2 branch 3 times, most recently from b41ff8a to aceba71 Compare January 15, 2025 11:38
rymnc
rymnc commented Jan 15, 2025
View reviewed changes
crates/compression/src/registry.rs
}
}};
}

// Don't change the order of these, it will affect the order of hashing
Copy link
Member Author

Choose a reason for hiding this comment

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

created: FuelLabs/fuel-specs#623

@rymnc rymnc marked this pull request as ready for review January 15, 2025 11:40
@rymnc rymnc force-pushed the proving/compressed-header-changes-p2 branch from aceba71 to 4d998ae Compare January 15, 2025 12:46
@rymnc rymnc mentioned this pull request Jan 15, 2025
Open
9 tasks
@rymnc rymnc force-pushed the proving/compressed-header-changes-p2 branch from 4d998ae to 4ca0445 Compare January 15, 2025 15:13
netrome
netrome requested changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Are you sure RegistrationsPerTable contains the entire registry? I was under the impression that this is only the new registrations introduced by the block, and we need to commit to the entire registry here.

In either case, it would be great if we could add a test which compresses a few subsequent blocks and verifies the compression registry ID.

@rymnc
Copy link
Member Author

rymnc commented Jan 16, 2025

Are you sure RegistrationsPerTable contains the entire registry? I was under the impression that this is only the new registrations introduced by the block, and we need to commit to the entire registry here.

In either case, it would be great if we could add a test which compresses a few subsequent blocks and verifies the compression registry ID.

why do we have to commit to the whole registry?

@Dentosal
Copy link
Member

Are you sure RegistrationsPerTable contains the entire registry? I was under the impression that this is only the new registrations introduced by the block, and we need to commit to the entire registry here.

Your impression is correct

@rymnc
Copy link
Member Author

rymnc commented Jan 22, 2025

needs to be reworked. closing for now

@rymnc rymnc closed this Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@netrome netrome netrome requested changes

@xgreenx xgreenx Awaiting requested review from xgreenx xgreenx is a code owner

@Dentosal Dentosal Awaiting requested review from Dentosal Dentosal is a code owner

@MitchTurner MitchTurner Awaiting requested review from MitchTurner MitchTurner is a code owner

Assignees

@rymnc rymnc

Labels
fault-proving
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fault_proving(compression): add hash of registry to compressed block header
3 participants
@rymnc @Dentosal @netrome