Skip to content
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.

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

Raise PayloadTooLarge error instead of InputTooLarge error in InputBox #224

Closed
ZzzzHui opened this issue Jan 25, 2024 · 6 comments
Closed
Assignees
Labels
A-contracts Area: contracts T-debt Type: debt T-feature Type: feature

Comments

@ZzzzHui
Copy link
Contributor

ZzzzHui commented Jan 25, 2024

📚 Context

Going forward, inputs are blockchain-agnostic blobs containing metadata and the input payload itself. If the blob is larger than the limit set by Cartesi Virtual Machine, contract InputBox raises an InputTooLarge error, with the input blob size and the limit, defined in the interface IInputBox.
However, this error can be confusing to the users as they may not know about the construction of the input blob under the hood, and it takes some figuring out and guess work to know how to adjust their payload argument to avoid the error.
The direct way would be to prompt the PayloadTooLarge error with the payload size and limit.

✔️ Solution

Replace InputTooLarge error with PayloadTooLarge.
May add a constant INPUT_PAYLOAD_MAX_SIZE in the common/CanonicalMachine library.

@ZzzzHui ZzzzHui added T-feature Type: feature T-debt Type: debt D-average A-contracts Area: contracts labels Jan 25, 2024
@ZzzzHui ZzzzHui added this to the 2.0.0 milestone Jan 25, 2024
@ZzzzHui ZzzzHui self-assigned this Jan 25, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Rollups Unit Jan 25, 2024
@guidanoli
Copy link
Collaborator

How would you calculate INPUT_PAYLOAD_MAX_SIZE based on INPUT_MAX_SIZE at compile time?

@ZzzzHui
Copy link
Contributor Author

ZzzzHui commented Jan 25, 2024

After this PR being merged, we add block.chainid and address(app) to the current structure of the input blob. So the input blob structure becomes:

Field # bytes
selector 4
block.chainid 32
address(app) 32
msg.sender 32
block.number 32
block.timestamp 32
index 32
offset 32
size 32
payload DYNAMIC

Therefore, the payload size limit is

INPUT_PAYLOAD_MAX_SIZE = 
                        (INPUT_MAX_SIZE - (4 + 32 * 8))
                                                        / 32 * 32

This is based on the encoding of EvmAdvance, which might change in the future. So the better place for this constant is probably in the InputBox contract rather than CanonicalMachine library. Because if the EvmAdvance encoding changes, the InputBox contract changes anyway.

What do you think?

@guidanoli
Copy link
Collaborator

Your thought process is solid, but the code that calculates INPUT_PAYLOAD_MAX_SIZE does not take the EvmAdvance function into consideration, which means that if someone changes the signature of this function but not this equation, they will be out-of-sync.

@ZzzzHui
Copy link
Contributor Author

ZzzzHui commented Jan 25, 2024

If someone changes EvmAdvance, they will update the contract InputBox as well, which is where the constant will be in.
If the equation is not updated properly, unit tests would catch that immediately.

@guidanoli
Copy link
Collaborator

Ideally, the constant should be updated automatically when the EvmAdvance function is updated, but I believe that is not currently possible with the Solidity compiler.

@guidanoli
Copy link
Collaborator

Having said that, I would keep things the way they are now.

@ZzzzHui ZzzzHui removed this from the 2.0.0 milestone Jan 26, 2024
@guidanoli guidanoli closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
@guidanoli guidanoli moved this from 📋 Backlog to 🧊 Icebox in Rollups Unit Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contracts Area: contracts T-debt Type: debt T-feature Type: feature
Projects
Status: 🧊 Icebox
Development

No branches or pull requests

2 participants