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: use the right key for EarliestAvailableAttestationNonce check method #1902

Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Jun 10, 2023

Overview

We were using the wrong key when checking if the EarliestAvailableAttestationNonce exists in store

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@rach-id rach-id added the x/qgb label Jun 10, 2023
@rach-id rach-id self-assigned this Jun 10, 2023
@MSevey MSevey requested review from a team and mojtaba-esk and removed request for a team June 10, 2023 12:03
@codecov-commenter
Copy link

Codecov Report

Merging #1902 (2359b17) into main (bb00764) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1902   +/-   ##
=======================================
  Coverage   21.85%   21.85%           
=======================================
  Files         117      117           
  Lines       13304    13304           
=======================================
  Hits         2907     2907           
  Misses      10107    10107           
  Partials      290      290           
Impacted Files Coverage Δ
x/qgb/keeper/keeper_attestation.go 66.66% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Nice find 👍 . I assume we will need to backport this to v1.x right? Do we have tests for this btw?

@rach-id rach-id mentioned this pull request Jun 12, 2023
@rach-id
Copy link
Member Author

rach-id commented Jun 12, 2023

Yes will need to be backported to v1.x. I will open a PR once this one gets merged.

For the test, it needs to be done for other check methods as well before genesis block state is applied. I opened an issue to work on it separately #1908

@rach-id rach-id merged commit 5c2c640 into celestiaorg:main Jun 13, 2023
15 of 16 checks passed
@evan-forbes
Copy link
Member

is there anyway this could be consensus breaking?

@rach-id
Copy link
Member Author

rach-id commented Jun 13, 2023

yes, it is consensus breaking, but I believe everything is setup correctly, only the check was missing. So, it shouldn't halt the network

@rach-id rach-id added the consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules label Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants