Add Proof tx submission enhancement/bug

[nodiesBlade]

Description

Whenever the evidence does not match the total proofs, the node tries to delete the evidence and submits a claim. It should never submit a proof whenever there is LESS than the number of relay proofs as that can cause an array index of out-bounds exception if the randomly selected index is more than what we have in the store. This can cause a node crash.

Example: Servicer starts off with 10 relays in the evidence store

1. Session ends
2. Servicer submits claim for 10 relays on chain
4. Servicer stops node and restarts evidence.db & evidence.db gets corrupted
5. Servicer will attempt to submit a proof, and psuedo-random selected index for merkle proof generation can result in AIOB

Solution: If the number of relay proofs in the evidence store is less than the submitted claims total relay proofs then delete the evidence and don't submit a proof.

---

There is a potential race condition where the evidence is not sealed while submitting a claim, allowing for more relays to enter the evidence claim. Whenever a proof is generated via GenerateMerkleRoot, it will result in an incorrect Merkle proof from being generated due to a mismatch of how many relays are in the store and how many was submitted on chain. This can inadvertently happen more often as we accept session rollover relays

Example: Servicer starts off with 10 relays in the evidence store

1. Session ends
2. Servicer handles a relay, while also submitting the claim (RACE Condition, no proper locking mechanism here)
3. Servicer submits claim for 10 relays on chain
4. Servicer finishes handling the relay and evidence store now contains 11 relays
5. Servicer (after waiting a couple of blocks) submits a proof tx with a merkle proof for 11 relays, not 10
6. Validators will mark the TX as invalid due to invalid merkle proof.

Solution

• In order to allow for a best-effort process to submit proof for the claim the servicer submitted, we will calculate the maxRelays based off:
  1. Use the claim's total proof if the proof count doesn't exceed the maximum relays per session.
  2. Otherwise, fallback to the maximum relays per session.

The GenerateMerkleRoot already accepts a maxRelays parameter which was introduced to fix the Chocalate Rain/Overservicing vulnerability. It discards relays above maxRelays with the following implementation

    if int64(len(ev.Proofs)) > maxRelays {
        ev.Proofs = ev.Proofs[:maxRelays]
        ev.NumOfProofs = maxRelays
    }

---

Note: The more ideal solution is to only submit the claim whenever we are done servicing for sure, but this involves locks and potential side effects that Pocket core was never really built with in mind

Note: Something tells me that we were missing a continue statement in the old code whenever the evidence does not match the total proofs in the claim. It shouldn't submit if there is a mismatch / if we deleted the evidence. My solution adds a continue for less than, and tries a best effort to submit proof if we do have enough relay proofs in our evidence store.

Summary generated by Reviewpad on 17 Jul 23 19:44 UTC

This pull request adds better proof validation to the proof.go file in the x/pocketcore/keeper package. It includes changes to handle a potential race condition where the evidence is not sealed while submitting a claim, allowing for more relays to enter the evidence claim. It also generates a merkle proof for the claim's total proof count if it doesn't exceed the maximum number of relays per session, otherwise it falls back to the max relays per session. Additionally, it includes validation of the level count on the claim by the total relays.

[nodiesBlade]

Added all the comments as suggested! Thanks for the review.

In regards to the tests, I haven't made any progress to them - don't think I will have the bandwidth moving forward to diagnose / fix the integration tests. The changes here are minimal and only guard rails for proof tx, not consensus. If needed, we can push an additional hot fix without the need for consensus upgrades.

[nodiesBlade]

I'm handing it off as is. The PR is already merged into Poktscan's fork.

[nodiesBlade]

Since I don't have time to dedicate more to this issue, please cherry pick or fork my changes and move forward.

Closing since it won't be merged in / blocked.

[jorgecuesta]

@Olshansk all the request you ask about test are not fair to be handle by the community people trying to help if pocket has people that get a salary for this. This is why then people hesitate to help on the project. I think u guys may need to figure out a way to work on v0 because has a lot of things that are not working and should be working like TESTs and that is pocket responsibility, not the community indeed.

[Olshansk]

@Olshansk all the request you ask about test are not fair to be handle by the community people trying to help if pocket has people that get a salary for this. This is why then people hesitate to help on the project. I think u guys may need to figure out a way to work on v0 because has a lot of things that are not working and should be working like TESTs and that is pocket responsibility, not the community indeed.

Thanks for the feedback.

I will work on adding the tests myself this time and reach out about getting a budget for it in the future.