search

cartesi / rollups

Cartesi Rollups
30 stars 12 forks source link

Encode inputs with strict mode #140

Closed guidanoli closed 1 year ago

guidanoli commented 1 year ago

Closes cartesi/rollups-contracts#46.

guidanoli commented 1 year ago

As requested by @tuler, I will do some benchmarks to see the differences in gas costs caused by this change.

guidanoli commented 1 year ago

Benchmark :test_tube:

I've encountered some interesting results! First, the raw data:

Warning: these numbers are highly influenced by our test cases, specially for inputs with variable-length fields. To take into account the randomness of the fuzz tests, we ran all tests with the fuzz seed equal to 1.

Contract Function Min Max Avg
DAppAddressRelay 1800 (1.36%)
DAppAddressRelay relayDAppAddress 42 (0.06%) 42 (0.06%) 42 (0.06%)
EtherPortal 4206 (2.21%)
EtherPortal depositEther 796 (0.75%) 531 (0.75%)
ERC20Portal 2600 (1.20%)
ERC20Portal depositERC20Tokens 908 (1.07%) 590 (1.00%)
ERC721Portal -15212 (-5.95%)
ERC721Portal depositERC721Token -439 (-0.47%) -263 (-0.37%)
ERC1155SinglePortal -17212 (-6.52%)
ERC1155SinglePortal depositSingleERC1155Token -426 (-0.47%) -213 (-0.33%)
ERC1155BatchPortal -14613 (-4.66%)
ERC1155BatchPortal depositBatchERC1155Token -7658 (-0.31%)

Now, some analysis. Notice how some input relays encountered an increase in deployment and average deposit cost, but some encountered a decrease.

Increased gas cost :arrow_up: :fuelpump:

Why some increased

As for those input relays that have 0-1 variable-length fields, abi.encode has only increased the size of the input, which makes both the function and the deployment cost more.

Decreased gas cost :arrow_down: :fuelpump:

Why some decreased

Note that all that decreased had 2+ variable-length field encoded in the input. This has forced us to use abi.encode to avoid ambiguity. As for the rest of the fields, we still used abi.encodePacked as not to add any unnecessary padding. It seems that the cost of an extra abi.encodePacked was greater than the cost of encoding everything with a single abi.encode.

For example, take a look at the ERC-721 input encoding function before and after the change, respectively.

https://github.com/cartesi/rollups/blob/15e6c167814c6bf8dd46a2bf904845d9bdd82d85/onchain/rollups/contracts/common/InputEncoding.sol#L81-L88

https://github.com/cartesi/rollups/blob/e0b626f568decc071d70c12d1eedf2b2c391a6c9/onchain/rollups/contracts/common/InputEncoding.sol#L69

tuler commented 1 year ago

Is this for 1.0 (main) or next?

This breaks the examples that use the portal, right?

guidanoli commented 1 year ago

Is this for 1.0 (main) or next?

This is a breaking change, since it changes the encoding of inputs. We'll leave this to another upcoming release.

guidanoli commented 1 year ago

Rebased.

guidanoli commented 1 year ago

This PR has been migrated to https://github.com/cartesi/rollups-contracts/pull/47.