-
Notifications
You must be signed in to change notification settings - Fork 20.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EIP-1559: miner changes #22896
EIP-1559: miner changes #22896
Conversation
…t to TransactionsByMinerFeeAndNonce miner: set base fee when creating a new header, handle gas limit, log miner fees
…veTip miner: activate 1559 for testGenerateBlockAndImport tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -299,6 +300,19 @@ func (tx *Transaction) Cost() *big.Int { | |||
return total | |||
} | |||
|
|||
// EffectiveTip returns the effective miner tip for the given base fee. | |||
// Returns error in case of a negative effective miner tip. | |||
func (tx *Transaction) EffectiveTip(baseFee *big.Int) (*big.Int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct but I also added this function with slightly different semantics in my tx pool PR: my version does not return an error, it returns the negative value which the caller can handle as it wishes. For me this value is useful even if negative (I described the reasons in my gist). Do you think we should have two versions? Or use my version and check at the caller side when necessary? I guess you should check it for the minimum effective fee threshold anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe let's start with this. We could have this method maybe wrap your method, so the 'normal' case doesn't need to handle negative tips
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I could not meaningfully review the CalcGasLimit1559
function
// TxByPriceAndTime implements both the sort and the heap interface, making it useful | ||
// for all at once sorting as well as individually adding and removing elements. | ||
type TxByPriceAndTime Transactions | ||
type TxByPriceAndTime []*TxWithMinerFee | ||
|
||
func (s TxByPriceAndTime) Len() int { return len(s) } | ||
func (s TxByPriceAndTime) Less(i, j int) bool { | ||
// If the prices are equal, use the time the transaction was first seen for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is outdated
Ok |
* core/types, miner: create TxWithMinerFee wrapper, add EIP-1559 support to TransactionsByMinerFeeAndNonce miner: set base fee when creating a new header, handle gas limit, log miner fees * all: rename to NewTransactionsByPriceAndNonce * core/types, miner: rename to NewTransactionsByPriceAndNonce + EffectiveTip miner: activate 1559 for testGenerateBlockAndImport tests * core,miner: revert naming to TransactionsByPriceAndTime * core/types/transaction: update effective tip calculation logic * miner: update aleut to london * core/types/transaction_test: use correct signer for 1559 txs + add back sender check * miner/worker: calculate gas target from gas limit * core, miner: fix block gas limits for 1559 Co-authored-by: Ansgar Dietrichs <[email protected]> Co-authored-by: [email protected] <[email protected]>
This PR contains the changes for the miner, taken from #22833 , and lightly squashed.
This PR is based on
master
, and does not require the txpool changes to be merged first.Supersedes #22833