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

added latest snapshot height querying #970

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Conversation

brennanjl
Copy link
Collaborator

As discussed earlier, we hit a sync issue with TSN testnet due to Comet trying to verify too many block headers. Since our snapshot providers are trusted anyways, this isn't necessary. I added the ability to query ABCI for the latest snapshot from a node, so that we can avoid this.

This is untested, but I just wanted to get something for it because I need to get going and I didn't want to lose my train of thought on it. There is a TODO in there asking about why we don't return errors in a certain part of ABCI (CC @charithabandi you might be able to answer this for me)

@jchappelow
Copy link
Member

@charithabandi for background: cometbft/cometbft#742
The gap between trusted height and the latest available snapshot height determines how many block headers get pulled for validation. I'm pretty certain trusted height (like a checkpoint height in btc) is expected to be below the available snapshot heights. What we found is that if the trusted height was the current block (higher than latest snapshot height), for some reason it would pull and ungodly amount of data when verifying headers.

@@ -1595,7 +1595,9 @@ func (a *AbciApp) ProcessProposal(ctx context.Context, req *abciTypes.RequestPro
func (a *AbciApp) Query(ctx context.Context, req *abciTypes.RequestQuery) (*abciTypes.ResponseQuery, error) {
a.log.Debug("ABCI Query", zap.String("path", req.Path), zap.String("data", string(req.Data)))

if req.Path == statesync.ABCISnapshotQueryPath { // "/snapshot/height"
switch {
// TODO: why do we not return errors in case of the snapshotter not being configured?
Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, I don't see a major issue with erroring out here. Let's add that.

charithabandi
charithabandi previously approved these changes Sep 11, 2024
Copy link
Contributor

@charithabandi charithabandi left a comment

Choose a reason for hiding this comment

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

LGTM!

@brennanjl
Copy link
Collaborator Author

I will add in errors

@brennanjl
Copy link
Collaborator Author

Ok I added errors to abci query @charithabandi. I tested them out and it works as expected (it just returns the error to the client)

@charithabandi charithabandi merged commit d155a3f into main Sep 11, 2024
2 checks passed
@charithabandi charithabandi deleted the query-latest-snapshot branch September 11, 2024 18:00
brennanjl added a commit that referenced this pull request Sep 13, 2024
* added latest snapshot height querying

* added errors to abci query
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.

3 participants