contracts: Use WeakBoundedVec for instrumented code

[athei]

cumulus companion: paritytech/cumulus#1595

Every contract code is stored in two forms: The pristine wasm blob as uploaded and a instrumented version that functions as cache so that the costly instrumentation does not need to be repeated on every call. The instrumented version is always bigger than the pristine version since instructions are added. When a code is uploaded we enforce the same maximum for the pristine and the instrumented version. Therefore contract code which is too big after instrumentation can't be deployed and the transaction simply fails which tries to upload it.

However, whenever the cost schedule of pallet-contracts changes any contract already deployed on-chain needs to be re-instrumented which happens on the next call of any contract. This re-instrumentation of the on-chain pristine code could potentially increase the size above the maximum if the instrumentation algorithm changed so that it emits more code than before. We can't fail in this case because that would make the contract inaccessible since there is no way to change its code. Previously, we just allowed the code to be much bigger after instrumentation which pushed the MaxEncodedLen way too high and harmed the throughput in the happy case.

This PR changes the logic to just allow the code to be bigger than the maximum after re-instrumentation. We argue this is safe since the user does not control this size. We just need to be careful to not change the instrumentation in a way that inflates the code massively.

[agryaznov]

Now that the re-instrumented code size could excess max_code_size, shoudn't we charge more gas for it here? Like CodeToken::Reinstrument(max_code_len.max(code_size)).

Just a thought, what if we'd charge gas exponentially on code_size - max_code_size if re-instrumented code size excesses its maximum boundary? I suppose this could be done in the benchmark. Or, once the point here is possible excessive storage use, maybe we could take more storage deposit on the transaction leading to code re-instrumentation? This would economically stop user from using the contract if its re-instrumented code becomes too large (i.e. from sending a first tx leading to such a re-instrumentation), while still keeping the contract from becoming a tombstone (this part is already solved by your changes in this PR).

[athei]

Now that the re-instrumented code size could excess max_code_size, shoudn't we charge more gas for it here? Like CodeToken::Reinstrument(max_code_len.max(code_size)).

The weight is based on the non instrumented size. Reason is that coming up with a benchmark for the instrumented size is way too hard and sketchy.

Just a thought, what if we'd charge gas exponentially on code_size - max_code_size if re-instrumented code size excesses its maximum boundary?

The point of this whole PR is to make the contract inaccessible if the size gets a bit bigger. We basically ignore it because the only way this can be exploited is if we push a buggy instrumentation.

Or, once the point here is possible excessive storage use, maybe we could take more storage deposit on the transaction leading to code re-instrumentation?

Yeah I think this makes sense but the deposit for the code blob is charged from the original uploader and not the caller. Would be very complicated to take a surcharge here from the caller.

This whole PR is about the fact that we cannot really do much if some instrumentation change will greatly increase the code blob size. We don't expect that this will happen. If this will happen we know because we are the ones changing it. Then we need to take a closer look how to address this. In the meantime we just make sure that the contract can't get in accessible by some minor changes.

[cmichi]

Why WeakBoundedVec instead of BoundedVec?

According to https://substrate.stackexchange.com/questions/1152/when-to-use-boundedvec-vs-weakboundedvec-vs-vec:

Please don't use WeakBoundedVec ever, unless you are migrating code which you plan to fix in the future.

[athei]

It is explained in the PR: We cannot put a bound on that on re-instrumented contracts. It will make contracts inaccessible. Long term goal is to make instrumentation on the fly and get rid of the cache.

[agryaznov]

The point of this whole PR is to make the contract inaccessible if the size gets a bit bigger. We basically ignore it because the only way this can be exploited is if we push a buggy instrumentation.

My suggestion was: okay, if the instrumented code size becomes just "a bit" bigger, we can ignore it (and charge more or less the same amount of gas), meanwhile, if it becomes significantly bigger, we charge a lot more gas (i.e. let's make charged gas calculated as an exponential function on the excess value of the new contract size). In case of a buggy instrumentation code is introduced, this probably will lead to exceed of gas limit for the transaction that initializes the re-instrumentation, thus making it too expensive to use the contract if its re-instrumented code size becomes too large. By implementing it this way, we'll make the possible outcome of a developer mistake less damaging for the network as a whole: instead of the network-wide storage congestion, we'll get signals from contract users that using the contracts became too way expensive, and will be able to fix the problem without altering chain data much.

Same effect could probably be reached by making the first transaction leading to the re-instrumentation too expensive because of the storage deposit, again only in case the new contract size overwhelms limits significantly.

[athei]

I understand and agree with the goals. However, I don't think it is feasible with reasonable effort:

1. We can't take an additional deposit because the code deposit is tied to the code owner not the caller of the re-instrumentation. If we just take a deposit it would be stored into the contract and be subject to the pro rata refunds. It would not work because it adds deposit which is not accounted for by bytes and items deposited.

2. The solution to charge a prohibitive amount from the gas meter is just using a BoundedVec with extra steps. It makes the contract inaccessible in case something goes wrong. Also, it is unfair to charge the user in case the instrumentation changes to add a few more bytes.

I know the situation is not optimal but I feel the cure is worse than the disease.

[athei]

bot merge