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

QGB state machine remove invalid validator set check #646

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Aug 22, 2022

closes #638

@rach-id rach-id added bug Something isn't working C: QGB labels Aug 22, 2022
@rach-id rach-id self-assigned this Aug 22, 2022
@rach-id rach-id linked an issue Aug 22, 2022 that may be closed by this pull request
4 tasks
@rach-id
Copy link
Member Author

rach-id commented Aug 22, 2022

Depends on #645

@rach-id rach-id marked this pull request as draft August 22, 2022 07:50
@@ -47,48 +45,11 @@ func (k msgServer) ValsetConfirm(
return nil, sdkerrors.Wrap(types.ErrInvalid, "acc address invalid")
}

// Verify if validator exists
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question][non-blocking] do any checks need to be removed from https://github.com/celestiaorg/celestia-app/blob/qgb-integration/docs/architecture/ADR-002-QGB-ValSet.md?plain=1#L158 ? I don't see these checks in that ADR section so maybe not but the contents of the ADR don't match 1:1 with the ValsetConfirm implementation here so wanted to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the ADRs are pretty oooold now. We will need to update them once we have a stable QGB 1.0. The reason we stopped updating them, is we were trying a lot of approaches, and we changed the design multiple times. Thus, it was too much work to keep updating them always.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. If the ADR is no longer accurate then perhaps we can include a disclaimer at the top stating that it's unmaintained and create a GH issue to update it once we have a stable QGB 1.0? Defer to you on how best to track / update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. We have few issues concerning this:
https://github.com/celestiaorg/celestia-app/issues/217
#501
#270

@rach-id rach-id marked this pull request as ready for review August 22, 2022 16:56
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

:shipit:

@rach-id rach-id merged commit a965914 into celestiaorg:qgb-integration Aug 23, 2022
@rach-id rach-id deleted the remove_invalid_validator_check_from_state_machine branch August 23, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

QGB state machine confirm signed by validator
3 participants