Skip to content
This repository has been archived by the owner on Apr 27, 2022. It is now read-only.

Implement gas refunds as an interaction. #445

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Feb 15, 2021

Closes #442

The idea is to make gas refunds be encoded as interactions instead of being part of the settlement parameters.

Gas Cost Analysis

Native Order Refunds

image

Interaction Based Order Refunds

image

Results:

So, as @fleupold mentioned, it would be smarter to accumulate refunds and then use them all at once. This reduces the overall gas cost per order.

In the case of native order refunds, the optimal "refund batch" size to use is 7:

  • The "cold start" gas cost per order is 181490 (this is the average cost per order when doing 7 settlements without refunds, and then an 8th settlement with 7 refunds)
  • The "continuous" gas cost per order is 180467 (this is the average cost per order when doing 6 settlements without refunds, then a 7th settlement with 7 refunds)

In the case of interaction based order refunds, the optimal "refund batch" size is 8:

  • The "cold start" gas cost per order is 181237
  • The "continuous" gas cost per order is 180484 (only 17 gas more)

This means that the gas cost per order is almost the same 🎉. One other bonus of keeping order refunds as interactions, is that if, for whatever reason, storage refunds get removed from the EVM, we don't pay additional costs per settlement for a dead feature if we use interaction based storage refunds.

Full Settlements

The full settlement benchmarks show a similar story, where settlements without gas costs become cheaper, and settlements without become more expensive.

Benchmarks
=== Settlement Gas Benchmarks ===
--------------+--------------+--------------+--------------+--------------
       tokens |       trades | interactions |      refunds |          gas 
--------------+--------------+--------------+--------------+--------------
            2 |           10 |            0 |            0 |       752558  (change: -137 / trade)
            3 |           10 |            0 |            0 |       753102  (change: -136 / trade)
            4 |           10 |            0 |            0 |       762034  (change: -139 / trade)
            5 |           10 |            0 |            0 |       766766  (change: -137 / trade)
            6 |           10 |            0 |            0 |       763086  (change: -139 / trade)
            7 |           10 |            0 |            0 |       772018  (change: -134 / trade)
            8 |           10 |            0 |            0 |       776690  (change: -136 / trade)
            8 |           20 |            0 |            0 |      1463360  (change: -72 / trade)
            8 |           30 |            0 |            0 |      2111933  (change: -47 / trade)
            8 |           40 |            0 |            0 |      2764685  (change: -33 / trade)
            8 |           50 |            0 |            0 |      3418082  (change: -28 / trade)
            8 |           60 |            0 |            0 |      4067559  (change: -24 / trade)
            8 |           70 |            0 |            0 |      4721274  (change: -19 / trade)
            8 |           80 |            0 |            0 |      5374021  (change: -18 / trade)
            2 |           10 |            1 |            0 |       823946  (change: -137 / trade)
            2 |           10 |            2 |            0 |       870598  (change: -142 / trade)
            2 |           10 |            3 |            0 |       917286  (change: -133 / trade)
            2 |           10 |            4 |            0 |       963926  (change: -135 / trade)
            2 |           10 |            5 |            0 |      1010614  (change: -136 / trade)
            2 |           10 |            6 |            0 |      1057230  (change: -135 / trade)
            2 |           10 |            7 |            0 |      1103870  (change: -139 / trade)
            2 |           10 |            8 |            0 |      1150498  (change: -142 / trade)
            2 |           50 |            0 |           10 |      3126161  (change: 196 / trade)
            2 |           50 |            0 |           15 |      3085349  (change: 197 / trade)
            2 |           50 |            0 |           20 |      3044429  (change: 200 / trade)
            2 |           50 |            0 |           25 |      3003485  (change: 197 / trade)
            2 |           50 |            0 |           30 |      2962745  (change: 202 / trade)
            2 |           50 |            0 |           35 |      2921813  (change: 203 / trade)
            2 |           50 |            0 |           40 |      2880953  (change: 204 / trade)
            2 |           50 |            0 |           45 |      2839997  (change: 202 / trade)
            2 |           50 |            0 |           50 |      2799233  (change: 207 / trade)
            2 |            2 |            0 |            0 |       186781  (change: -683 / trade)
            2 |            1 |            1 |            0 |       187309  (change: -1378 / trade)
           10 |          100 |           10 |           20 |      6618975  (change: 100 / trade)

Test Plan

CI, added new unit tests for interaction-only methods. Coverage stays at 100%.

@nlordell
Copy link
Contributor Author

nlordell commented Feb 15, 2021

It also looks like the maximum number of interactions we can use is ~7:
image

This implies that the smallest overhead per refund is ~700 gas

Here are the same benchmarks applied to PR #441

image

@nlordell nlordell requested a review from a team February 15, 2021 08:14
@nlordell
Copy link
Contributor Author

One big advantage of using interactions is that all settlement paths can use refunds, and don't pay for them if they aren't used. The 5K gas overhead is quite steep though.

@nlordell nlordell force-pushed the settle-single-trade branch from 6b73933 to b78baa2 Compare February 15, 2021 08:32
Base automatically changed from settle-single-trade to main February 15, 2021 08:35
@nlordell nlordell force-pushed the refund-as-interaction branch 2 times, most recently from 3eaeb48 to d01704f Compare February 15, 2021 09:37
@fedgiac
Copy link
Contributor

fedgiac commented Feb 15, 2021

This means that from a gas refund of 15k gas we only get back 2.5k. I was expecting a bit less overhead for a CALL.

@fleupold
Copy link
Contributor

This means that from a gas refund of 15k gas we only get back 2.5k.

I think the refund is technically only 10k as we pay 5k to set the value to 0. I agree that losing 5k here seems a bit much.

Can we, with all these measurements, simulate the avg. cost following scenarios:

  • Fast path without refund + default path with refund every 6 or 7 trades (to free 6 or 7 previous orders)
  • Fast path with refund for each trade

Then we can decide if returns should be part of the fast path (natively)

@nlordell
Copy link
Contributor Author

nlordell commented Feb 26, 2021

edit: Updated PR deciption with analysis.

I think we should merge this change 🚀

@nlordell nlordell force-pushed the refund-as-interaction branch from d01704f to a6ac8be Compare February 26, 2021 10:50
@nlordell nlordell marked this pull request as ready for review February 26, 2021 11:20
@nlordell nlordell mentioned this pull request Feb 26, 2021
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analysis makes sense and the code looks good.

The current gas benchmark looks very misleading with 200gas/refund more, I wonder if the 6/7 argument could be in some way included in the benchmark.

Comment on lines +300 to +301
["freeFilledAmountStorage", filledAmounts] as const,
["freePreSignatureStorage", preSignatures] as const,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the as const here for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as const makes it so the type is [string, BytesLike[]] instead of (string | BytesLike[])[]. The as const is necessary for tuple type inference.

Comment on lines +414 to 417
* @param settlement The address of the settlement contract.
* @param orderRefunds The order refunds to encode.
*/
public encodeOrderRefunds(orderRefunds: Partial<OrderRefunds>): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new line of documentation is not needed.

@bh2smith
Copy link
Contributor

May not be worth it since these refunds will be obsolete sooner than we think

ethereum/EIPs#3298

@nlordell
Copy link
Contributor Author

nlordell commented Mar 1, 2021

May not be worth it since these refunds will be obsolete sooner than we think

I think that makes this change more worth it, as currently we pay a fixed overhead on every settlement even if we don't use refunds. This change makes it so we pay a larger overhead for gas refunds, but only if we make use of them, (and pay less gas when a settlement is made without any refunds).

@nlordell
Copy link
Contributor Author

nlordell commented Mar 1, 2021

The current gas benchmark looks very misleading with 200gas/refund more, I wonder if the 6/7 argument could be in some way included in the benchmark.

Hmm, I'm not sure I understand exactly what you mean.

@fedgiac
Copy link
Contributor

fedgiac commented Mar 1, 2021

The current gas benchmark looks very misleading with 200gas/refund more, I wonder if the 6/7 argument could be in some way included in the benchmark.

Hmm, I'm not sure I understand exactly what you mean.

This:

The "continuous" gas cost per order is 180467[...]
In the case of interaction based order refunds [...] the "continuous" gas cost per order is 180484 (only 17 gas more)

and this:

            2 |           50 |            0 |           10 |      3126161  (change: 196 / trade)
            2 |           50 |            0 |           15 |      3085349  (change: 197 / trade)
            2 |           50 |            0 |           20 |      3044429  (change: 200 / trade)
            2 |           50 |            0 |           25 |      3003485  (change: 197 / trade)
            2 |           50 |            0 |           30 |      2962745  (change: 202 / trade)
            2 |           50 |            0 |           35 |      2921813  (change: 203 / trade)
            2 |           50 |            0 |           40 |      2880953  (change: 204 / trade)
            2 |           50 |            0 |           45 |      2839997  (change: 202 / trade)

look contradictory to me at first glance. My understanding is that the first reasoning is right but it's not captured by the gas benchmark, in the sense that the benchmark measures something else we are not interested in.

@nlordell
Copy link
Contributor Author

nlordell commented Mar 1, 2021

look contradictory to me at first glance. My understanding is that the first reasoning is right but it's not captured by the gas benchmark, in the sense that the benchmark measures something else we are not interested in.

Ah, I see. The first benchmark is the gas/order for single order settlements, the benchmark is for 50 order settlements. To me this indicates a flat additional cost of 200*50=10k for doing refunds.

The original purpose of this benchmark was to measure that the gas cost decreases linearly with order refunds (which it mostly does). We can add additional benchmarks for computing optimal "continuous gas/order" costs.

edit: Addressed in #471

@nlordell
Copy link
Contributor Author

nlordell commented Mar 2, 2021

cc @fleupold

Are you OK with merging this?

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @fleupold
Are you OK with merging this?

@nlordell nlordell added the merge when green Merge when green! label Mar 2, 2021
@mergify mergify bot merged commit 7b23110 into main Mar 2, 2021
@mergify mergify bot deleted the refund-as-interaction branch March 2, 2021 10:47
mergify bot pushed a commit that referenced this pull request Mar 2, 2021
This PR modifies the `single` settlement benchmarks to run multiple settlements and simulate batching refunds for optimal order gas refund results.

/cc @fedgiac Does this address your concerns from #445 (comment)

### Test Plan

`yarn bench:single`

```
=== Single Order Gas Benchmarks ===
--------------+--------------+--------------+--------------
  settlements |      refunds |     gasToken |    gas/order 
--------------+--------------+--------------+--------------
            1 |            0 |            0 |       187285 
            1 |            1 |            0 |       184666 
            8 |            8 |            0 |       180509 
            1 |            4 |            2 |       148431 
            1 |            0 |            5 |       121983
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge when green Merge when green!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider allowing storage refunds to be triggered as Interactions
4 participants