Consensus upgrade for ExtraProgramPages fix #2694

algorandskiy · 2021-08-05T19:11:36Z

Summary

Make sure there is no overflow on TotalExtraAppPages calculation + test

Test Plan

One new test for overflow check on subtract

data/basics/overflow.go

ledger/apply/application.go

shiqizng · 2021-08-05T19:16:56Z

data/basics/overflow.go

a typo in the comment?

ledger/apply/application_test.go

shiqizng · 2021-08-05T19:39:53Z

ledger/apply/application_test.go

I'm not following why br.TotalExtraAppPages==expTotalExtraPages-1. isn't it set to expTotalExtraPages on line 1116?

expTotalExtraPages is a poor name, it should be initTotalExtraPages

tsachiherman · 2021-08-05T19:40:32Z

ledger/apply/application.go

if we got below zero here, then we have a bug elsewhere. I'm not sure what the best course of action here.

I kind of agree but 1) it should not never happen :) 2) I guess being error tolerate here is fine

What, if anything, do we do during the same calculation on schemas?

Nothing,

totalSchema = totalSchema.SubSchema(globalSchema) record.TotalAppSchema = totalSchema

would set it to zeros

codecov-commenter · 2021-08-05T19:42:39Z

Codecov Report

Merging #2694 (10dbce6) into master (39be0c7) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2694      +/-   ##
==========================================
- Coverage   47.07%   47.06%   -0.01%     
==========================================
  Files         349      349              
  Lines       55858    55878      +20     
==========================================
+ Hits        26294    26301       +7     
- Misses      26617    26624       +7     
- Partials     2947     2953       +6

Impacted Files	Coverage Δ
protocol/consensus.go	`0.00% <ø> (ø)`
config/consensus.go	`84.01% <100.00%> (+0.22%)`	⬆️
data/basics/overflow.go	`43.42% <100.00%> (+12.77%)`	⬆️
ledger/apply/application.go	`80.59% <100.00%> (+2.20%)`	⬆️
ledger/blockqueue.go	`81.03% <0.00%> (-4.03%)`	⬇️
network/wsPeer.go	`72.14% <0.00%> (-2.23%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
ledger/acctupdates.go	`61.88% <0.00%> (-0.42%)`	⬇️
network/wsNetwork.go	`60.73% <0.00%> (-0.19%)`	⬇️
catchup/service.go	`69.35% <0.00%> (+0.77%)`	⬆️
... and 2 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 39be0c7...10dbce6. Read the comment docs.

tsachiherman

The changes is this PR, as far as I can tell, are flawless.
However, I found an issue in the previous EnableExtraPagesOnAppUpdate implementation:

updateApplication:
The code tests len(ac.ApprovalProgram) + len(ac.ClearStateProgram) against int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen, which is incorrect.
It should test each of the programs against int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen and test the sum of their length against MaxAppTotalProgramLen

shiqizng · 2021-08-06T13:33:58Z

The changes is this PR, as far as I can tell, are flawless.
However, I found an issue in the previous EnableExtraPagesOnAppUpdate implementation:

updateApplication:
The code tests len(ac.ApprovalProgram) + len(ac.ClearStateProgram) against int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen, which is incorrect.
It should test each of the programs against int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen and test the sum of their length against MaxAppTotalProgramLen

I believe program size calculation has been combined for approval and clear state. MaxAppProgramLen is increased to 2048 in v28.

tsachiherman · 2021-08-06T13:54:14Z

The changes is this PR, as far as I can tell, are flawless.
However, I found an issue in the previous EnableExtraPagesOnAppUpdate implementation:

updateApplication:
The code tests len(ac.ApprovalProgram) + len(ac.ClearStateProgram) against int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen, which is incorrect.
It should test each of the programs against int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen and test the sum of their length against MaxAppTotalProgramLen

I believe program size calculation has been combined for approval and clear state. MaxAppProgramLen is increased to 2048 in v28.

Correct - the total size was updated to be 2048. But the check in the updateApplication should align with the check in the WellFormed for new apps. I believe that it's not an issue as long as the MaxAppProgramLen == MaxAppTotalProgramLen, but it could be an issue if they differ.
i.e. if we increase the size of MaxAppTotalProgramLen to 4096, for instance, we could create an app with two programs (1K+1K) or with (2K+2K), but we won't be able to them to be (2K+2K).

* Make sure there is no overflow on TotalExtraAppPages calculation + test * proto.MaxAppTotalProgramLen is used in updateApplication

algorandskiy · 2021-08-06T15:14:22Z

Thank you, changed to MaxAppTotalProgramLen. Merged-in a e2e test from @jasonpaulos

tsachiherman · 2021-08-06T16:05:40Z

daemon/algod/api/client/restClient.go

+func (client RestClient) PendingTransactionInformationV2(transactionID string) (response generatedV2.PendingTransactionResponse, err error) {
+	transactionID = stripTransaction(transactionID)
+	err = client.get(&response, fmt.Sprintf("/v2/transactions/pending/%s", transactionID), nil)
+	return
+}


This should probably be implemented with versionAffinity, but it doesn't have to be implemented with that on this PR.

…ate (#604) Consensus upgrade for ExtraProgramPages: algorand/go-algorand#2694 Added EnableExtraPagesOnAppUpdate to v29 update.

Make sure there is no overflow on TotalExtraAppPages calculation + test

ledger/apply/application.go

algorandskiy requested review from jannotti, shiqizng and tsachiherman August 5, 2021 19:11

tsachiherman reviewed Aug 5, 2021

View reviewed changes

data/basics/overflow.go Outdated Show resolved Hide resolved

tsachiherman reviewed Aug 5, 2021

View reviewed changes

ledger/apply/application.go Outdated Show resolved Hide resolved

shiqizng reviewed Aug 5, 2021

View reviewed changes

tsachiherman reviewed Aug 5, 2021

View reviewed changes

algorandskiy force-pushed the pavel/extra-page-upgrade branch from 760def1 to b598a3d Compare August 5, 2021 20:38

algorandskiy self-assigned this Aug 5, 2021

algorandskiy added Team Scytale unplanned labels Aug 5, 2021

algorandskiy force-pushed the pavel/extra-page-upgrade branch from b598a3d to 10dbce6 Compare August 5, 2021 21:46

algorandskiy changed the title ~~WIP: Consensus upgrade for ExtraProgramPages fix~~ Consensus upgrade for ExtraProgramPages fix Aug 5, 2021

algorandskiy requested review from shiqizng and tsachiherman August 5, 2021 21:46

jannotti previously approved these changes Aug 5, 2021

View reviewed changes

algochoi mentioned this pull request Aug 5, 2021

Update v29 consensus protocol parameters for EnableExtraPagesOnAppUpdate algorand/indexer#604

Merged

tsachiherman reviewed Aug 6, 2021

View reviewed changes

Consensus upgrade for ExtraProgramPages fix

42e260b

* Make sure there is no overflow on TotalExtraAppPages calculation + test * proto.MaxAppTotalProgramLen is used in updateApplication

algorandskiy dismissed jannotti’s stale review via 42e260b August 6, 2021 15:07

algorandskiy force-pushed the pavel/extra-page-upgrade branch from 10dbce6 to 42e260b Compare August 6, 2021 15:07

Add e2e test for extra pages

696c34e

algorandskiy force-pushed the pavel/extra-page-upgrade branch from 90bef2b to 696c34e Compare August 6, 2021 15:13

algorandskiy requested review from jannotti, jasonpaulos and tsachiherman August 6, 2021 15:14

shiqizng previously approved these changes Aug 6, 2021

View reviewed changes

jasonpaulos previously approved these changes Aug 6, 2021

View reviewed changes

Fix linter

c38d8dd

algorandskiy dismissed stale reviews from jasonpaulos and shiqizng via c38d8dd August 6, 2021 15:36

jasonpaulos approved these changes Aug 6, 2021

View reviewed changes

tsachiherman reviewed Aug 6, 2021

View reviewed changes

shiqizng approved these changes Aug 6, 2021

View reviewed changes

algojohnlee merged commit f3671c0 into algorand:master Aug 6, 2021

bricerisingalgorand pushed a commit that referenced this pull request Aug 6, 2021

Consensus upgrade for ExtraProgramPages fix (#2694)

a362c4d

Make sure there is no overflow on TotalExtraAppPages calculation + test

onetechnical mentioned this pull request Aug 13, 2021

go-algorand v2.9.1-stable re-merge #2740

Merged

This was referenced Aug 19, 2021

DO NOT MERGE --- go-algorand 2.10.0-stable #2773

Closed

go-algorand 2.10.1-beta #2800

Merged

bricerisingalgorand mentioned this pull request Aug 27, 2021

go-algorand 2.10.1-stable #2813

Merged

jannotti reviewed Jul 24, 2025

View reviewed changes

ledger/apply/application.go Show resolved Hide resolved

Consensus upgrade for ExtraProgramPages fix #2694

Consensus upgrade for ExtraProgramPages fix #2694

Uh oh!

Conversation

algorandskiy commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsachiherman left a comment

Choose a reason for hiding this comment

Uh oh!

shiqizng commented Aug 6, 2021

Uh oh!

tsachiherman commented Aug 6, 2021

Uh oh!

algorandskiy commented Aug 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

algorandskiy commented Aug 5, 2021 •

edited

Loading

codecov-commenter commented Aug 5, 2021 •

edited

Loading