-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Verify tx response data against request #6439
Conversation
482c157
to
31d2a5e
Compare
Co-authored-by: Matthias Seitz <[email protected]>
Co-authored-by: Matthias Seitz <[email protected]>
…reth into emhane/validate-tx-response
32c3736
to
963012b
Compare
573f333
to
358f3ac
Compare
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 like the new traits and types because they make this process easier to reason about.
left some suggestions for reducing a few allocs that I think we don't need/can live without.
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
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.
smallest nit and a question, feel free to ignore the nits
|
||
let unvalidated_payload_len = verified_payload.len(); | ||
|
||
// todo: report peer for sending invalid response |
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 for a follow up?
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.
yeah exactly can add which issues it depends on
@@ -972,6 +983,8 @@ where | |||
return | |||
} | |||
|
|||
let mut transactions = transactions.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.
Should we impl deref etc on PooledTransactions
?
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.
yeah sure, Deref
is always safe, DerefMut
you have to be sure this doesn't effect existing behaviour. usually only do it on my own new types cause of this.
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.
anyhow, out of scope of pr I think
Co-authored-by: Oliver Nordbjerg <[email protected]>
Co-authored-by: Oliver Nordbjerg <[email protected]>
…reth into emhane/validate-tx-response
Closes #6438, closes #6396