cartesi / rollups

Cartesi Rollups
30 stars 12 forks source link

Default value of max input size should match Cartesi Machine default value #98

Closed tuler closed 1 year ago

tuler commented 1 year ago

🙂 Expected behavior

The default value of maximum input size in the onchain code should match the default value of maximum input size in the Cartesi-machine CLI.

The value in the CLI is 2Mb, as set by the --rollups CLI option:

--rollup
    defines appropriate values for rollup-rx-buffer, rollup-tx-buffer,
    rollup-input-metadata, rollup-voucher-hashes, rollup-notice hashes,
    and htif yield for use with rollups.
    equivalent to the following options:

    --rollup-rx-buffer=start:0x60000000,length:2<<20
    --rollup-tx-buffer=start:0x60200000,length:2<<20
    --rollup-input-metadata=start:0x60400000,length:4096
    --rollup-voucher-hashes=start:0x60600000,length:2<<20
    --rollup-notice-hashes=start:0x60800000,length:2<<20
    --htif-yield-manual
    --htif-yield-automatic

Note the length of rollup-rx-buffer is 2Mb (2<<20).

🫠 Actual behavior

Default max input size in the onchain code is set to 32Mb, as the INPUT_MAX_LOG2_SIZE is set to 25.

✔️ Possible solutions

Change the onchain value to Log2Size.wrap(21). Also add comments saying that changing those values highly depend on the configuration of the machine. Check if there is a test case for that limit, and add one if there is not.

guidanoli commented 1 year ago

I'm wondering whether we should set a limit to the input size on-chain at all, since it was meant to serve all DApps, whose off-chain machines can have wildly different configurations. If we establish such a limit, then wouldn't we be restricting the possible machine configurations that would work with a given Input Box?

tuler commented 1 year ago

Yes, we would. What is the alternative?

guidanoli commented 1 year ago

What is the alternative?

Restrict the input size at the off-chain layer. If an input is too large to fit inside the machine, we simply skip it? One weakness of this approach, however, is that the input won't even be processed, and so if the input is associated with any asset deposit, it would be locked... :-(

tuler commented 1 year ago

We will also change the behavior in the server-manager to skip large inputs, as you propose.

As of not having any onchain limits at all, it's debatable. cc @diegonehab

gligneul commented 1 year ago

The default value of maximum input size in the onchain code should match the default value of maximum input size in the Cartesi-machine CLI.

Why not increase the value of the input in the cartesi machine?

tuler commented 1 year ago

Why not increase the value of the input in the cartesi machine?

Increasing the value in the machine input will require to change all memory ranges, because they are all tightly aligned.

--rollup-rx-buffer=start:0x60000000,length:2<<20
--rollup-tx-buffer=start:0x60200000,length:2<<20
--rollup-input-metadata=start:0x60400000,length:4096
--rollup-voucher-hashes=start:0x60600000,length:2<<20
--rollup-notice-hashes=start:0x60800000,length:2<<20

Why not decrease the onchain value to 2Mb?

We need a limit. I don't know if we need to leave this limit as a configuration per DApp, or a system-wide configuration.

gligneul commented 1 year ago

Why not decrease the onchain value to 2Mb?

I don't have strong opinions. 2MB per input seems enough anyway.

I don't know if we need to leave this limit as a configuration per DApp, or a system-wide configuration.

At this point, I'm in favor of using a single limit to keep things simpler. Especially if we set a high enough limit that nobody is really going to reach.

guidanoli commented 1 year ago

A partial temporary solution is to have several Input Boxes, one with each limit. So, if you want your DApp to work with inputs larger than 2 MB, you can use the Input Box that accepts inputs of 5 MB, or 10 MB, or 1 GB. This allows DApps to choose their maximum input size, and avoids assets getting locked in the DApp contract.

This would, of course, not be compatible with the idea of a Universal Input Box we envision for the future. In this scenario, we'd be forced to either ask each DApp what is its maximum input size, or apply a common limit.

Having said that, I agree with changing the on-chain limit to 2 MB.

diegonehab commented 1 year ago

Just subtle issue. The input that is written to the machine RX buffer (2MB max) is ABI encoded. Therefore, it includes an offset and length fields. So the maximum payload size is 64 bytes fewer than 2MB. So, what is the meaning of INPUT_MAX_LOG2_SIZE? Is it the payload exclusively or does it include the offset and length fields? It has to include them. Does that make sense? @tuler @guidanoli

guidanoli commented 1 year ago

Does that make sense?

Yes, this makes sense. I will make sure this will be accounted for in the on-chain code.

diegonehab commented 1 year ago

See server manager issue 4.

guidanoli commented 1 year ago

@diegonehab Question: When an input is ABI-encoded and written to the machine RX buffer, does the encoding also pad the input with zeros so that its size is a multiple of 32 bytes?

guidanoli commented 1 year ago

Oops, didn't mean to close this issue.

guidanoli commented 1 year ago

The InputBox now only accepts inputs with size less than or equal to $2^{21} - 64$ bytes.

guidanoli commented 1 year ago

I believe this issue has been addressed by #104