-
Notifications
You must be signed in to change notification settings - Fork 20k
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
eth/catalyst: use setcanonical instead of sethead in simulated fork #30465
base: master
Are you sure you want to change the base?
Conversation
Hmm looks like a test for the simulated backend fails with this |
In order to fix the failing test, I had to change the test semantics. In the old behaviour, it expected a dropped block to simply disappear, along with all txs. Now, however, we expect it to use the fork-events, where the tx pool actually picks up transactions that were dropped in a dropped head. I'm not 100% sure if the changed test is ok, or if we should replace/fix it some other way. cc @karalabe ? |
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.
sgtm
ethclient/simulated/backend_test.go
Outdated
@@ -228,15 +228,13 @@ func TestForkResendTx(t *testing.T) { | |||
if err := sim.Fork(parent.Hash()); err != nil { | |||
t.Errorf("forking: %v", err) | |||
} | |||
|
|||
// 5. | |||
sim.Commit() | |||
if err := client.SendTransaction(ctx, tx); err != nil { |
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 the 2nd SendTransaction
can be omitted if the tx
is expected to be picked up automatically?
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.
Good point, thanks
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
Fixes #30448