contract preparation: actually replace all memories with a single memory import, including imported memories

[Ekleog-NEAR]

Before this change, our behavior regarding memory items was erratic at best:

• If passed with exactly one module-local memory, we would correctly remove it and replace it with an import
• If passed with more than one module-local memory, we would fail deserialization due to multiple memories being defined, and then ensure_no_internal_memory would be pointless
• If passed with one or many memory imports, they would be left in the prepared code, resulting in two memories being in the output webassembly, that would then fail the second validation

With this change, memory items are just all ignored, and a single memory import with the parameters we defined is added.

The test_multiple_memories test is updated to actually be a (manually-crafted because no tools seem to accept doing it) wasm blob that defines two memories, instead of a wasm blob that defines a memory import and incidentally due to preparation bugs turns out having multiple memories after preparation.

---

This is a draft PR because it will require a protocol upgrade. However, before doing this boilerplate work, I’d like to put that out here for other people to have a look and check they’d be ok with this in a finalized form.

Fixes #6042

[nagisa]

I’m happy with this.

[nagisa]

manually-crafted because no tools seem to accept doing it

They probably want multi-memory extension to be enabled. wasm-encoder should not mind either way though...

[matklad]

With this change, memory items are just all ignored, and a single memory import with the parameters we defined is added.

Hm, I think that's not the entirely correct logic here.

My understanding is we do not support multiple memories proposal. So our intention is only to allow contracts with exactly zero or one memory.

We shouldn't be making contracts with multiple memories valid (if there are two memories, and we remove one, than code might try to access non-existing memories). We also shouldn't allow to "manually" improt memories.

I think the code today mostly does all this:

• the validate_contract(original_code, config)?; should catch multiple memories
• we don't catch contract importing memory in the validation ....
• but we catch it during compilation, or when we try to link it

But yeah, it' be great do to all this in the preparation. And I think this doesn't have to be a protocol chage? We don't want to allow more contracts (so, two memories is still an error), we just want to shift a place where we emit an error to preparation from our defence in depth in compiler?

[nagisa]

My understanding is we do not support multiple memories proposal.

Correct.

So our intention is only to allow contracts with exactly zero or one memory.

Well, sort of. In practice it is okay if we "internally" always normalize this to exactly one imported memory. This is I believe what our prep was already trying to implement.

---

We shouldn't be making contracts with multiple memories valid (if there are two memories, and we remove one, than code might try to access non-existing memories). We also shouldn't allow to "manually" improt memories.

We’re still rejecting any contracts with multiple memories by the virtue of running any instrumentation code after validation has already passed (which rejects multiple memories). So the only contracts that reach this instrumentation code are: those that have no memories, those that have 1 internal memory, and those that import one memory.

That is, for contracts, if there was a valid memory index, this index is 0, and the same both before and after this PR. There isn’t a behaviour change for contract semantics.

This PR brings the behaviour wrt. contracts with imported memories in-line with the behaviour wrt. contracts with internal memories. In principle, this change means nearcore accepts a few more contracts (those that contain one imported memory,) but it isn’t a change that is obviously incorrect. We could instead reject contracts with imported memories instead of fixing them up, but then we either end up with inconsistent behaviour between internal and imported memories, or we implement a protocol change and disallow internal memories too.

[Ekleog-NEAR]

I’ve just implemented the review comments :)

@matklad As @nagisa mentioned, the current behavior is:

• Contracts with no imported memories and zero or one local memories are accepted
• Contracts with imported memories or more than one local memories are rejected
• Anyways, whichever memories are imported by the contract are normalized so as to strictly limit their sizes, so the metadata attached to them is ignored

And we have a choice between:

1. Deciding that the current behavior is correct, and enforcing it manually rather than as a side-effect of a bug in our contract preparation
2. Deciding that contracts with one imported memory should suffer the same memory normalization treatment as contracts with one local memory, which requires a protocol upgrade and is basically this PR

TBH, upon more thought I’m no longer sure this PR is the right way to go, and maybe instead we should just go with option 2, because that’s one fewer protocol upgrade and we don’t really care about contracts with imported memories anyway…

WDYT?

[matklad]

This is I believe what our prep was already trying to implement.

Hm, my reading is slightly different: I think the intention is to take contract with one internal memory, and make it a contract with one imported memory.

I think the motivation is basically "accidents of our implementation". Compilers produce .wasm with an internal memory, and, ideally, we want to run just that. But in our implementation we want to read from VM's memory directly, so we want to somehow inject the memory we control, so we want to substitute an import for a memory. That is, I don't think there's good reason why we need an imported memory at all, I think that's the accident of implementation.

Again, not sure, but the fact that the code explicitly does .pop(), rather than .clear(), and then an ensure_no_memories points to me at that intention.

[matklad]

Contracts with imported memories or more than one local memories are rejected

I think the case where ther is an imported memory, no local memories, and the imported memory happens to match our own config,we also accept.

---

I think what we should do is:

• maintain the current behavior (so, don't do a protocol change)
• make sure that contract with bad imported memories is caught during preparation, and not later.

The core is that I think we can fix the bug (catching invalid contract too late) while not doing a protocol version?

Separately from that, it does seem to me that the current behavior of swapping one local memory for one import is the reasonable one (*), and that the proposal to remove arbitrary number of memories is less so.

(*) the best behavior would probably be to not mess with contract memories at all, as I don't think theer's a fundamental reason why we need this imported memory at all, but that's very out of scope.

[Ekleog-NEAR]

That works for me too!

To sum up, the final state would be:

• contracts with zero or one internal memory "just work" (though it gets rewritten to be with our parameters)
• contracts with one imported memory do not compile (and it fails early instead of after preparation)

@nagisa Are you also OK with this plan?

[nagisa]

Yeah, keeping the current behaviour seems reasonable enough to me.

[Ekleog-NEAR]

Closing in favor of #8146