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.

Sign up for GitHub

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

Jump to bottom

Add Proof tx submission enhancement/bug #1574

Closed
wants to merge 5 commits into from
Closed

Add Proof tx submission enhancement/bug #1574

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc1a500
add better proof validation
Jul 17, 2023
6665996
add comments describing changes
Jul 17, 2023
ec2135a
update comment clarity for profo submission
Jul 17, 2023
64750f8
update comments
Jul 19, 2023
85b1623
update comments
Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions x/pocketcore/keeper/proof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,19 @@ func (k Keeper) SendProofTx(ctx sdk.Ctx, n client.Client, node *pc.PocketNode, p
ctx.Logger().Error(fmt.Sprintf("could not delete evidence is not sealed, could cause a relay leak: %s", err.Error()))
}
}
if evidence.NumOfProofs != claim.TotalProofs {

Copy link
Member

Choose a reason for hiding this comment

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

@PoktBlade Have you looked into the difficulty of adding a unit test for this? I realize it's not easy, but just wanted to make sure that we at least put effort into it.

Copy link
Contributor Author

@nodiesBlade nodiesBlade Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I can look into it some more for sanity purposes.

In regards to some other testing, @jorgecuesta did run it live in his fleet, and saw that there weren't any invalid proofs being submitted on chain. This was a dramatic decrease (0 invalid proofs) vs what he saw before the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I wrote some integration tests last night based off TestClaimProtoTx, but it turns out the test for this was already broken.. The reason why we don't see them when running tests, these tests are skipped whenever -s flag is passed.

Anyhow.. will see if I can fix the initial broken tests (seems like a crashing error when submitting proofs, due to AIOB)

Copy link
Member

Choose a reason for hiding this comment

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

@PoktBlade Bump on existing tests and/or new tests.

// In the case of evidence.NumOfProofs < claim.TotalProofs, we don't submit a proof because we cannot generate a valid merkle proof with a smaller number of relays.
nodiesBlade marked this conversation as resolved.
Show resolved Hide resolved
// In the case of evidence.NumOfProofs > claim.TotalProofs, we truncate proofs of the evidence (that happens inside GenerateMerkleProof) and submit a proof.
if evidence.NumOfProofs < claim.TotalProofs {
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
err := pc.DeleteEvidence(claim.SessionHeader, claim.EvidenceType, node.EvidenceStore)
ctx.Logger().Error(fmt.Sprintf("evidence num of proofs does not equal claim total proofs... possible relay leak"))
ctx.Logger().Error(fmt.Sprintf("evidence num of proofs is less than submitted claim's total proofs, can't generate merkle proof"))
if err != nil {
ctx.Logger().Error(fmt.Sprintf("evidence num of proofs does not equal claim total proofs... possible relay leak: %s", err.Error()))
ctx.Logger().Error(fmt.Sprintf("evidence num of proofs is less than submitted claim's total proofs, can't generate merkle proof: %s", err.Error()))
}
// If this is the case, delete the evidence and continue iterating through the claims to submit.
nodiesBlade marked this conversation as resolved.
Show resolved Hide resolved
continue
}

// get the session context
sessionCtx, err := ctx.PrevCtx(claim.SessionHeader.SessionBlockHeight)
if err != nil {
Expand All @@ -73,8 +79,24 @@ func (k Keeper) SendProofTx(ctx sdk.Ctx, n client.Client, node *pc.PocketNode, p
if !found {
ctx.Logger().Error(fmt.Sprintf("an error occurred creating the proof transaction with app %s not found with evidence %v", evidence.ApplicationPubKey, evidence))
}

// There is a potential race condition where the evidence is not sealed while submitting a claim
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any idea why this wasn't a major concern in the past?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally whenever the claim is sealed, relays stop coming in and so there is some non deterministic timing in which the race condition could happen. If relays keep coming in (i.e session rollover), the problem becomes more prominent.

We did find some on chain tx's to show that this did periodically happen even without session rollover though.

Copy link
Contributor Author

@nodiesBlade nodiesBlade Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

image
You can go here and filter the transactions by "Proof" and "Failed", and put the start date before ~ the July 10th and it will show some of the failed submissions even with session rollover disabled.

Copy link
Member

Choose a reason for hiding this comment

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

Was running into some issues but reached out to poktscan here: https://discord.com/channels/854406364931686400/950799595171086377/1131367539667107872

// Allowing for more relays to enter the evidence claim, and whenever a proof is generated, it will
// result in an incorrect merkle proof from being generated.
nodiesBlade marked this conversation as resolved.
Show resolved Hide resolved
// In order to allow for best-effort process to submit a proof for the claim the service submitted
nodiesBlade marked this conversation as resolved.
Show resolved Hide resolved
// 1. Generate a merkle proof for claim's total proof if proof count doesn't exceed max relays per session
// 2. Otherwise, fall back to max relays per session
relaysCompleted := claim.TotalProofs

maxSessionRelays := pc.MaxPossibleRelays(app, k.SessionNodeCount(sessionCtx)).Int64()

// If the relays completed is more than what protocol will allow, set it to the max.
if relaysCompleted > maxSessionRelays {
relaysCompleted = maxSessionRelays
}

// get the merkle proof object for the pseudorandom index
mProof, leaf := evidence.GenerateMerkleProof(claim.SessionHeader.SessionBlockHeight, int(index), pc.MaxPossibleRelays(app, k.SessionNodeCount(sessionCtx)).Int64())
mProof, leaf := evidence.GenerateMerkleProof(claim.SessionHeader.SessionBlockHeight, int(index), relaysCompleted)
// if prevalidation on, then pre-validate
if pc.GlobalPocketConfig.ProofPrevalidation {
// validate level count on claim by total relays
Expand Down
Loading