V31 consensus upgrade #3553

id-ms · 2022-02-02T16:25:37Z

Summary

Create an update path that would enable the following features:

Batch Verification (batch verification: add ed25519 batch verification implementation #3031)
State Proof Keys (Integrate State Proof keys #2990)
TEAL 6 (Feature/contract to contract -> master #3397)
Expired Participation Keys (Added Participation Key Expiration Check #2924)
Fix rewards calculation bug (ledger: fix NextRewardsState() #3403)

Test Plan

Unit tests added.

tsachiherman

Four more requests here:

Don't forget to update ConsensusCurrentVersion to ConsensusV30
Add v30.ApprovedUpgrades[protocol.ConsensusV31] = 140000
Add v31.RewardsCalculationFix to true ( and remove from vFuture )
Add v31.MaxProposedExpiredOnlineAccounts to 32 ( and remove from vFuture )

codecov-commenter · 2022-02-02T17:19:40Z

Codecov Report

Merging #3553 (00e75f8) into master (18b5ccb) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3553      +/-   ##
==========================================
+ Coverage   47.74%   47.77%   +0.02%     
==========================================
  Files         370      370              
  Lines       60262    60266       +4     
==========================================
+ Hits        28773    28790      +17     
+ Misses      28183    28170      -13     
  Partials     3306     3306

Impacted Files	Coverage Δ
protocol/consensus.go	`0.00% <ø> (ø)`
config/consensus.go	`84.98% <100.00%> (+0.19%)`	⬆️
data/transactions/verify/txn.go	`44.15% <0.00%> (-0.87%)`	⬇️
network/wsNetwork.go	`62.79% <0.00%> (-0.20%)`	⬇️
ledger/acctupdates.go	`66.60% <0.00%> (+0.18%)`	⬆️
network/wsPeer.go	`68.88% <0.00%> (+0.83%)`	⬆️
data/abi/abi_type.go	`88.62% <0.00%> (+0.94%)`	⬆️
network/requestTracker.go	`71.55% <0.00%> (+1.29%)`	⬆️
ledger/tracker.go	`75.75% <0.00%> (+1.29%)`	⬆️
ledger/blockqueue.go	`85.05% <0.00%> (+2.87%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18b5ccb...00e75f8. Read the comment docs.

config/consensus.go

Co-authored-by: Pavel Zbitskiy <[email protected]>

looks good, thanks for the changes.

jannotti

Looks ok from AVM 1.1 perspective. My other comments are very minor.

ledger/apply/keyreg_test.go

jannotti · 2022-02-10T19:06:02Z

test/e2e-go/features/participation/participationExpiration_test.go

@@ -76,7 +76,7 @@ func testExpirationAccounts(t *testing.T, fixture *fixtures.RestClientFixture, f
 		_, currentRound := fixture.GetBalanceAndRound(richAccount)
 		// account adds part key
 		partKeyFirstValid := uint64(0)
-		partKeyValidityPeriod := uint64(150)
+		partKeyValidityPeriod := uint64(150) // TODO: maybe increase?


What are the units? I would have guessed rounds, but that would be very short.

Yes they are rounds and it is very short, maybe we should put 1000-3000 there instead, as no State Proof keys will be generated for such a short interval.

Aharonee

I've added some minor fixes here: id-ms#15

Aharonee · 2022-02-10T19:33:51Z

test/e2e-go/features/participation/participationExpiration_test.go

@@ -76,7 +76,7 @@ func testExpirationAccounts(t *testing.T, fixture *fixtures.RestClientFixture, f
 		_, currentRound := fixture.GetBalanceAndRound(richAccount)
 		// account adds part key
 		partKeyFirstValid := uint64(0)
-		partKeyValidityPeriod := uint64(150)
+		partKeyValidityPeriod := uint64(150) // TODO: maybe increase?


Yes they are rounds and it is very short, maybe we should put 1000-3000 there instead, as no State Proof keys will be generated for such a short interval.

gmalouf

Non-blocking FYI, didn't update a few references to ConsensusFuture - quick find in IDE will turn some up.

participationRegistry_test.go
accountdb_test.go

I'm opening an issue for the general case - we should have a single approach to referencing the ConsensusParams protocol inclusive of current+future that does not have to change when we bump the version.

gmalouf · 2022-02-10T19:17:30Z

config/consensus.go

+	v31.EnableStateProofKeyregCheck = true
+
+	// Maximum validity period for key registration, to prevent generating too many StateProof keys
+	v31.MaxKeyregValidPeriod = 256*(1<<16) - 1


I was about to ask some questions about the numbers going into this formula, seems like intent is to address this in a follow on issue: #3257 cc @algonautshant

The assumption is that the source of the truth is here: v31.MaxKeyregValidPeriod = 256*(1<<16) - 1
Anywhere else should be referencing this parameter.

The issue in #3257 is to make sure this variable is used for testing (instead of hardcoding the same value elsewhere), and fix the package dependency issue.

This is not a blocking issue.

MaxKeyregValidPeriod combines 256 and 16, which are based on two different limits.
for example:
merklearray/merkle.go: MaxEncodedTreeDepth = 16

gmalouf · 2022-02-10T19:38:36Z

test/e2e-go/restAPI/restClient_test.go

@@ -590,6 +594,7 @@ func TestAccountParticipationInfo(t *testing.T) {
 	a.Equal(uint64(firstRound), account.Participation.VoteFirst, "API must print correct first participation round")
 	a.Equal(uint64(lastRound), account.Participation.VoteLast, "API must print correct last participation round")
 	a.Equal(dilution, account.Participation.VoteKeyDilution, "API must print correct key dilution")
+	// TODO: should we update the v1 API to support state proof? Currently it does not return this field.


@tsachiherman did we already address this?

I copied this comment from @Aharonee 's PR, but I believe that the answer is no, and I think it's fine. We want to avoid updating the v1 endpoint any further.

agreed, if we are using v1 somewhere internally that needs it, we should just convert it to using v2 instead

Aharonee · 2022-02-10T20:04:13Z

Non-blocking FYI, didn't update a few references to ConsensusFuture - quick find in IDE will turn some up.

participationRegistry_test.go accountdb_test.go

I'm opening an issue for the general case - we should have a single approach to referencing the ConsensusParams protocol inclusive of current+future that does not have to change when we bump the version.

@gmalouf If you're referring to data/account/participationRegistry_test.go:45 then it should remain ConsensusFuture still, since it references a consensus parameter which will only be enabled when we enable the Compact Certificates.

In ledger/accountdb_test.go:293 the ConsensusFuture can change in my opinion and the test won't break. I'll push a fix into id-ms#15.

tsachiherman · 2022-02-10T20:15:09Z

Non-blocking FYI, didn't update a few references to ConsensusFuture - quick find in IDE will turn some up.
participationRegistry_test.go accountdb_test.go
I'm opening an issue for the general case - we should have a single approach to referencing the ConsensusParams protocol inclusive of current+future that does not have to change when we bump the version.

@gmalouf If you're referring to data/account/participationRegistry_test.go:45 then it should remain ConsensusFuture still, since it references a consensus parameter which will only be enabled when we enable the Compact Certificates.

In ledger/accountdb_test.go:293 the ConsensusFuture can change in my opinion and the test won't break. I'll push a fix into algoidan#15.

@Aharonee I agree with @gmalouf here; the proper way to test this before the protocol is created is to take an existing protocol version, and "add" the needed flags to its parameters in order to test using a custom protocol. I know that this might mean slightly more work, but it's the "cleaner" way to accomplish that. It also means that there won't be any changes when we apply new protocol version.

gmalouf · 2022-02-10T21:12:42Z

Right, I was suggesting a reference to constants articulating what those limits source from would be ideal.

…

On Thu, Feb 10, 2022 at 4:10 PM Shant Karakashian ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In config/consensus.go <#3553 (comment)>: > + v31 := v30 + v31.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{} + v31.EnableBatchVerification = true + v31.RewardsCalculationFix = true + v31.MaxProposedExpiredOnlineAccounts = 32 + + // Enable TEAL 6 / AVM 1.1 + v31.LogicSigVersion = 6 + v31.EnableInnerTransactionPooling = true + v31.IsolateClearState = true + + // stat proof key registration + v31.EnableStateProofKeyregCheck = true + + // Maximum validity period for key registration, to prevent generating too many StateProof keys + v31.MaxKeyregValidPeriod = 256*(1<<16) - 1 MaxKeyregValidPeriod combines 256 and 16, which are based on two different limits. for example: merklearray/merkle.go: MaxEncodedTreeDepth = 16 — Reply to this email directly, view it on GitHub <#3553 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHP3U7U4DCVPZZOHKXIDI3U2QSTHANCNFSM5NMQONXQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

create v31 consensus version.

8b54ef8

id-ms changed the title ~~create v31 consensus version.~~ create v31 consensus version Feb 2, 2022

tsachiherman previously requested changes Feb 2, 2022

View reviewed changes

tsachiherman assigned id-ms Feb 2, 2022

tsachiherman added the Team Sphinx label Feb 2, 2022

algorandskiy reviewed Feb 2, 2022

View reviewed changes

config/consensus.go Outdated Show resolved Hide resolved

algoidan and others added 2 commits February 2, 2022 20:35

v31 include more feature + change the current

7d34cfa

Update config/consensus.go

355b97f

Co-authored-by: Pavel Zbitskiy <[email protected]>

tsachiherman added the High Priority label Feb 2, 2022

tsachiherman added 3 commits February 8, 2022 15:59

Merge branch 'master' into create-v31-version

00e75f8

Merge branch 'master' into create-v31-version

4236478

update upgrade.

3ce0afb

tsachiherman changed the title ~~create v31 consensus version~~ V31 consensus upgrade Feb 10, 2022

jannotti previously approved these changes Feb 10, 2022

View reviewed changes

Aharonee suggested changes Feb 10, 2022

View reviewed changes

gmalouf reviewed Feb 10, 2022

View reviewed changes

update per peer review

4ba3d33

tsachiherman dismissed jannotti’s stale review via 4ba3d33 February 10, 2022 20:23

tsachiherman requested a review from Aharonee February 10, 2022 20:23

Aharonee previously approved these changes Feb 10, 2022

View reviewed changes

reduce runtime of unit test: TestKeyregValidityPeriod

6326a0c

Aharonee dismissed their stale review via 6326a0c February 10, 2022 22:08

tsachiherman approved these changes Feb 10, 2022

View reviewed changes

tsachiherman merged commit 71e39cd into algorand:master Feb 10, 2022

onetechnical mentioned this pull request Feb 11, 2022

go-algorand 3.4.0-beta #3613

Merged

egieseke mentioned this pull request Feb 16, 2022

Script to generate go-algorand release notes #3520

Closed

id-ms deleted the create-v31-version branch February 28, 2022 16:11

onetechnical mentioned this pull request Mar 1, 2022

go-algorand 3.4.2-stable #3700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V31 consensus upgrade #3553

V31 consensus upgrade #3553

id-ms commented Feb 2, 2022 •

edited by tsachiherman

Loading

tsachiherman left a comment

codecov-commenter commented Feb 2, 2022 •

edited

Loading

jannotti left a comment

jannotti Feb 10, 2022

Aharonee Feb 10, 2022

Aharonee left a comment

Aharonee Feb 10, 2022

gmalouf left a comment

gmalouf Feb 10, 2022

algonautshant Feb 10, 2022

algonautshant Feb 10, 2022

gmalouf Feb 10, 2022

tsachiherman Feb 10, 2022

cce Feb 10, 2022

Aharonee commented Feb 10, 2022

tsachiherman commented Feb 10, 2022

gmalouf commented Feb 10, 2022 via email

V31 consensus upgrade #3553

V31 consensus upgrade #3553

Conversation

id-ms commented Feb 2, 2022 • edited by tsachiherman Loading

Summary

Test Plan

tsachiherman left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 2, 2022 • edited Loading

Codecov Report

jannotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aharonee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmalouf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aharonee commented Feb 10, 2022

tsachiherman commented Feb 10, 2022

gmalouf commented Feb 10, 2022 via email

id-ms commented Feb 2, 2022 •

edited by tsachiherman

Loading

codecov-commenter commented Feb 2, 2022 •

edited

Loading