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

Remove manifest versions #472

Merged
merged 3 commits into from
Jul 13, 2024
Merged

Remove manifest versions #472

merged 3 commits into from
Jul 13, 2024

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jul 13, 2024

This also allows us to re-config without re-bootstrap (by restarting the local F3 instance). This is only safe for non-consensus parameters and will be horribly unsafe until we fix 392.

Also:

  • Removes reliance on hashing json, relies on the network name and manual equality checks.
  • Removes versions. We now expect the version to be explicitly specified in the network name.
  • Starts message sequence numbers at the current time so we don't need to save them.
  • Remove the EC from the dynamic manifest provider as it's unused.
  • Tests.

fixes #468

@Stebalien Stebalien force-pushed the steb/remove-manifest-versions branch from d21b9bd to e74d3b2 Compare July 13, 2024 14:32
Copy link

codecov bot commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.56%. Comparing base (7108cc5) to head (f23b8ae).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #472      +/-   ##
==========================================
+ Coverage   81.16%   81.56%   +0.39%     
==========================================
  Files          41       41              
  Lines        3652     3666      +14     
==========================================
+ Hits         2964     2990      +26     
+ Misses        408      396      -12     
  Partials      280      280              
Files Coverage Δ
manifest/dynamic_manifest.go 71.42% <100.00%> (-2.05%) ⬇️
manifest/manifest_sender.go 84.74% <100.00%> (+4.74%) ⬆️
manifest/manifest.go 88.23% <95.00%> (+30.34%) ⬆️
gpbft/powertable.go 84.49% <55.55%> (-2.18%) ⬇️

... and 3 files with indirect coverage changes

@Stebalien Stebalien force-pushed the steb/remove-manifest-versions branch from e74d3b2 to 9ae7b8c Compare July 13, 2024 15:11
@Stebalien Stebalien requested a review from Kubuxu July 13, 2024 15:12
This also allows us to re-config without re-bootstrap (by restarting the
local F3 instance). This is only safe for non-consensus parameters and
will be horribly unsafe until we fix #392.

Also:

- Removes reliance on hashing json, relies on the network name and
  manual equality checks.
- Removes versions. We now expect the version to be explicitly specified
  in the network name.
- Starts message sequence numbers at the current time so we don't need
  to save them.
- Remove the EC from the dynamic manifest provider as it's unused.
- Tests.

fixes #468
@Stebalien Stebalien force-pushed the steb/remove-manifest-versions branch from 9ae7b8c to 9b6dd91 Compare July 13, 2024 15:13
cmd/f3/manifest.go Outdated Show resolved Hide resolved
@Stebalien Stebalien force-pushed the steb/remove-manifest-versions branch from 2232040 to f23b8ae Compare July 13, 2024 15:38
@Stebalien Stebalien added this pull request to the merge queue Jul 13, 2024
Merged via the queue into main with commit 4aba72e Jul 13, 2024
13 checks passed
@Stebalien Stebalien deleted the steb/remove-manifest-versions branch July 13, 2024 15:58
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.

Manifest server does not handle sequence numbers and versions
2 participants