Fix- extraProgramPages #2580

shiqizng · 2021-07-19T16:00:15Z

This PR contains patch proposed by @jeapostrophe that fixes issue 2431.

codecov-commenter · 2021-07-19T16:41:02Z

Codecov Report

Merging #2580 (a861a58) into master (c58349e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2580      +/-   ##
==========================================
+ Coverage   46.94%   46.97%   +0.03%     
==========================================
  Files         346      348       +2     
  Lines       55697    55725      +28     
==========================================
+ Hits        26147    26177      +30     
  Misses      26603    26603              
+ Partials     2947     2945       -2

Impacted Files	Coverage Δ
config/consensus.go	`83.79% <100.00%> (+0.05%)`	⬆️
data/transactions/transaction.go	`32.09% <100.00%> (+1.26%)`	⬆️
ledger/apply/application.go	`79.12% <100.00%> (+1.62%)`	⬆️
network/wsPeer.go	`72.14% <0.00%> (-2.79%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
ledger/eval.go	`76.38% <0.00%> (-0.30%)`	⬇️
catchup/service.go	`69.79% <0.00%> (ø)`
ledger/ledgercore/statedelta.go	`41.66% <0.00%> (ø)`
util/db/fullfsync_darwin.go	`100.00% <0.00%> (ø)`
libgoal/lockedFileUnix.go	`0.00% <0.00%> (ø)`
... and 10 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 c58349e...a861a58. Read the comment docs.

config/consensus.go

jannotti · 2021-07-19T18:26:46Z

ledger/apply/application_test.go

+	err = ApplicationCall(ac, h, &b, ad, &ep, txnCounter)
+	a.NoError(err)


Why does this work? You are updating with a 6k program, under the old proto. That should fail. Am I misreading?

this works because b is a mocked Balances obj and it actually invokes this mocked StatefulEval

go-algorand/ledger/apply/application_test.go

Line 189 in af3858c

func (b *testBalances) StatefulEval(params logic.EvalParams, aidx basics.AppIndex, program []byte) (passed bool, evalDelta basics.EvalDelta, err error) {

.

since I wanted to test updateApplication() only, b.pass is set to true at line 1006.

ApplicationCall should still have failed, I think, even before it got to StatefulEval(). Your extra program pages is 1 in params. But you are calling this with UpdateApplicationOC and a 6k approval program. Why does updateApplication allow such a big approval program?

I don't think we can trust these kinds of tests for actual transaction execution very much. We need to put them in e2e subs tests against a running network or we'll never have the level of confidence we need. There, we can try to update an app and then actually check to see that it's been changed.

I see the bug. In the new code, you don't check the length at all in old versions. But you must check the length - you must maintain the same behavior in old versions. So the behaviour should be:

Old version: check against a single program page of space
New version: check against the extra pages specified in params

I'll add an e2e test.
do I need to add a check for the old version in updateApplication() though?
doesn't these checks,

go-algorand/data/transactions/transaction.go

Lines 405 to 414 in e3e4197

pages := int(1 + effectiveEPP)

if lap > pages*proto.MaxAppProgramLen {

return fmt.Errorf("approval program too long. max len %d bytes", pages*proto.MaxAppProgramLen)

}

if lcs > pages*proto.MaxAppProgramLen {

return fmt.Errorf("clear state program too long. max len %d bytes", pages*proto.MaxAppProgramLen)

}

if lap+lcs > pages*proto.MaxAppTotalProgramLen {

return fmt.Errorf("app programs too long. max total len %d bytes", pages*proto.MaxAppTotalProgramLen)

}

enforce that the old version would maintain the same behavior?

I think you are right. I find it a bit confusing because in old code, a too big program is rejected in WellFormed, while in the new code, it's rejected in updateApplication. When just reading updateApplication, it appears we have no check at all unless proto.EnableExtraPagesOnAppUpdate is true.

Could you add a comment in updateApplication explaining that WellFormed rejects all multipage program updates when proto.EnableExtraPagesOnAppUpdate is false? Or else simply reject them here too, even though it's redundant.

jannotti · 2021-07-19T18:27:41Z

ledger/apply/application_test.go

+
+	b.pass = true
+	err = ApplicationCall(ac, h, &b, ad, &ep, txnCounter)
+	a.Contains(err.Error(), "updateApplication app programs too long")


We want to confirm that it IS possible to have a 3k approval program here. Currently, you can not update to 3k, even though your extra pages = 1 when you created it (which is what I think this is testing).

this test is for validating this added check is actually invoked,

go-algorand/ledger/apply/application.go

Lines 203 to 209 in af3858c

if proto.EnableExPagesOnAppUpdate {

allowed := int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen

actual := len(ac.ApprovalProgram) + len(ac.ClearStateProgram)

if actual > allowed {

return fmt.Errorf("updateApplication app programs too long, %d. max total len %d bytes", actual, allowed)

}

}

.

But, yes, I need to add another test to validate that it is possible to have a 3K approval program.

algorandskiy · 2021-07-22T18:05:35Z

ledger/apply/application_test.go

+	appr := make([]byte, 6050, 6050)
+
+	for i := range appr {
+		appr[i] = 2
+	}


nit:

Suggested change

appr := make([]byte, 6050, 6050)

for i := range appr {

appr[i] = 2

}

appr := bytes.Repeat([]byte{2}, 6050)

But! it not even needed since b.pass = true assumes logic.EvalStateful not even called.
so this works and setting zero byte to version 4 not needed as well:

Suggested change

appr := make([]byte, 6050, 6050)

for i := range appr {

appr[i] = 2

}

appr := make([]byte, 6050)

It's needed to validate this new check in updateApplication, https://github.com/shiqizng/go-algorand/blob/fix/exPages/ledger/apply/application.go#L208.

the line you pointed is a size check and not eval call (it is mocked in this unit test)

I get static check error from checkProgram if I set appr := make([]byte, 6050). check failed on ApprovalProgram: pc=702 static cost budget of 700 exceeded. https://github.com/shiqizng/go-algorand/blob/fix/exPages/ledger/apply/application.go#L364

then appr[0] = 4 is needed indeed

ledger/apply/application.go

data/transactions/transaction_test.go

algorandskiy · 2021-07-22T18:22:50Z

data/transactions/transaction.go

@@ -344,6 +344,7 @@ func (tx Transaction) WellFormed(spec SpecialAddresses, proto config.ConsensusPa
 			}
 		}

+		effectiveEPP := tx.ExtraProgramPages


call it maxExtraProgramPages ?

I think effectiveEPP is better because it doesn't always equal to maxExtraProgramPages. when ApplicationID=0, effectiveEPP = tx.ExtraProgramPages

ledger/apply/application_test.go

algorandskiy · 2021-07-22T23:31:42Z

data/transactions/transaction_test.go

+				},
+			},
+			spec:          specialAddr,
+			proto:         curProto,


this test will fail after the protocol upgrade, use v28 instead?

algorandskiy

LGTM

Fix for updating an application with extra program pages

shiqizng added 4 commits July 15, 2021 16:46

implement fix

911d0aa

Merge remote-tracking branch 'upstream/master' into fix/exPages

a6f8a02

add test

5b35502

update test

af3858c

shiqizng requested a review from jannotti July 19, 2021 16:06

jannotti reviewed Jul 19, 2021

View reviewed changes

shiqizng added 3 commits July 19, 2021 15:26

update concensus var name and added a test

e3e4197

added e2e test

a5c7905

e2e fix, added a comment in updateApplication

a861a58

algorandskiy requested changes Jul 22, 2021

View reviewed changes

algorand deleted a comment from algorandskiy Jul 22, 2021

algorandskiy reviewed Jul 22, 2021

View reviewed changes

shiqizng added 2 commits July 22, 2021 22:59

update tests

d6a64ff

update test

59f3d4c

shiqizng requested a review from algorandskiy July 23, 2021 15:42

resolve merge conflict

0bf0b8d

algorandskiy approved these changes Jul 26, 2021

View reviewed changes

algojohnlee merged commit 99ed67b into algorand:master Jul 27, 2021

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

Fix- extraProgramPages (#2580)

9645054

Fix for updating an application with extra program pages

bricerisingalgorand mentioned this pull request Aug 9, 2021

Hotfix/v2.9.1 #2704

Merged

onetechnical mentioned this pull request Aug 13, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix- extraProgramPages #2580

Fix- extraProgramPages #2580

shiqizng commented Jul 19, 2021

codecov-commenter commented Jul 19, 2021 •

edited

jannotti Jul 19, 2021

shiqizng Jul 19, 2021

jannotti Jul 19, 2021 •

edited

jannotti Jul 19, 2021

shiqizng Jul 19, 2021

jannotti Jul 20, 2021

jannotti Jul 19, 2021

shiqizng Jul 19, 2021 •

edited

algorandskiy Jul 22, 2021

shiqizng Jul 22, 2021

algorandskiy Jul 22, 2021

shiqizng Jul 23, 2021

algorandskiy Jul 23, 2021

algorandskiy Jul 22, 2021

shiqizng Jul 22, 2021

algorandskiy Jul 22, 2021

algorandskiy left a comment

		err = ApplicationCall(ac, h, &b, ad, &ep, txnCounter)
		a.NoError(err)

	pages := int(1 + effectiveEPP)
	if lap > pages*proto.MaxAppProgramLen {
	return fmt.Errorf("approval program too long. max len %d bytes", pages*proto.MaxAppProgramLen)
	}
	if lcs > pages*proto.MaxAppProgramLen {
	return fmt.Errorf("clear state program too long. max len %d bytes", pages*proto.MaxAppProgramLen)
	}
	if lap+lcs > pages*proto.MaxAppTotalProgramLen {
	return fmt.Errorf("app programs too long. max total len %d bytes", pages*proto.MaxAppTotalProgramLen)
	}

	if proto.EnableExPagesOnAppUpdate {
	allowed := int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen
	actual := len(ac.ApprovalProgram) + len(ac.ClearStateProgram)
	if actual > allowed {
	return fmt.Errorf("updateApplication app programs too long, %d. max total len %d bytes", actual, allowed)
	}
	}

Fix- extraProgramPages #2580

Fix- extraProgramPages #2580

Conversation

shiqizng commented Jul 19, 2021

codecov-commenter commented Jul 19, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannotti Jul 19, 2021 • edited

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

shiqizng Jul 19, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 19, 2021 •

edited

jannotti Jul 19, 2021 •

edited

shiqizng Jul 19, 2021 •

edited