[solana] Skip last checkpoint in filled checkpoints account

[zpoken]

No description provided.

[zpoken]

The idea of copying the last checkpoint value in a filled checkpoint account to a new checkpoint account implied that the last value in the account would never be used as a valid checkpoint.
That is, if find_checkpoint_le found a checkpoint that is last (in a filled checkpoint account), then we should always assume it is wrong.
The idea was that only the first checkpoint in the next checkpointaccount could be considered a valid checkpoint in this case. If the account does not exist, we consider that there is no checkpoint.
This check is in the program code here:
https://github.com/wormhole-foundation/multigov/blob/main/solana/programs/staking/src/lib.rs#L550

When we go through the checkpoints in the vote_weight window, we have to skip the last checkpoint in the filled account!!!
Since we believe that the last checkpoint of a filled account ALWAYS has a non-valid value. Instead we should consider the first checkpoint of the next account if it is within the window range.

Thus, we have an error in this condition:
https://github.com/wormhole-foundation/multigov/blob/main/solana/programs/staking/src/lib.rs#L565
To fix this we first need to fix the condition in this way:
instead of
&& (checkpoint_index as u32) == config.max_checkpoints_account_limit
should be
&& config.max_checkpoints_account_limit == (checkpoint_index as u32) + 1
Next, we have to check that in case our account is almost full, then we will not try to go to the next account immediately:
https://github.com/wormhole-foundation/multigov/blob/solana/skip-last-checkpoint-in-filled-checkpoints-account/solana/programs/staking/src/lib.rs#L572
And in case the next account is not transferred, there should also be an error:
https://github.com/wormhole-foundation/multigov/blob/solana/skip-last-checkpoint-in-filled-checkpoints-account/solana/programs/staking/src/lib.rs#L584

[zpoken]

I also put the cast_vote tests in a separate test file, because api_test.ts was quite large.

[kosedogus]

When we go through the checkpoints in the vote_weight window, we have to skip the last checkpoint in the filled account!!! Since we believe that the last checkpoint of a filled account ALWAYS has a non-valid value. Instead we should consider the first checkpoint of the next account if it is within the window range.

Considering the last value in a checkpoint account and the first value in the next checkpoint account are always same, it should not be correct to say the last checkpoint of a filled account has a non-valid value. The reason behind the check in https://github.com/wormhole-foundation/multigov/blob/main/solana/programs/staking/src/lib.rs#L550, should be to prevent using a filled checkpoint account from past, so it is only for the case where find_checkpoint_le returns directly the last checkpoint of an account.

PR creates an unnecessary burden for a user when find_checkpoint_le did not returned the last checkpoint, while user has a filled checkpoint account but did not created the next account yet.

Consider this case:

1. Max checkpoint limit is = 5
2. User has a filled checkpoint account that is within voting window range.
3. find_checkpoint_le returns index 3 for the user.
4. User's 3rd index will be read and assigned to total_weight.
5. checkpoint_index will become = 4
6. Because of the change we will enter to the if clause (max checkpoint account limit == checkpoint_index + 1)
7. Since user's checkpoint account is filled, it's next_index will be bigger than checkpoint_index and we don't exit the loop via break
8. Now it will try to load the next account, and will fail because it is not provided.

Currently this part of the program seems to work as expected, it does indeed counts the same value twice (once in last index, once in first index of the next account), but instead of optimizing this, giving the user flexibility to not create a checkpoint when it is not necessary might be a better approach.

Apart from this, I would like to bring attention to last value of the checkpoint account is not an invalid value. Thinking that can lead to problems in future developments.

[zpoken]

Considering the last value in a checkpoint account and the first value in the next checkpoint account are always same,

This is not always true, as the value in the first checkpoint can be overwritten if both the account creation and delegation instructions are executed within the same block. I have added a new test that demonstrates this.
Therefore, we cannot always consider the last value in a filled account to be correct. We should always rely only on the first checkpoint in the new account. That was the original idea.

[zpoken]

The reason behind the check in https://github.com/wormhole-foundation/multigov/blob/main/solana/programs/staking/src/lib.rs#L550, should be to prevent using a filled checkpoint account from past

This is true, but other than that we always require the next account to be created as soon as the previous one is full. In fact, no instructions work for a user account until the next checkpoints account is created.

[kosedogus]

Understood your point, you are mentioning in the case that: in the same timestamp account is filled, a new checkpoint is created and a new delegate happened. Thank you for clarification with test.
In terms of security both versions is safe though I believe, because even if aforementioned actions are performed which will lead to last checkpoint being different than the first checkpoint of the next account, considering their timestamp are same, program will try to reach to next checkpoint account and will fail if not provided (because it didn't reached to vote_start yet, it has to try the next account), if provided that value will be read as well which if it is less than the last one, weight will be updated.
In terms of consistency though seems reasonable to bypass the last one, thank you for clarification.

[mdulin2]

Logic is sane to me but just took some time to reason about.

If the checkpoint limit is 5 and it only has four elements, it will break since it's the end of it. If the checkpoint is full with 5 elements, then the final checkpoint is skipped, requiring the NEXT checkpoint account to be provided.

Tests also seem fairly comprehensive.