Skip to content

Make config.MaxLogCalls dependant on some consensus param #2732

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.

Sign up for GitHub

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

Merged
jannotti merged 1 commit into from
Aug 13, 2021
Merged

Make config.MaxLogCalls dependant on some consensus param #2732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions config/consensus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,8 +423,8 @@ var MaxEvalDeltaAccounts int
var MaxStateDeltaKeys int

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

// MaxLogicSigMaxSize is the largest logical signature appear in any of the supported
// protocols, used for decoding purposes.
Expand Down Expand Up @@ -486,6 +486,9 @@ func checkSetAllocBounds(p ConsensusParams) {
checkSetMax(p.MaxExtraAppProgramPages, &MaxExtraAppProgramLen)
// MaxAvailableAppProgramLen is the max of supported app program size
MaxAvailableAppProgramLen = MaxAppProgramLen * (1 + MaxExtraAppProgramLen)
// There is no consensus parameter for MaxLogCalls and MaxAppProgramLen as an approximation
// Its value is much larger than any possible reasonable MaxLogCalls value in future
checkSetMax(p.MaxAppProgramLen, &MaxLogCalls)
}

// SaveConfigurableConsensus saves the configurable protocols file to the provided data directory.
Expand Down
9 changes: 9 additions & 0 deletions data/transactions/logic/backwardCompat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,3 +404,12 @@ done:`
})
}
}

func TestExplicitConstants(t *testing.T) {
partitiontest.PartitionTest(t)

require.Equal(t, 4096, MaxStringSize, "constant changed, move it to consensus params")
require.Equal(t, 64, MaxByteMathSize, "constant changed, move it to consensus params")
require.Equal(t, 1024, MaxLogSize, "constant changed, move it to consensus params")
require.Equal(t, 32, MaxLogCalls, "constant changed, move it to consensus params")
}
6 changes: 3 additions & 3 deletions data/transactions/logic/eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ import (
// EvalMaxVersion is the max version we can interpret and run
const EvalMaxVersion = LogicVersion

// EvalMaxScratchSize is the maximum number of scratch slots.
const EvalMaxScratchSize = 255
// The constants below control TEAL opcodes evaluation and MAY NOT be changed
// without moving them into consensus parameters.

// MaxStringSize is the limit of byte strings created by `concat`
const MaxStringSize = 4096
Expand All @@ -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

@jannotti jannotti Aug 12, 2021

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

@algorandskiy algorandskiy Aug 13, 2021

Choose a reason for hiding this comment

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

Looks reasonable, done


// stackValue is the type for the operand stack.
// Each stackValue is either a valid []byte value or a uint64 value.
Expand Down
4 changes: 2 additions & 2 deletions data/transactions/logic/eval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4764,7 +4764,7 @@ func TestLog(t *testing.T) {
loglen: 2,
},
{
source: fmt.Sprintf(`%s int 1`, strings.Repeat(`byte "a logging message"; log;`, config.MaxLogCalls)),
source: fmt.Sprintf(`%s int 1`, strings.Repeat(`byte "a logging message"; log;`, MaxLogCalls)),
loglen: MaxLogCalls,
},
{
Expand Down Expand Up @@ -4816,7 +4816,7 @@ func TestLog(t *testing.T) {
runMode: runModeApplication,
},
{
source: fmt.Sprintf(`%s; int 1`, strings.Repeat(`byte "a"; log;`, config.MaxLogCalls+1)),
source: fmt.Sprintf(`%s; int 1`, strings.Repeat(`byte "a"; log;`, MaxLogCalls+1)),
errContains: "too many log calls",
runMode: runModeApplication,
},
Expand Down