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

Use finality_blocks instead of reorg_period in validator #532

Closed
Tracked by #2215
tkporter opened this issue Jun 3, 2022 · 9 comments
Closed
Tracked by #2215

Use finality_blocks instead of reorg_period in validator #532

tkporter opened this issue Jun 3, 2022 · 9 comments
Assignees
Labels
agent good first issue Good for newcomers

Comments

@tkporter
Copy link
Collaborator

tkporter commented Jun 3, 2022

In #504, finality_blocks was added for each ChainConfig. The reorg_period in the Validator agent settings is basically a duplicate of this-- we should remove reorg_period in favor of finality_blocks and update the infra package accordingly

@tkporter tkporter added agent good first issue Good for newcomers labels Jun 3, 2022
@ritikBhandari
Copy link

ritikBhandari commented Jun 3, 2022

Hi,
I'd like to take up this issue if possible.

@tkporter
Copy link
Collaborator Author

tkporter commented Jun 3, 2022

hey @ritikBhandari, amazing please do! I'm happy to answer any questions you may have

@ritikBhandari
Copy link

Sure. Thanks!

@tkporter
Copy link
Collaborator Author

Hey @ritikBhandari, just wondering if you've taken a stab at this?

@nambrot
Copy link
Contributor

nambrot commented Sep 14, 2022

Related is also the confirmations field in ChainConnection which should be part of this merge

@avious00
Copy link
Contributor

avious00 commented Aug 3, 2023

hey @mattiecnvr is this still in review?

@mattiekat
Copy link
Contributor

@avious00
No, and from what I can tell it will need to be re-done if we want to move forward with it.

@mattiekat
Copy link
Contributor

I think this would make sense to do with #2215

@mattiekat
Copy link
Contributor

This ended up changing a bit with the new config shapes and we are now using reorg period but it is the one defined in the common chain metadata.

tkporter added a commit that referenced this issue Oct 24, 2023
…to edge cases (#2836)

### Description

* #532 wasn't
actually fully closed out - finality_blocks was used still for indexing.
Sadly testnet4 infra wasn't configuring finality blocks anymore, so we
weren't indexing only finalized blocks. Moved fully to reorg_period
* Refactored `checkpoint_submitter` in light of races revealed by the
above ^ problem. It used to assume that message indexing and the
`latest_checkpoint()` call would align with one another, and wasn't
resilient to the tree already being past the correctness checkpoint. A
couple situations were possible before that aren't now:
a. Indexing is ahead of the latest_checkpoint() call, which will result
in tree ingesting the new indexed messages and the tree being ahead of
the correctness checkpoint :(
b. It's possible for the tree() call that constructs the tree initially
to be made against a block that's after the next latest_checkpoint()
call, which would result in the tree being ahead of the correctness
checkpoint from the very beginning :(

### Drive-by changes

removed a function that wasn't being used anymore

### Related issues

#532

### Backward compatibility

Removes finality blocks entirely

### Testing

Builds, e2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants