Skip to content
This repository has been archived by the owner on Apr 15, 2024. It is now read-only.

Sign up to the earliest attestation nonce #522

Closed
rach-id opened this issue Oct 10, 2023 · 5 comments · Fixed by #555
Closed

Sign up to the earliest attestation nonce #522

rach-id opened this issue Oct 10, 2023 · 5 comments · Fixed by #555
Assignees
Labels
limitation orchestrator orchestrator related

Comments

@rach-id
Copy link
Member

rach-id commented Oct 10, 2023

Currently, the orchestrator will sign up to the last unbonding height, and according to the pruning logic:
https://github.com/celestiaorg/celestia-app/blob/0e1fa70cc36f74d1be8a43f38afb0134a02dd1d6/x/qgb/abci.go#L137-L196

We might have a case where the network is stable for more than 3 weeks, that we have the earliestAttestationNonce > lastUnbondingNonce (which is the nonce corresponding to the last unbonding height). This would make the orchestrator try to sign attestations that are already pruned.

Proposal: change the orchestrator implementation to sign up to the earliestAttestationNonce, which would guarantee the attestations to be always in store.

cc @evan-forbes

@rach-id rach-id added orchestrator orchestrator related limitation labels Oct 10, 2023
@rach-id rach-id self-assigned this Oct 10, 2023
@evan-forbes
Copy link
Member

change the orchestrator implementation to sign up to the earliestAttestationNonce, which would guarantee the attestations to be always in store.

If I'm understanding correctly, won't this still run into the same issue? The nice we need to sign will be pruned?

What if we rely on events instead of state?

@rach-id
Copy link
Member Author

rach-id commented Oct 16, 2023

The nice we need to sign will be pruned?

You mean the valset before nonce? If so, then, as it is now, the orchestrator will sign by default if it can't find a valset before that nonce. So, it won't be an issue.

What if we rely on events instead of state?

we rely on events for listening to new attestations, but for old ones, that would mean parsing the previous blocks, etc, I don't see how that would help here

@evan-forbes
Copy link
Member

You mean the valset before nonce? If so, then, as it is now, the orchestrator will sign by default if it can't find a valset before that nonce. So, it won't be an issue.

what is meant by "up to"? does that mean that the orchestrator will start at the latest nonce and then sign backwards until the earliest nonce? if so, then what will signing up to the earliest nonce do? won't the attestation already be pruned still?

we rely on events for listening to new attestations, but for old ones, that would mean parsing the previous blocks, etc, I don't see how that would help here

using events allows us to access attestations that are already pruned

@rach-id
Copy link
Member Author

rach-id commented Oct 17, 2023

what is meant by "up to"? does that mean that the orchestrator will start at the latest nonce and then sign backwards until the earliest nonce? if so, then what will signing up to the earliest nonce do? won't the attestation already be pruned still?

The earliest nonce == the earliest nonce in store. So all the attestation will be in store. However, the valset before the earliest attestation nonce might not be. But, the orchestrator behavior now is signing by default, unless it finds a valset that explicitly states that it shouldn't. So, that would mean the orchestrator will sign up to the earliest nonce successful.

using events allows us to access attestations that are already pruned

We're only putting the nonce in events, not the whole attestations. So, how will we get them?
Regardless of that, I think using events is an overkill. We could just sign up to the earliest nonce and have all the problems disappear. Generally, the valset doesn't change much in cosmos-sdk chains, so orchestrators would be signing regardless.

@rach-id
Copy link
Member Author

rach-id commented Oct 19, 2023

@evan-forbes When you have time please, thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
limitation orchestrator orchestrator related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants