-
Notifications
You must be signed in to change notification settings - Fork 278
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
Validate all relevant square sizes are committed to in MsgWirePayForData
#666
Validate all relevant square sizes are committed to in MsgWirePayForData
#666
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry for prematurely moving this from draft to ready for review. Tests passed locally but CI is failing on UPDATE: ready for review. The integ test failure was a known flake: #574 |
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.
left a few comments 🙂
this code is really important because it makes arranging the square way easier when the block proposer knows that all the txs in their mempool are signed over all the relevant square sizes.
Codecov Report
@@ Coverage Diff @@
## main #666 +/- ##
==========================================
+ Coverage 36.06% 36.54% +0.47%
==========================================
Files 24 24
Lines 2623 2657 +34
==========================================
+ Hits 946 971 +25
- Misses 1594 1600 +6
- Partials 83 86 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
just to note that this is consensus breaking |
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.
approving due to #677
I'm dumb, this is probably not consensus breaking. It will be nice when we have tests tho |
I think this is consensus breaking b/c prior to this change, validators would've accepted PFDs that didn't sign over all commits. After this change, those PFDs are considered invalid. Consensus tests would be very nice. +1 to #313 |
We didn't actually change anything for the |
Correct only the wire message, sorry for the lack of precision. Why does that have implications on consensus breaking vs non consensus breaking? |
Then that should only affect the mempool, as even if a |
Closes #620 (after #654 merges)