Implemented the limit per fee currency

[carterqw2]

Description

In the light of the recent growth of interest in extending of the gas currencies allowlist, we need to implement a protective mechanism that would allow validators more precisely control the percentage of available block space used paid with an alternative fee currency (other than CELO).

Goals

• Allow validators to limit the block space allocated for transactions paid with alternative fee currencies (with a per currency granularity if needed).
• Minimize the effort required for this feature:
  • We want people to use this feature more, not less, so the limiting should be permissive by default.
  • The allowlist is dynamic, it shouldn’t be required to change any configuration parameters when a new currency gets added to the list.

Proposed Solution

Add a flag celo.feecurrency.limits with a comma-separated currency address hash=limit mappings for currencies listed in the FeeCurrencyWhitelist contract, where limit represents the maximal fraction of the block gas limit as a float point number available for the given fee currency.

For example, --celo.feecurrency.limits=0x765DE816845861e75A25fCA122bb6898B8B1282a=0.1,0xD8763CBa276a3738E6DE85b4b3bF5FDed6D6cA73=0.05;0xEd6961928066D3238134933ee9cDD510Ff157a6e=0.

Add an overridable default value celo.feecurrency.default (initially set to 0.5 ) for currencies not listed in the map, meaning that if not specified otherwise, a TX with a given fee currency can take up to 50% of the block space.

CELO token doesn’t have a limit.

Default Configuration

Based on historical data we will use this default configuration:

--celo.feecurrency.default=0.5
--celo.feecurrency.limits=0x765DE816845861e75A25fCA122bb6898B8B1282a=0.9,0xD8763CBa276a3738E6DE85b4b3bF5FDed6D6cA73=0.5,0xe8537a3d056DA446677B9E9d6c5dB704EaAb4787=0.5

It imposes the following limits:

cUSD up to 90%
cEUR up to 50%
cREAL up to 50%
any other token except CELO - 50%

Other changes

No.

Tested

Added tests.

Backwards compatibility

Yes, both flags have default options. Behavior-wise, the effect should be similar to what we observe now.

It would be easier to review by following the commits.

[github-actions]

Test failures:
TestTransactionIndices: core blockchain_test.go:1946: Oldest indexded block mismatch, want 69, have 100
TestEthersJSCompatibility: e2e_test Checking getExchangeSpenders. spenders = [0x000000000000000000000000000000000000d028] Checking medianRate. numerator = 1000000000000000000000000 denominator = 1000000000000000000000000 Checking gas price minimum. cusdValue = 100000000 executing mocha test with /usr/bin/npm run --networkaddr=http://127.0.0.1:39703 test --blocknum=0x0 -- --grep ethers.js compatibility tests with state ethersjs-api-check@1.0.0 test /runner/_work/celo-blockchain/celo-blockchain/e2e_test/ethersjs-api-check mocha -r ts-node/register test/*.ts "--grep" "ethers.js compatibility tests with state" ethers.js compatibility tests with state ✔ provider.getBlock works (block has gasLimit set) ✔ EIP-1559 transactions supported (can get feeData) ✔ block has gasLimit 1) block has baseFeePerGas 3 passing (110ms) 1 failing ethers.js compatibility tests with state block has baseFeePerGas: AssertionError: expected false to be true expected - actual -false +true at /runner/_work/celo-blockchain/celo-blockchain/e2e_test/ethersjs-api-check/test/test.ts:52:10 at Generator.next () at fulfilled (test/test.ts:5:58) at processTicksAndRejections (internal/process/task_queues.js:95:5) npm ERR! code ELIFECYCLE npm ERR! errno 1 npm ERR! ethersjs-api-check@1.0.0 test: mocha -r ts-node/register test/*.ts "--grep" "ethers.js compatibility tests with state" npm ERR! Exit status 1 npm ERR! npm ERR! Failed at the ethersjs-api-check@1.0.0 test script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above. npm ERR! A complete log of this run can be found in: npm ERR! /home/runner/.npm/_logs/2023-12-07T15_07_08_800Z-debug.log e2e_test.go:685: </code></pre></td></tr>
TestEthersJSCompatibilityDisableAfterGingerbread: e2e_test Checking getExchangeSpenders. spenders = [0x000000000000000000000000000000000000d028] Checking medianRate. numerator = 1000000000000000000000000 denominator = 1000000000000000000000000 Checking gas price minimum. cusdValue = 100000000 e2e_test.go:722: e2e_test.go:722: e2e_test.go:722: e2e_test.go:722: e2e_test.go:722: e2e_test.go:724:
TestEthersJSCompatibilityDisableBeforeGingerbread: e2e_test Checking getExchangeSpenders. spenders = [0x000000000000000000000000000000000000d028] Checking medianRate. numerator = 1000000000000000000000000 denominator = 1000000000000000000000000 Checking gas price minimum. cusdValue = 100000000 e2e_test.go:770: Checking getExchangeSpenders. spenders = [0x000000000000000000000000000000000000d028] Checking medianRate. numerator = 1000000000000000000000000 denominator = 1000000000000000000000000 Checking gas price minimum. cusdValue = 100000000
This test report was produced by the test-summary action.  Made with ❤️ in Cambridge.

[codecov]

Codecov Report

Attention: 48 lines in your changes are missing coverage. Please review.

Comparison is base (6731538) 55.09% compared to head (347d8bc) 55.09%.

Files	Patch %	Lines
cmd/utils/flags.go	0.00%	25 Missing ⚠️
miner/block.go	44.82%	14 Missing and 2 partials ⚠️
core/sys_context.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2203   +/-   ##
=======================================
  Coverage   55.09%   55.09%           
=======================================
  Files         682      683    +1     
  Lines      114449   114533   +84     
=======================================
+ Hits        63055    63106   +51     
- Misses      47501    47536   +35     
+ Partials     3893     3891    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[palango]

The approach looks right to me, left one additional comment inline.

[github-actions]

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 99c7190

coverage: 46.9% of statements across all listed packages

coverage:  57.2% of statements in consensus/istanbul
coverage:  23.7% of statements in consensus/istanbul/announce
coverage:  54.3% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  65.1% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.2% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

[gastonponti]

I've added a few comments, one is something to change and the others are more opinionated about the defaults.
On the other hand, I've also checked what could happen in the case of having a validator accepting a proposal that whitelists a new currency, and in the same block sending a tx with that field, but we are not allowing that because we are checking against the whitelist that we have in the block context, which won't change until the next one, so we won't have that scenario.

[gastonponti]

LGTM, optional nit