-
Notifications
You must be signed in to change notification settings - Fork 6
Qbft gossip validation #454
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
Qbft gossip validation #454
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
=======================================
Coverage ? 83.13%
Complexity ? 1120
=======================================
Files ? 228
Lines ? 8071
Branches ? 629
=======================================
Hits ? 6710
Misses ? 1019
Partials ? 342
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
consensus/src/main/kotlin/maru/consensus/qbft/QbftMessageProcessor.kt
Outdated
Show resolved
Hide resolved
jonesho
left a comment
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, just 2 minor comments
consensus/src/main/kotlin/maru/consensus/qbft/QbftMessageProcessor.kt
Outdated
Show resolved
Hide resolved
| import org.junit.jupiter.api.Test | ||
|
|
||
| class MinimalQbftMessageDecoderTest { | ||
| private val signatureAlgorithm = SignatureAlgorithmFactory.getInstance() |
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.
IMO, we should use Maru's crypto module for that if we're using real signatures
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.
Maru's crypto module only provides key generation and hashing. The Signing object only has sign method for ULong. So I can replace key generation and hashing with Maru code, but the message bytes signing I will need to use NodeKey.
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.
There is a ULongSigner, but you're free to add another one
consensus/src/testFixtures/kotlin/maru/consensus/qbft/TestBesuMessageData.kt
Outdated
Show resolved
Hide resolved
consensus/src/testFixtures/kotlin/maru/consensus/qbft/TestBesuMessageData.kt
Outdated
Show resolved
Hide resolved
consensus/src/test/kotlin/maru/consensus/qbft/MinimalQbftMessageDecoderTest.kt
Outdated
Show resolved
Hide resolved
consensus/src/test/kotlin/maru/consensus/qbft/QbftMessageProcessorTest.kt
Outdated
Show resolved
Hide resolved
consensus/src/test/kotlin/maru/consensus/qbft/QbftMessageProcessorTest.kt
Show resolved
Hide resolved
consensus/src/main/kotlin/maru/consensus/qbft/MinimalQbftMessageDecoder.kt
Show resolved
Hide resolved
consensus/src/main/kotlin/maru/consensus/qbft/QbftMessageProcessor.kt
Outdated
Show resolved
Hide resolved
crypto/src/testFixtures/kotlin/maru/crypto/PrivateKeyGenerator.kt
Outdated
Show resolved
Hide resolved
| roundNumber = roundNumber, | ||
| author = Address.wrap(Bytes.wrap(author)), | ||
| ) | ||
| } |
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.
Bug: RLP Decoder: Unclosed Lists Corrupt Parsing
The RLP decoder enters an extra list for PROPOSAL and ROUND_CHANGE messages but never exits it. After calling signedDataRlp.leaveList() on line 73, there should be another leaveList() call for these message types to properly exit the outer list wrapper entered on line 62, ensuring the RLP parsing state is correctly maintained.
This adds basic validation of qbft messages before they are gossiped. The validation is takes a simple approach and follows what is done in Besu for gossiping messages in QbftController.processMessages which determines whether a qbft message should be gossiped and processed by Qbft.
I also added a new deserialiser for QbftMessage to get only the basic metadata to determine whether message is valid. This avoids needing to potentially decode a lot more data such blocks which can be quite large.
The basic validation rules are:
Note
Adds lightweight QBFT message decoding and validation for gossiping, and refactors crypto usage to SecpCrypto across the codebase.
MinimalQbftMessageDecoderandQbftMessageProcessorto decode minimal metadata (sequence, round, author) and validate messages before queuing/gossiping.QbftValidatorFactoryviap2PNetwork.subscribeToQbftMessages.SCEP256SealVerifiernow usesSecpCrypto.signatureAlgorithm.MinimalQbftMessageDecoderTestandQbftMessageProcessorTest.Cryptointerface andSecpCryptoimplementation; addssignatureToAddressandprivateKeyBytesWithoutPrefixhelpers.PrivateKeyGeneratorto exposenodeKey.Cryptousages withSecpCryptoinMaruApp, P2P (ENR,P2PNetworkImpl), tests, and utilities.consensusaddstestFixtures(project(":crypto")).cryptotest fixtures add required Besu/Tuweni dependencies.Written by Cursor Bugbot for commit e43fb11. This will update automatically on new commits. Configure here.