-
Notifications
You must be signed in to change notification settings - Fork 238
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
Geth v1.12.2 #1090
Geth v1.12.2 #1090
Conversation
@@ -2082,7 +2082,7 @@ func TestBuildSubnetEVMBlock(t *testing.T) { | |||
} | |||
txs[i] = signedTx | |||
} | |||
errs := vm.txPool.AddRemotes(txs) | |||
errs := vm.txPool.AddRemotesSync(txs) |
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.
Not sure if sync/async matters here, used sync to avoid creating a wrapper
@@ -2644,7 +2644,7 @@ func TestFeeManagerChangeFee(t *testing.T) { | |||
t.Fatal(err) | |||
} | |||
|
|||
err = vm.txPool.AddRemote(signedTx2) | |||
err = vm.txPool.AddRemotesSync([]*types.Transaction{signedTx2})[0] |
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.
same as above
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.
Mostly LGTM, just few questions.
I also put up a nit PR here: #1099
core/txpool/txpool.go
Outdated
return shouldContinue | ||
} | ||
for _, subpool := range p.subpools { | ||
subpool.IteratePending(callback) |
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 wonder if we should just change the signature for subpool to IteratePending(f func(tx *Transaction) bool) bool
core/state/pruner/pruner.go
Outdated
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.
why we havent applied this? ethereum/go-ethereum#27525
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.
added
} | ||
pool.mu.Unlock() | ||
resetDone <- newHead | ||
p.reorgFeed.Send(core.NewTxPoolReorgEvent{Head: newHead}) |
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.
reorgFeed
is added here compared to upstream.
// BlobPool is the transaction pool dedicated to EIP-4844 blob transactions. | ||
// | ||
// Blob transactions are special snowflakes that are designed for a very specific | ||
// purpose (rollups) and are expected to adhere to that specific use case. These |
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.
lol
// - The first observation is that comparing 1559 base fees or 4844 blob fees | ||
// needs to happen in the context of their dynamism. Since these fees jump | ||
// up or down in ~1.125 multipliers (at max) across blocks, comparing fees | ||
// in two transactions should be based on log1.125(fee) to eliminate noise. |
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.
Is this true that they will increase at max of 1.125 of the current base fee? I think that may be incorrect for EIP-1559
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.
cc @abi87 who just pointed this out to me today
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.
It seems to me it was the original intention (see ethereum/EIPs#1559) but that is not strictly true anymore in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1559.md nor in the implementation.
To be clear, I understand fee changes are still capped, but not of 1/changeDemon
(which is 1/8 or 12,5% as mentioned above). changeDemon
seems a smoothing parameter to control fee dynamics.
Why this should be merged
Updates shared code with go-ethereum
How this works
Takes recent changes from the upstream repository, v1.12.0..v1.12.2. Steps:
How this was tested
CI, manual testing pending
How is this documented
Not yet