Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Geth v1.12.2 #1090

wants to merge 1 commit into from

Conversation

darioush
Copy link
Collaborator

@darioush darioush commented Feb 11, 2024

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:

  1. Add upstream as a remote and fetch tags eg:
  • git remote add ethereum [email protected]:ethereum/go-ethereum.git
  • git fetch ethereum --tags
  • (I didn't set up a config to fetch the tags with a prefix, but we can do that if we prefer)
  1. run ./scripts/format_as_upstream
  • This will rename the repository as "ethereum/go-ethereum"
  1. git pull ethereum v1.12.0 -s ours --allow-unrelated-histories
  2. git pull ethereum v1.12.2
  3. Resolve merge conflicts & update code
  4. ./scripts/build_test.sh (make sure tests pass locally)
  5. ./scripts/format_as_fork.sh v1.12.2
  • Note the version specified as an argument should be the newer version.
  • (unfortunately this script has a small bug, which I didn't fix yet, so it will not rename these comments d08dfe2)

How this was tested

CI, manual testing pending

How is this documented

Not yet

@@ -2082,7 +2082,7 @@ func TestBuildSubnetEVMBlock(t *testing.T) {
}
txs[i] = signedTx
}
errs := vm.txPool.AddRemotes(txs)
errs := vm.txPool.AddRemotesSync(txs)
Copy link
Collaborator Author

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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Collaborator

@ceyonur ceyonur left a 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

return shouldContinue
}
for _, subpool := range p.subpools {
subpool.IteratePending(callback)
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@darioush darioush marked this pull request as ready for review February 26, 2024 21:52
}
pool.mu.Unlock()
resetDone <- newHead
p.reorgFeed.Send(core.NewTxPoolReorgEvent{Head: newHead})
Copy link
Collaborator Author

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.

Comment on lines +148 to +151
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

Comment on lines +259 to +262
// - 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.
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Contributor

@abi87 abi87 Feb 28, 2024

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.

@darioush darioush changed the title Geth v1.12.2 x Geth v1.12.2 Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants