Skip to content

Conversation

@darioush
Copy link

@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
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
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
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
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.

5 participants