-
Notifications
You must be signed in to change notification settings - Fork 475
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
simulators/ethereum/engine/cancun: Maintain devp2p client peering #1052
simulators/ethereum/engine/cancun: Maintain devp2p client peering #1052
Conversation
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.
Some comments, thanks!
@@ -27,6 +27,7 @@ import ( | |||
type CancunTestContext struct { |
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.
Could we take this opportunity to rename this to something more generic, like TestContext
.
And also create a func NewTestContext() *TestContext {
that performs all init operations required such as the t.DevP2PConnections = make(map[uint64]*devp2p.Conn)
that we do in line 782.
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.
We could also create a func (t *TestContext) Close() error {
that we could defer t.Close()
after we call NewTestContext()
when creating the context to do some proper cleanup such as the closing of the connections that are still open in t.DevP2PConnections
.
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.
Very happy to extend to this!
My only rebutle is that we'd be adding the extra initialization steps for all the CancunBaseSpec tests, where its only required for 2 of them.
Maybe we have a generic TestContext
and additionally a specific DevP2PTestContext
, the latter having the init steps you mentioned. Where we only init these steps for the DevP2P tests?
(edited) What I wrote above didn't make sense, will implement your suggestions.
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 added your suggestions in: 25ffdf9
Let me know what you think :)
5178de4
to
25ffdf9
Compare
Description
Request Blob Pooled Transactions
for geth.NewPooledTransactionHashes
devp2p msg for blob txs, we must now peer first before sending blob txs.