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

Make config.MaxLogCalls dependant on some consensus param #2732

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Aug 11, 2021

Summary

  • Commented some constants in teal evaluator and added a test
    enforcing their values and directing on how to change them if/when needed.

Test Plan

Existing tests

data/basics/teal.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

Codecov Report

Merging #2732 (e6b06fa) into master (58bc231) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2732      +/-   ##
==========================================
- Coverage   47.09%   47.09%   -0.01%     
==========================================
  Files         350      350              
  Lines       56309    56309              
==========================================
- Hits        26520    26519       -1     
  Misses      26817    26817              
- Partials     2972     2973       +1     
Impacted Files Coverage Δ
config/consensus.go 84.01% <ø> (ø)
data/basics/msgp_gen.go 35.46% <0.00%> (ø)
data/basics/teal.go 38.61% <ø> (ø)
data/transactions/logic/eval.go 90.61% <ø> (ø)
ledger/blockqueue.go 81.03% <0.00%> (-4.03%) ⬇️
catchup/service.go 68.57% <0.00%> (-1.56%) ⬇️
ledger/acctupdates.go 62.55% <0.00%> (-0.09%) ⬇️
data/transactions/verify/txn.go 45.08% <0.00%> (ø)
network/wsNetwork.go 61.11% <0.00%> (+0.18%) ⬆️
network/requestTracker.go 71.12% <0.00%> (+0.43%) ⬆️
... 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 58bc231...e6b06fa. Read the comment docs.

// MaxLogCalls is the highest allowable log messages that may appear in
// any version, used for decoding purposes. Never decrease this value.
const MaxLogCalls = 32

// MaxLogicSigMaxSize is the largest logical signature appear in any of the supported
// protocols, used for decoding purposes.
var MaxLogicSigMaxSize int
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to move this one variable out when all the nearby values are of the same variety - they are not consensus parameters, rather they are conservative esitmates of the largest that some encoded message part can be. It's not just this variable that is like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with other values they are derived from consensus param, and this one is not because MaxLogCalls not in consensus. In fact it should not be even called Max...

@@ -57,7 +57,7 @@ const MaxByteMathSize = 64
const MaxLogSize = 1024

// MaxLogCalls is the limit of total log calls during a program execution
const MaxLogCalls = config.MaxLogCalls
const MaxLogCalls = 32
Copy link
Contributor

Choose a reason for hiding this comment

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

By not using the same variable, the MaxEncodedTealLogCalls becomes completely "invisible". A reasonable person might make it so that teal v6 allows more logs calls, but preserves compatibility for v5 and below. They would not even know there is a place where we put alloc bounds on it.

Here's an entirely different proposal that I think is simple, correct, and the same as what we do elsewhere. Notice how we set the bounds on deltas:

	// These bounds could be tighter, but since these values are just to
	// prevent DoS, setting them to be the maximum number of allowed
	// executed TEAL instructions should be fine (order of ~1000)
	checkSetMax(p.MaxAppProgramLen, &MaxStateDeltaKeys)
	checkSetMax(p.MaxAppProgramLen, &MaxEvalDeltaAccounts)
	checkSetMax(p.MaxAppProgramLen, &MaxAppProgramLen)

This is wildly bigger than it needs to be, but we're only trying to avoid crazy huge DoS messages. So let's just set the bound on MaxLogCalls the same way. Add:
checkSetMax(p.MaxAppProgramLen, &MaxLogCalls)

Now it's derived from consensus and doesn't need to be revisited.

Although we have another problem - nowadays, I suppose these bounds are not as obviously correct as they used to be. You could loop, setting many keys, perhaps more that 1000. We could use the MaxAppCost, rather than MaxAppLen though, since that's what would stop us. (Though it would need to be MaxGroupSize*MaxAppCost. because of pooling)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks reasonable, done

* Commented some constants in teal evaluator and added a test
  enforcing their values and directing on how to change them if/when needed.
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Seems good, glad we found middle ground.

@algorandskiy algorandskiy changed the title Rename config.MaxLogCalls to basics.MaxEncodedTealLogCalls Make config.MaxLogCalls dependant on some consensus param Aug 13, 2021
Copy link
Contributor

@shiqizng shiqizng left a comment

Choose a reason for hiding this comment

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

look good to me

@jannotti jannotti merged commit 80bd5ed into algorand:master Aug 13, 2021
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.

None yet

4 participants