feat: remove MaxPrecompileCalls

[Pitasi]

Description

Closes: #135

This should not have a huge impact, after #244.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• tackled an existing issue or discussed with a team member
• left instructions on how to review the changes
• targeted the main branch

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

• added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• confirmed all author checklist items have been addressed
• confirmed that this PR does not change production code
• reviewed content
• tested instructions (if applicable)
• confirmed all CI checks have passed

[vladjdk]

Would be helpful to have some sort of benchmarking done to see what a reasonable limit here would be. Do you have any immediate reason to lift it now?

[Pitasi]

Would be helpful to have some sort of benchmarking done to see what a reasonable limit here would be.

yeah absolutely, do you already have some rough ideas on how to benchmark it? do i just run a contract function that invokes a precompile N times and monitor memory usage?

Do you have any immediate reason to lift it now?

not really, i used to in the past, but i ended up rewriting my precompile in a way that allows to batch operations

[vladjdk]

do i just run a contract function that invokes a precompile N times and monitor memory usage?

This sounds pretty reasonable. A graph that shows memory increasing over N precompile calls would be pretty useful to have.

[Pitasi]

Hey @vladjdk, here's what I did (a bit cumbersome and "manual" but it was the quickest way I found, I'm not very familiar with all the codebase):

• I added a "foo()" method to the staking precompile that does nothing:
  • https://github.com/Pitasi/evm/blob/benchmark-precompile-memory/precompiles/staking/StakingI.sol#L298
  • https://github.com/Pitasi/evm/blob/benchmark-precompile-memory/precompiles/staking/tx.go#L34-L45
• changed StakingReverter.sol to call foo():
  • https://github.com/Pitasi/evm/blob/benchmark-precompile-memory/precompiles/testutil/contracts/StakingReverter.sol#L29
• Changed the existing integration test with different precompile runs counter (0/7/100):
  • https://github.com/Pitasi/evm/blob/benchmark-precompile-memory/tests/integration/precompiles/staking/test_integration.go#L1816
  • and changed the test to expect the tx to pass: https://github.com/Pitasi/evm/blob/benchmark-precompile-memory/tests/integration/precompiles/staking/test_integration.go#L1833

I run the test 3 times to average out the memory usages, using pprof, like this:

go test -count=1 -run TestStakingPrecompileIntegrationTestSuite -v ./tests/integration/precompiles/staking/... -ginkgo.focus "should revert the changes and NOT delegate - failed tx - max precompile calls reached" -memprofile /tmp/mem.out

and here are the results I collected:

0 precompile calls
Run 1) Showing nodes accounting for 54.66MB, 50.54% of 108.17MB total
Run 2) Showing nodes accounting for 43.19MB, 46.89% of 92.12MB total
Run 3) Showing nodes accounting for 53.11MB, 54.91% of 96.73MB total
avg: 99 MB

7 precompile calls
Run 1) Showing nodes accounting for 50.05MB, 44.36% of 112.83MB total
Run 2) Showing nodes accounting for 46.59MB, 42.38% of 109.93MB total
Run 3) Showing nodes accounting for 57.58MB, 48.56% of 118.57MB total
avg: 113.77 MB
without baseline: 113.77 - 99 = 14.77 MB
per precompile call:  14.77 / 7 = 2.11 MB

20 precompile calls
Run 1) Showing nodes accounting for 55.55MB, 45.60% of 121.83MB total
Run 2) Showing nodes accounting for 48.19MB, 40.66% of 118.52MB total
Run 3) Showing nodes accounting for 58.15MB, 46.31% of 125.57MB total
avg: 121.97 MB
without baseline: 121.97 - 99 = 22.97 MB
per precompile call: 22.97 / 20 = 1.14 MB

50 precompile calls
Run 1) Showing nodes accounting for 48.17MB, 40.07% of 120.21MB total
Run 2) Showing nodes accounting for 54.66MB, 43.47% of 125.74MB total
Run 3) Showing nodes accounting for 54.54MB, 42.98% of 126.89MB total
avg: 124.28 MB
without baseline: 124.28 - 99 = 25.98 MB
per precompile call: 253.18/100 = 0.52 MB

75 precompile calls
Run 1) Showing nodes accounting for 51.55MB, 38.43% of 134.11MB total
Run 2) Showing nodes accounting for 59.53MB, 41.82% of 142.33MB total
Run 3) Showing nodes accounting for 51.59MB, 36.29% of 142.16MB total
avg: 139.53 MB
without baseline: 139.53 - 99 = 40.53 MB
per precompile call: 40.53 / 100 = 0.41 MB

100 precompile calls
Run 1) Showing nodes accounting for 56.08MB, 34.88% of 160.80MB total
Run 2) Showing nodes accounting for 59.04MB, 36.25% of 162.88MB total
Run 3) Showing nodes accounting for 70.03MB, 43.22% of 162.03MB total
avg: 161.90 MB
without baseline: 161.90 - 99 = 62.90 MB
per precompile call: 62.90 / 100 = 0.63 MB

I couldn't do more that 100 precompile calls because it would exceed the gas allowance of 250.000 when estimating gas fees.

The interesting thing I see is that the more precompile calls the less memory they use (at least after the initial 20-50 calls), then the cost becomes fixed. In my opinion the gas cost already prevents abuse so a limit is not really needed, but we could leave it to a high number (e.g. 100).

Is that enough data for you to take a decision?

[aljo242]

@Pitasi conflicts right now

[Pitasi]

@Pitasi conflicts right now

resolved

[Pitasi]

btw fyi the test i removed "should revert the changes and NOT delegate - failed tx - max precompile calls reached" was not working anyway because the first staking call always reverts :)

so the test was expecting a revert after 7 calls, but it actually is receiving a revert after the first one

[vladjdk]

@Pitasi we're thinking of merging this in to enable the more complex precompile reversion test cases. Few questions:

1. Could you upstream the benchmarking that you did?
2. Could you increase the gas limit to the max block gas (i.e. 10M-100M)
3. Could you post benchmarking results from that here?

I'm debating whether we should increase the limit or outright remove it.

[aljo242]

we are going to hold off on merging this until we have more concrete numbers validating that this is safe from a liveness and performance perspective

see comment

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs.