AVM: Allow app downgrades in old protocol versions

[jannotti]

Added test for protocol version before inners were introduced to ensure downgrading is allowed then.

[algorandskiy]

moving MinInnerApplVersion back in protocols smells - my understanding is set of parameters in a protocol is fixed. It might be OK from a technical perspective, but looks really strange from the "protocol guarantees" and "specs" point of view.
so I suggest another fix for this specific problem: emulate the old behavior for pre v31 when MinInnerApplVersion was introduced.

[jannotti]

Are you suggesting that I detect the version (number?) and skip the downgrade check before v31? Or are you suggesting that I treat MinInnerApplyVersion=0 to mean that the downgrade check should not be done? Both seem like more complexity in the code. I don't think (a) is possible, and (b) seems like we're just admitting that the value exists before v31, but we are putting in the knowledge that it will start at 0 because that's how Go works.

[algorandskiy]

Or are you suggesting that I treat MinInnerApplyVersion=0 to mean that the downgrade check should not be done?
I suggest to do not change consensus params due to the concern above.

if proto.MinInnerApplVersion == 0 {
	// preserve pre v32 behavior
	if pv >= syncProgramsVersion && av < pv {
		return fmt.Errorf("program version downgrade: %d < %d", av, pv)
	}
} else if pav >= proto.MinInnerApplVersion && av < pav {
	return fmt.Errorf("program version downgrade: %d < %d", av, pv)
}

[jannotti]

In your suggestion, MinInnerApplVersion does still exist on old protocol versions, but we've also introduced the convention that setting it to 0 means something very different that setting it to non-zero. I think that makes it harder to reason about the code.
I prefer configuration over code. By setting the value in the right place, more than 50% of this code is eliminated.

[cce]

if you think of the protocol version as guaranteeing certain behavior as described in a spec, versus a set of specific parameter values in the code, I think it is OK to retroactively encode the behavior of a previous version with a new parameter variable that didn't exist back then. I assume we have done this kind of thing before?

[jannotti]

Yes, for example:
v24.MaxAppTotalProgramLen = 2048 // No effect until v28, when MaxAppProgramLen increased
v24.MaxAppSumKeyValueLens = 128 // Set here to have no effect until MaxAppBytesValueLen increases

	// Intended to have no effect in v24 (it's set to accounts +
	// asas + apps). In later vers, it allows increasing the
	// individual limits while maintaining same max references.
	v24.MaxAppTotalTxnReferences = 8

In a sense, it's what we do whenever we add an "Enable" value like:

	// Enable AssetCloseAmount field
	v25.EnableAssetCloseAmount = true

The default value, false, gets set on all old versions. It's just that in those cases we are happy with the Go default, so we don't go out of our way to say that we're adding that to all old versions. It would be strange to be ok with it when the value of 0 or false works for us, but to avoid it otherwise.

[cce]

I found one example of retroactively initializing a new consensus parameter in #172 when introducing MaxTxGroupSize and setting it default to 1 (instead of 0)
https://github.com/algorand/go-algorand/pull/172/files#diff-fe44f09c4d5977b5f5eaea29170b6a0748819c9d02271746a20d81a5f3efca17

[jannotti]

I think that when the retroactive change is done to codify something that wasn't even a parameter (the txgroup size is a good example) it is very hard to argue against. That what I felt I was doing here. Previously, there was a hardcoded value "syncVersion" set to 6, and there was a test against it to decide if we should prevent downgrades. When we decided we wanted that to change, it seemed proper to move that into a consensus parameter, so it could change properly.

My mistake was missing that the fact that, after the change, the simpler test is now run on all updates, so the new parameter had to be set from the earliest point updates could occur, not just since the old "syncVersion".

[algorandskiy]

So are you saying the syncVersion/ ExtraProgramChecksVersion appeared in the code with version v31 (with TEAL v6) and you just moved it into the params?

[codecov]

Codecov Report

Merging #4086 (ff17369) into rel/beta (1657db4) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##           rel/beta    #4086      +/-   ##
============================================
- Coverage     54.53%   54.49%   -0.05%     
============================================
  Files           391      391              
  Lines         48665    48665              
============================================
- Hits          26541    26519      -22     
- Misses        19896    19920      +24     
+ Partials       2228     2226       -2     

Impacted Files	Coverage Δ
config/consensus.go	85.71% <100.00%> (ø)
cmd/algoh/blockWatcher.go	77.77% <0.00%> (-3.18%)	⬇️
ledger/blockqueue.go	82.18% <0.00%> (-2.88%)	⬇️
crypto/merkletrie/trie.go	66.42% <0.00%> (-2.19%)	⬇️
crypto/merkletrie/node.go	91.62% <0.00%> (-1.87%)	⬇️
node/node.go	23.12% <0.00%> (-1.84%)	⬇️
catchup/service.go	68.88% <0.00%> (-0.75%)	⬇️
ledger/acctupdates.go	68.77% <0.00%> (-0.66%)	⬇️
network/wsPeer.go	70.41% <0.00%> (+2.46%)	⬆️
ledger/roundlru.go	96.22% <0.00%> (+5.66%)	⬆️

---

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 1657db4...ff17369. Read the comment docs.

[algorandskiy]

Ok, I digged the history and indeed ExtraProgramChecksVersion = 6 appeared with TEALv6 and MinInnerApplVersion value in the protocol. It is safe to proceed.