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

Add fuzz test for epoch finalization/confirmation #375

Merged
merged 1 commit into from
May 4, 2023

Conversation

KonradStaniec
Copy link
Collaborator

Adds proper fuzz test for the logic which handles epoch finalization/confirmation.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Slightly confused about the logic of this:

@@ -589,3 +592,107 @@ func TestStateTransitionOfValidSubmission(t *testing.T) {
t.Errorf("Epoch Data missing of in unexpected state")
}
}

func FuzzConfirmAndDinalizeManyEpochs(f *testing.F) {
datagen.AddRandomSeedsToFuzzer(f, 20)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
datagen.AddRandomSeedsToFuzzer(f, 20)
datagen.AddRandomSeedsToFuzzer(f, 10)

To follow similar tactic with other tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tbh this was deliberate, as I consider this logic quite a bit important and the tests are pretty fast so there is no downside from going to 20 iterations

Comment on lines +603 to +604
kDeep := defaultParams.BtcConfirmationDepth
wDeep := defaultParams.CheckpointFinalizationTimeout
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
kDeep := defaultParams.BtcConfirmationDepth
wDeep := defaultParams.CheckpointFinalizationTimeout
kDeep := r.Intn(10) + 1
wDeep := kDeep + r.Intn(100) + 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

those parameters needs to be the same as the ones used in btccheckpoint.Keeper created by InitTestKeepers(t). And InitTestKeepers uses default parameters.

if j == 1 {
bestSumbissionInfos[epoch] = uint64(finalizationDepth)
}
finalizationDepth = finalizationDepth - 1
Copy link
Member

Choose a reason for hiding this comment

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

We are executing this statement two times in this if condition, why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because each state (Finalize, Confirmed, Submitted) have different variable to track depth, so to register proper depth for given epoch we need to know what will be to final state of the epoch.

finalizationDepth = finalizationDepth - 1
} else if epoch <= uint64(numFinalizedEpochs+numConfirmedEpochs) {
tk.BTCLightClient.SetDepth(blck1.HeaderBytes.Hash(), int64(confirmationDepth))
confirmationDepth = confirmationDepth - 1
Copy link
Member

Choose a reason for hiding this comment

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

What if confirmationDepth (initially wDeep - 1) starts less than numConfirmedEpochs * 2 (i.e. the number of times this subtraction, along with the next one are going to be executed) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure I understand, but if confirmationDepth would start too low then most probably test would fail, as epoch which we expected to be confirmed would no be.

In general in this test there are three depth intervals:
form math.MaxUint32 to wDeep - those depth will be finalized
from wDepth - 1 to kDeep - those depths will be confirmed
from kDepp - 1 to 0 - those depths will be submitted
And whole data generation is adjusted so that expected blocks, ends in correct intervals

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations!

@KonradStaniec KonradStaniec merged commit 4e2b30a into dev May 4, 2023
@KonradStaniec KonradStaniec deleted the add-fuzz-test-for-epoch-finalization branch May 4, 2023 12:18
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.

2 participants