Limit the longest validity period allowed for key registration to pre… #3181

Aharonee · 2021-11-03T12:55:15Z

…vent merklekeystore tree from being too big

Summary

Test Plan

codecov-commenter · 2021-11-03T13:28:07Z

Codecov Report

Merging #3181 (eff1253) into feature/dilithium-scheme-integration (ea61ca0) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@                           Coverage Diff                            @@
##           feature/dilithium-scheme-integration    #3181      +/-   ##
========================================================================
- Coverage                                 47.62%   47.61%   -0.01%     
========================================================================
  Files                                       381      381              
  Lines                                     61460    61466       +6     
========================================================================
- Hits                                      29268    29265       -3     
- Misses                                    28796    28804       +8     
- Partials                                   3396     3397       +1

Impacted Files	Coverage Δ
crypto/merklekeystore/keystore.go	`74.68% <ø> (ø)`
data/account/participation.go	`65.21% <83.33%> (+0.93%)`	⬆️
config/consensus.go	`84.74% <100.00%> (+0.04%)`	⬆️
data/transactions/transaction.go	`37.07% <100.00%> (+0.47%)`	⬆️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
cmd/tealdbg/debugger.go	`72.86% <0.00%> (-1.01%)`	⬇️
data/abi/abi_type.go	`92.03% <0.00%> (-1.00%)`	⬇️
ledger/acctupdates.go	`65.01% <0.00%> (-0.96%)`	⬇️
network/requestTracker.go	`70.25% <0.00%> (-0.87%)`	⬇️
data/transactions/verify/txn.go	`44.29% <0.00%> (ø)`
... and 3 more

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 ea61ca0...eff1253. Read the comment docs.

crypto/merklekeystore/keystore.go

crypto/merklekeystore/keystore_test.go

algonautshant

Can you please merge/rebase from feature/dilithium-scheme-integration?

crypto/merklekeystore/keystore.go

…vent merklekeystore tree from being too big

…backwards compatibility

algonautshant

Looks great.
Just some minor changes are needed.

crypto/merklekeystore/keystore.go

algonautshant · 2021-11-24T03:10:35Z

config/consensus_test.go

+
+	for _, params := range Consensus {
+		if params.CompactCertRounds != 0 {
+			require.Equal(t, uint64(1<<16), (params.MaxKeyregValidPeriod+1)/params.CompactCertRounds,


Can the "16" be replaced by an existing variable representing that value?

I can use merklearray.MaxTreeDepth, but what about the consensus parameters (MaxKeyregValidPeriod for example), should it use that as well? If not there might be some inconsistency in the future.

Let's postpone this parameter issue to a later PR. #3257

crypto/merklekeystore/keystore_test.go

algonautshant

Merge Aharonee#1 if you agree with it.
Then should be good to go.

algonautshant · 2021-11-26T01:47:13Z

Some casting to uint64 (e.g. for firstValid/lastValid) are not needed.
I think firstValid/lastValid parameters in New and other interfaces should be basics.Round. Casting form basics.Round to uint64 should not be done.

crypto/compactcert/builder_test.go

algonautshant · 2021-11-26T02:07:44Z

crypto/compactcert/builder_test.go

@@ -117,11 +121,11 @@ func TestBuildVerify(t *testing.T) {
 		ProvenWeight:      uint64(totalWeight / 2),
 		SigRound:          currentRound,
 		SecKQ:             128,


It is a sound design to have crypto independent of the consensus parameters.

However, the hard-coded values in the tests which are repeating the consensus values does not look right. And since the compiler-enforced link of these values to the consensus parameters is broken, this can be problematic in the event the consensus values change.

CompactCertRounds is getting its value from go-algorand/config, but SecKQ is not. They need to be uniform.

Let's postpone this #3257

data/account/participation.go

algonautshant · 2021-11-26T02:52:38Z

data/transactions/logic/evalAppTxn_test.go

@@ -609,10 +609,29 @@ func TestKeyReg(t *testing.T) {
 `
 		ep, ledger := makeSampleEnv()
 		ep.Proto.EnableStateProofKeyregCheck = true
+		ep.Proto.MaxKeyregValidPeriod = (1<<16)*128 - 1 // 2^16 StateProof keys times CompactCertRounds (interval)


Maybe not in this PR, but we need to find a way to consolidate these hard-coded values into a variable in a single location and use that.

Yes that would definitely be better, we need a single source of truth for all these place to inherit from

crypto/merklekeystore/keystore.go

data/transactions/transaction.go

Aharonee requested a review from id-ms November 3, 2021 12:55

tsachiherman reviewed Nov 4, 2021

View reviewed changes

crypto/merklekeystore/keystore.go Outdated Show resolved Hide resolved

tsachiherman reviewed Nov 4, 2021

View reviewed changes

crypto/merklekeystore/keystore.go Outdated Show resolved Hide resolved

tsachiherman reviewed Nov 4, 2021

View reviewed changes

crypto/merklekeystore/keystore.go Outdated Show resolved Hide resolved

tsachiherman reviewed Nov 4, 2021

View reviewed changes

crypto/merklekeystore/keystore_test.go Outdated Show resolved Hide resolved

algonautshant suggested changes Nov 10, 2021

View reviewed changes

crypto/merklekeystore/keystore.go Outdated Show resolved Hide resolved

Aharonee force-pushed the limit-max-merkletree-depth branch 2 times, most recently from 0df5c4f to be56ec4 Compare November 16, 2021 20:12

Aharonee requested review from tsachiherman and algonautshant November 16, 2021 20:16

Aharonee force-pushed the limit-max-merkletree-depth branch from 43cd29f to 8b4eaba Compare November 22, 2021 11:28

Aharonee added 12 commits November 23, 2021 16:00

Limit the longest validity period allowed for key registration to pre…

4802b0a

…vent merklekeystore tree from being too big

.

35c7a52

Fix validity limit and add some unit tests

af915dc

Small fix to condition and extend unit test

05f9ddc

Add a new Consensus parameter, unit test and some fixes in regard to …

0aee48a

…backwards compatibility

.

1e23154

Update and expand some unit tests

db298a9

update another unit test

a1dd729

add check in case param doesn't exist in consensus version

6807398

.

5bc8a3c

merge with Idan

eb5e3ee

Replace falcon with Ed25519 since it takes much less time

bd074ea

Aharonee force-pushed the limit-max-merkletree-depth branch from 8b4eaba to bd074ea Compare November 23, 2021 14:01

algonautshant suggested changes Nov 24, 2021

View reviewed changes

Aharonee added 3 commits November 24, 2021 15:03

minor fixes

e186b15

fix bug with defer Close() the same variable

e222b1f

revert some changes

76e65ef

algonautshant reviewed Nov 24, 2021

View reviewed changes

crypto/merklekeystore/keystore_test.go Outdated Show resolved Hide resolved

algonautshant suggested changes Nov 24, 2021

View reviewed changes

Refactoring to separate crypto dependency on Consensus

20a2aca

algonautshant suggested changes Nov 26, 2021

View reviewed changes

id-ms reviewed Nov 30, 2021

View reviewed changes

data/transactions/transaction.go Outdated Show resolved Hide resolved

Separate unit tests to long and short ones, small refactoring

eff1253

Aharonee force-pushed the limit-max-merkletree-depth branch from 48b3cf6 to eff1253 Compare November 30, 2021 15:40

algonautshant mentioned this pull request Nov 30, 2021

crypto parameters and dependence on config params #3257

Closed

algonautshant approved these changes Nov 30, 2021

View reviewed changes

algonautshant merged commit 5a71da5 into algorand:feature/dilithium-scheme-integration Nov 30, 2021

egieseke mentioned this pull request Feb 16, 2022

Script to generate go-algorand release notes #3520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit the longest validity period allowed for key registration to pre… #3181

Limit the longest validity period allowed for key registration to pre… #3181

Aharonee commented Nov 3, 2021

codecov-commenter commented Nov 3, 2021 •

edited

Loading

algonautshant left a comment

algonautshant left a comment

algonautshant Nov 24, 2021

Aharonee Nov 24, 2021

algonautshant Nov 30, 2021 •

edited

Loading

algonautshant left a comment

algonautshant commented Nov 26, 2021

algonautshant Nov 26, 2021

algonautshant Nov 30, 2021

algonautshant Nov 26, 2021

Aharonee Nov 28, 2021

algonautshant Nov 30, 2021

Limit the longest validity period allowed for key registration to pre… #3181

Limit the longest validity period allowed for key registration to pre… #3181

Conversation

Aharonee commented Nov 3, 2021

Summary

Test Plan

codecov-commenter commented Nov 3, 2021 • edited Loading

Codecov Report

algonautshant left a comment

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

algonautshant Nov 24, 2021

Choose a reason for hiding this comment

Aharonee Nov 24, 2021

Choose a reason for hiding this comment

algonautshant Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

algonautshant commented Nov 26, 2021

algonautshant Nov 26, 2021

Choose a reason for hiding this comment

algonautshant Nov 30, 2021

Choose a reason for hiding this comment

algonautshant Nov 26, 2021

Choose a reason for hiding this comment

Aharonee Nov 28, 2021

Choose a reason for hiding this comment

algonautshant Nov 30, 2021

Choose a reason for hiding this comment

codecov-commenter commented Nov 3, 2021 •

edited

Loading

algonautshant Nov 30, 2021 •

edited

Loading