LFM Phase-1 #30
LFM Phase-1 #30
Conversation
…nsfer before contract deployment
giladHaimov
requested review from
agustinorigone,
BarneyChambers,
denalimarsh,
RumeelHussain,
halbornteam,
RumeelHussainbnb,
corechainofficial,
eric0x01,
jackcrypto9527,
atasktos,
easonchan17,
kevin9936,
falcon2048 and
af-coredao
There was a problem hiding this comment.
Great work on this. Given the huge surface area of core-chain my review was not exhaustive - I mostly focused on the code you did change as opposed to the related code sections you chose not to modify. An auditor familiar with geth, such as Gokberk, is certainly needed here.
We are intentionally handling transactions originating from node terminals differently by not discounting them, as noted in Limitations: #4. Are there any implications of doing so that render certain node terminal operations obsolete? Does a differentiation of this type already exist between the two transaction categories (RPC, node terminal), or are we creating a novel differentiation?
consensus/satoshi/satoshi.go
Outdated
| @@ -269,7 +269,7 @@ func New( | |||
| candidateHubABI: cABI, | |||
| signer: types.NewEIP155Signer(chainConfig.ChainID), | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Remove leftover new line
| @@ -0,0 +1,318 @@ | |||
| package locaFeeMarket | |||
There was a problem hiding this comment.
| package locaFeeMarket | |
| package lfm |
- "local" is misspelled
- Package names in Golang should be lowercase, see here
Rename the package to either lfm or localfeemarket
| historicalMeanGasPrice = 35783571428 //wei | ||
| historicalSDGasPrice = 849870638 //wei | ||
| maxDiscountedGasPrice = historicalMeanGasPrice + historicalSDGasPrice/2 // =36208506747wei |
There was a problem hiding this comment.
Will we want to update historicalMeanGasPrice and historicalSDGasPrice as part of each future hardfork, with the values determined by the entire network history? Or will they be fixed and constant as of the introduction of lfm?
There was a problem hiding this comment.
{length warning followed by a general discussion of external network config}
One that I want to address here is regarding our ability to update the historical stats values and, in general, LFM data used by the new gas calc. model.
Indeed we can update these values on each hardfork point and this will allow is not to lag too much behind real world network stats, but there is a better way that will to update the LFM values in an ongoing manner without relying on hardforks.
To do this we will need to create a governance-managed smart contract that will contain said values, have its data periodically updates by the governance body, which may or may not be identcal to the genesis contract governance and basically make sure the stats used are always kept up-to date.
The problem with this approach is that content of this governance contract will need to be periodically read and agreed upon by all network nodes, which is tricky to achieve.
The simplest way to go would be to read the latest data upon each tx processing but that would impose a significant performance cost that we probably will not want to pay. remember that a 3-second block may contains thousands of txs.
The other route would be to cache and periodically refresh the contarct data on each occurrence ('period') agreed upon by all nodes say each block start or epoch or (block_number%1000 == 0) or something.
This approach will most likely reduce the performance impact to an acceptable value, but will introduce other problems
Data context: data regarding block number, current epoch etc. will need to be propagated to the LFM function which may result in additional code complexity
Node synch: since it typically takes some time for all nodes to agree on the current block number, we could run into data mismatch between different network validators
Solving these problems is a major step in moving towards the next steps of LFM.
I'm currently investigating several ways for solving these problems. Regardless, I prefer to keep the current, simpler, approach for the 1st delivery (KISS).
Once we have a solution that allows for such governance-managed contract update we will need to run extensive tests to verify that performance impact is acceptable and that no node synch problem has been introduced.
Note that such a solution will also be a solid baseline for the network subsidy-fund (aka LFM/v2) which, in fact, relies on such a contract for fund data synchronization.
| ) | ||
|
|
||
|
|
||
| func AdjustGasPriceForEstimation(_networkGasPrice *hexutil.Big, _gasAmount *hexutil.Uint64, _value *hexutil.Big, to *common.Address, data []byte) *hexutil.Big { |
There was a problem hiding this comment.
Why are some function arguments prefixed with _ here?
There was a problem hiding this comment.
to allow using non-underscored names inside function
| } | ||
|
|
||
| func AdjustGasPrice(gasAmount uint64, value *big.Int, to *common.Address, data []byte, networkGasPrice *big.Int) *big.Int { | ||
| // apply gas-price discount if tx is a 'native' transfer of either Core or whitelisted erc20 |
There was a problem hiding this comment.
| // apply gas-price discount if tx is a 'native' transfer of either Core or whitelisted erc20 | |
| // apply gas-price discount if tx is a 'native' transfer of either CORE or whitelisted erc20 |
| */ | ||
| func internalCalcDiscountedGasPrice(networkGasPrice *big.Int) *big.Int { | ||
| networkIntPrice := int(networkGasPrice.Uint64()) | ||
| MEAN := historicalMeanGasPrice |
There was a problem hiding this comment.
Technically an unnecessary var assignment here, although I'm okay with it. Guess the syntax looks cool
| return internalCalcDiscountedGasPrice(networkGasPrice) | ||
| } | ||
|
|
||
| func obsolete__internalCalcDiscountedGasPrice(networkGasPrice *big.Int) *big.Int { |
There was a problem hiding this comment.
obsolete__internalCalcDiscountedGasPrice has 0 references, what is the justification for not removing it? If it's just here for documentation purposes let's move it to a dedicated testing or documentation file. If this information is important enough to be colocated with the actual internalCalcDiscountedGasPrice function then we should condense the information and add it as inline comments to the internalCalcDiscountedGasPrice function.
There was a problem hiding this comment.
yep I left is for reviewers to diff. now removed
|
|
||
| errNotFunctionInvocation = errors.New("tx is not a function invocation") | ||
|
|
||
| //------- |
| coreTransferGasAmount = 21000 // hard-coded gas amount | ||
|
|
||
| functionSelectorSize = 4 | ||
| transferDataLen = 4+32+32 // transfer(address,uint256): 4 bytes for func ID, 32 bytes for address (20 bytes padded to 32), 32 bytes for amount |
There was a problem hiding this comment.
Technically this approach can be extrapolated to any contract address/function signature, correct? For example, we could make staking CORE on the Staking.sol contract a fee-reduced transaction type
There was a problem hiding this comment.
If it's possible we'll probably end up going in that direction for some key core-genesis-contracts actions. Which begs the question if we should already rearchitect the list of ERC20 token addresses + list of function signatures into a 1-to-1 structure such that each fee-reduced contract is a struct containing:
- an address
- a list of valid function signatures, where each signature has a data length
The above struct is probably off by a bit, but you get the idea. Thoughts?
There was a problem hiding this comment.
Technically this approach can be extrapolated to any contract address/function signature, correct? For example, we could make staking CORE on the
Staking.solcontract a fee-reduced transaction type
correct and part of our design goals. we want this mechanism to be easily extendable.
There was a problem hiding this comment.
If it's possible we'll probably end up going in that direction for some key
core-genesis-contractsactions. Which begs the question if we should already rearchitect the list of ERC20 token addresses + list of function signatures into a 1-to-1 structure such that each fee-reduced contract is a struct containing:
- an address
- a list of valid function signatures, where each signature has a data length
The above struct is probably off by a bit, but you get the idea. Thoughts?
see response above as to the erc20-compliant contract address. I would not add an additional 'discounted-functions' column due to KISS as well as having no immediate value
| @@ -332,6 +347,10 @@ func (tx *Transaction) GasTipCapCmp(other *Transaction) int { | |||
|
|
|||
| // GasTipCapIntCmp compares the gasTipCap of the transaction against the given gasTipCap. | |||
| func (tx *Transaction) GasTipCapIntCmp(other *big.Int) int { | |||
| legacyTx, isLegacy := tx.inner.(*LegacyTx) //@lfm | |||
| if isLegacy && legacyTx.origGasPrice() != nil { | |||
| return legacyTx.origGasPrice().Cmp(other) // tx gas price might have been reduced by the lfm module | |||
N
correct, that'sconcise decision made at the beginning due to reasons of complexity and value. note that there's no attack angle here |
…) each with its own gas-factor and with network configuration contract
Local-Fee Market phase1: Native-token discount
Design-doc
Target
Provide gas-price discount for native-token transfer txs. his discount will only be applied for EOA targeted transfer so to avoid discounting potentially complex receive() logic at the destination contract.
The statistical calculation of the discount is provided at locaFeeMarket.AdjustGasPrice and relies on the historical mean and standard-deviation Core values calculated over the last year.
Interleaving into main tx flow
In order to do this two major problems needed to be addressed after the discount has been introduced into the tx:
To handle both these issues the original gas price needed to be accessible and these points of the calculation so to avoid these problems. Since this price is determined per-tx, a new field - OrigGasPrice - has been added to the internal transaction struct and is initialized right before discount introduction for future processing.
It should be noted that this field is geth-internal and is not visible to users via the tx receipt or any other mean.
Account-Abstraction support
Since this feature was designed to only work between EOAs and since ERC-4337 always relies on smart-contract logic rather than private-key, no discount will be provided for AA accounts. While I could thinks of ways around this limitation, mostly involving creating some sort of detection-mechanism for paymasters, these will involve an amount of complexion we're currently better off without.
Valid native-transfer detection
The following conditions are used to detect a native-transfer tx which is applicable for discount:
It should be noted that condition #3 also allows us to omit txs issued from a contract's constructor, where data size is still zero due to the contract not yet being deployed.
Limitations
Executed tests
Only (1) and (2) have resulted in a discount, as should be.