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

StateProofs: Use stateproof tracker #4733

Merged

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented Nov 2, 2022

Summary

Test Plan

@id-ms id-ms changed the title Use stateproof tracker StateProofs: Use stateproof tracker Nov 2, 2022
@id-ms id-ms self-assigned this Nov 2, 2022
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #4733 (03af600) into feature/stateproofs-recoverability (e569879) will decrease coverage by 0.00%.
The diff coverage is 30.50%.

@@                          Coverage Diff                           @@
##           feature/stateproofs-recoverability    #4733      +/-   ##
======================================================================
- Coverage                               54.69%   54.68%   -0.01%     
======================================================================
  Files                                     415      415              
  Lines                                   53783    53805      +22     
======================================================================
+ Hits                                    29415    29424       +9     
- Misses                                  21916    21933      +17     
+ Partials                                 2452     2448       -4     
Impacted Files Coverage Δ
cmd/tealdbg/localLedger.go 64.41% <0.00%> (-0.40%) ⬇️
ledger/acctupdates.go 69.22% <0.00%> (-0.09%) ⬇️
ledger/apply/apply.go 0.00% <ø> (ø)
ledger/apply/stateproof.go 0.00% <0.00%> (ø)
ledger/evalindexer.go 49.38% <0.00%> (-0.62%) ⬇️
ledger/internal/eval.go 48.93% <0.00%> (-0.07%) ⬇️
daemon/algod/api/server/v2/dryrun.go 71.29% <50.00%> (-0.22%) ⬇️
stateproof/verify/stateproof.go 66.66% <60.86%> (-9.05%) ⬇️
config/consensus.go 87.56% <100.00%> (+0.03%) ⬆️
ledger/internal/cow.go 65.21% <100.00%> (+0.30%) ⬆️
... and 14 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -382,7 +382,7 @@ func TestStateProofMessageCommitmentVerification(t *testing.T) {
}

func getDefaultStateProofConsensusParams() config.ConsensusParams {
consensusParams := config.Consensus[protocol.ConsensusCurrentVersion]
consensusParams := config.Consensus[protocol.ConsensusFuture]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remember to change this after the consensus upgrade. Maybe add a ticket?

@id-ms id-ms marked this pull request as ready for review November 3, 2022 12:48
Copy link
Contributor

@almog-t almog-t left a comment

Choose a reason for hiding this comment

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

Good work, not a lot we can or should change.

config/consensus.go Outdated Show resolved Hide resolved
stateproof/verify/stateproof.go Show resolved Hide resolved
}

var verificationData *ledgercore.StateProofVerificationData
if config.Consensus[atRoundHdr.CurrentProtocol].StateProofUseTrackerVerification {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should cover the verification method switch with a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two tests -
1 TestCowStateProof check the old code - verification using blockheader
2 TestCowStateProofV34 - using the tracker.

@id-ms id-ms merged commit 0bd34b8 into algorand:feature/stateproofs-recoverability Nov 6, 2022
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Looks reasonable, and it looks like a time to review the entire change


// 100% coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

no more 100% coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just thought that the comments doesn't help much :)
If you think there is an edge case that we didn't cover, I will happily test it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants