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

fork_db check prevents voting on blocks including a qc claim on a block not in fork_db. #778

Closed
greg7mdp opened this issue Sep 13, 2024 · 0 comments · Fixed by #788 or #867
Closed
Assignees
Labels
bug The product is not working as was intended. 👍 lgtm
Milestone

Comments

@greg7mdp
Copy link
Contributor

greg7mdp commented Sep 13, 2024

While trying to reproduce the use case of issue #751:

/* -----------------------------------------------------------------------------------------------------
            Finality advancing past block claimed on alternate branch
            =========================================================

Time:        t1      t2      t3      t4      t5      t6      t7      t8
Blocks:
    B0 <---  B1 <--- B2 <--- B3 <-|- B4 <--- B5
                                  |
                                  \----------------- B6 <--- B7 <--- B8 <--- B9
QC claim:
           Strong          Strong  Strong  Strong  Strong   Weak   Strong  Strong
             B0              B1      B3      B4      B2      B6      B7      B8

Vote:                      Strong  Strong   Strong  Weak   Strong  Strong  Strong

                                                     ^
                                                     |
                                                 validating those weak votes on b2
                                                 would fail if nodes have received b4 and b5
                                                 which advanced lib to b3

    - Node D is isolated and has not seen B3, B4, and B5
    - it received B3 via push_block, (so it can make it its head and produce a child of B3), but has not
      received votes on b3 (only on b2), so b6 includes a strong QC on b2.
    - when b6 is pushed to A, B and C, qc validation should fail.
--------------------------------------------------------------------------------------------------------*/

I was unable to obtain a QC on B6 (which itself has a QC claim on B2).

This is because finalizers who have voted on B5 (top fork), have advanced lib to B3, and therefore B2 is not in their fork_db anymore.

Unfortunately, we have the following check (also here), which prevents these finalizers from voting.

This check was intended to find an ancestor block of the current block, with block_num >= latest_qc_claim__block_ref.block_num(), that is validated. However the check is incorrect and only does a lookup for latest_qc_claim__block_ref.block_id, which fails in this case.

We should instead walk back from the current block to either the root or latest_qc_claim__block_ref.block_id and break when we find a validated block. If we don't find that latest_qc_claim__block_ref.block_id or one of its descendents has been validated, we don't want to vote on the new block claiming it because it could make a non-validated block final.

@enf-ci-bot enf-ci-bot moved this to Todo in Team Backlog Sep 13, 2024
@greg7mdp greg7mdp self-assigned this Sep 13, 2024
@greg7mdp greg7mdp changed the title Unnecessary fork_db check prevents voting on blocks including a qc claim on a block not in fork_db. fork_db check prevents voting on blocks including a qc claim on a block not in fork_db. Sep 13, 2024
@greg7mdp greg7mdp added bug The product is not working as was intended. and removed bug The product is not working as was intended. labels Sep 13, 2024
@arhag arhag added bug The product is not working as was intended. 👍 lgtm and removed triage labels Sep 16, 2024
@arhag arhag added this to the Spring v1.0.2 milestone Sep 16, 2024
@greg7mdp greg7mdp moved this from Todo to In Progress in Team Backlog Sep 16, 2024
@greg7mdp greg7mdp moved this from In Progress to Awaiting Review in Team Backlog Sep 16, 2024
@github-project-automation github-project-automation bot moved this from Awaiting Review to Done in Team Backlog Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The product is not working as was intended. 👍 lgtm
Projects
Archived in project
3 participants