apply policy check to txpool and block verification #6
Conversation
|
Is |
Yes, it is a policy of the lowest gas price that a transaction sender should provide. And this policy will be added to block verification as well in the future, when Bane's DBFT is built up. |
|
merge? |
|
@txhsl, we have a dedicated issue for PrepareRequest verification, could you please write a couple of words about minGasPrice requirement to #53? This constraint then will likely be implemented as a part of PrepareRequest verification. Or at list it won't hang in the air and we'll transfer it to a separate issue if needed. |
|
Can we move on? Now we have,
What the newest fee rules? |
As far as I remember, we may going to add the I'm free to do it now, but some research is necessary as well, especially when the |
|
Sure. |
txhsl
changed the title
|
@chenquanyu Need your review about policy check in block verification, related to #138. |
| func (b *EthAPIBackend) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { | ||
| return b.gpo.SuggestTipCap(ctx) | ||
| suggestTipCap, err := b.gpo.SuggestTipCap(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| stateDb, err := b.eth.BlockChain().StateAt(b.CurrentHeader().Root) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| minGasTipCap := stateDb.GetState(systemcontracts.PolicyProxyHash, systemcontracts.GetMinGasTipCapStateHash()).Big() | ||
| return cmath.BigMax(suggestTipCap, minGasTipCap), nil |
There was a problem hiding this comment.
I know it's necessary but this change makes the value of eth_gasPrice inaccurate, e.g.
When baseFee is 7 wei, miner.gasPrice is 1 Gwei(just like in T2), policy_minGasTipCap is 1.000000005 Gwei, eth_gasPrice will return policy_minGasTipCap+baseFee which is 1.000000012 Gwei as suggested gasPrice, while 1.000000007 Gwei is enough.
The root cause is, miners still rely on their own miner_gasPrice setting and network baseFee to decide whether to package a tx.
There was a problem hiding this comment.
The policy check in mempool and consensus will filter them out, but yes, I'm worried about the capability with eth_feehistory and related tools e.g. Metamask. In fact, it seems not working well.
@chenquanyu is going to set fixed basefee for burning, then only gastip can be shared as governance reward. If we don't use an "inaccurate" price, then governance reward can be always 0 with empty blocks.
There was a problem hiding this comment.
@chenquanyu is going to set fixed basefee for burning
0.0 What do you mean by set fixed basefee for burning? Burning block.basefee like ethereum or burning a fixed value?
I'm worried about the capability with eth_feehistory and related tools e.g. Metamask.
What's wrong with eth_feehistory? I see it just returns baseFeePerGas and gasUsedRatio, and seems OK.
If we don't use an "inaccurate" price, then governance reward can be always 0 with empty blocks.
Why? Empty blocks means no tx, no tx so no governance reward, it's OK. If you mean that empty blocks makes baseFee drop to the lowest value nearly 0, we still have miner settings to ensure governance reward.
But anyway, policy check prevent malicious txs with low fees from entering the pool. So by the way, I wonder if we still need --txpool.nolocals for t3 seeds, because node doesn't store transactions.rlp with this flag, txs in the txpool will be lost if the node restarts.
There was a problem hiding this comment.
Burning
block.basefeelike ethereum or burning a fixed value?
Still burn block.basefee, but maybe not based on the same rule.
What's wrong with
eth_feehistory?
eth_feehistory works well. But I tested it with Metamask, which set 1.5 Gwei as default when 5 blocks in feehistory are all empty, so when I improved minGasTipCap above that, this default setting got RPC errors.
There was a problem hiding this comment.
eth_feehistoryworks well. But I tested it with Metamask, which set 1.5 Gwei as default when 5 blocks infeehistoryare all empty, so when I improvedminGasTipCapabove that, this default setting got RPC errors.
What RPC error have you encountered?
I can set the feeCap and tipCap to the lowest value, and the tx succeed.
Even can set them lower than the network required. Although Metamask shows warning, tx can be sent to node txpool successfully, and whether it's packaged or not is a matter of miner.
I think the issue is that Metamask has its own way of calculating fees, users can't get appropriate fee value.
chenquanyu
force-pushed
the
policy-txpool
branch
from
21fce63 to
3128d31
Compare
| } | ||
| // Ensure the transaction is allowed by policy | ||
| // Apply policy minimum gas tip cap | ||
| var minGasTipCap = st.state.GetState(systemcontracts.PolicyProxyHash, systemcontracts.GetMinGasTipCapStateHash()) |
There was a problem hiding this comment.
I have a question here. As this will fetch the latest state of the chain. Does this affect transactions that follows after a MinGasTip-changing tx that appeared in the same block?
There was a problem hiding this comment.
It will, so I think you mean, it will lead to a case that some transactions accepted by txpool will be rejected by consensus.
There was a problem hiding this comment.
Check #166 if necessary, there is the latest code.
Close #4.
This PR is going to add a policy check to Geth transaction pool. Whenever there is a new transaction trying to be added into
legacypoolorblobpool, the methodValidateTransactionWithStatewill read aminGasPriceand ablockedstatus from current blockchain state database. The method will not use EVM but compute the index key and access the contract storage directly.The
PolicyAddressis going to be a field ofChainConfig, and be set to zero address by default. So if there is no policy contract or the field is missing in configuration, then the value ofminGasPriceandblockedwill be both zero and all transactions will pass the check.