Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8516 +/- ##
==========================================
+ Coverage 57.68% 57.74% +0.06%
==========================================
Files 291 291
Lines 21338 21265 -73
==========================================
- Hits 12308 12279 -29
+ Misses 8461 8418 -43
+ Partials 569 568 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gjermundgaraba
left a comment
gjermundgaraba
left a comment
There was a problem hiding this comment.
I just did an initial review, but it looks very good 👍
| }, | ||
| channeltypes.ErrRedundantTx, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Yep, wasn't clear to me why the the older Submit... behaviour affects message redundancy, but judging by the rest of the tests the update client message should behave as this test does
There was a problem hiding this comment.
Hmmm... this just seems like an oversight that happened once we merged the misbehaviour and recvPacket messages into a single endpoint.
Generally speaking, a relayer wouldn't pair a malicious update message with recvpacket messages together, since that would just freeze the client that you wanted to use later.
It could be the case that someone submits an update message that ends up being proof of misbehaviour (i.e. the update client is just a single header but it conflicts with a consensus state that already exists). If this update message was paired with only redundant receive packet messages it would also be ignored.
Perhaps we should check the update message first before rejecting the redundant messages in the antehandler
There was a problem hiding this comment.
In practice, this is not a huge deal though
There was a problem hiding this comment.
What is the most important scenario we should actually test for here in the antehandler?
There was a problem hiding this comment.
Or, to put it another way: is this test covering the most important scenario wrt misbehavior?
Also: I don't really think it makes sense to optimize for a scenario where someone posts misbehavior and recvPacket. I just want to make sure we have the right test case here
There was a problem hiding this comment.
The most important scenario:
There exists a consensus state in client with height 50 and hash: 0xABC
Relayer submits a new tx with:
- Valid Header Update for height 50 and hash: 0xDEF
- Redundant RecvPacket txs
Note: If the header update was submitted just on its own; IBC would automatically detect the misbehaviour even though it is a regular update because it conflicts with existing consensus state.
My suspicion however, is that this tx would be dropped by the antehandler
johnnylarner
requested review from
AdityaSripal,
DeshErBojhaa and
srdtrk
as code owners
gjermundgaraba
left a comment
gjermundgaraba
left a comment
There was a problem hiding this comment.
Looks mostly good to go. Left a couple of minor comments, and pending answer on the question from Aditya :)
Co-authored-by: Gjermund Garaba <gjermund@garaba.net>
AdityaSripal
left a comment
AdityaSripal
left a comment
There was a problem hiding this comment.
LGTM! Yes, the redundant tx antedecorator can be improved to allow misbehavior processing even if the attached packet messages are redundant
But in practice, this is a relatively small concern
| }, | ||
| channeltypes.ErrRedundantTx, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Hmmm... this just seems like an oversight that happened once we merged the misbehaviour and recvPacket messages into a single endpoint.
Generally speaking, a relayer wouldn't pair a malicious update message with recvpacket messages together, since that would just freeze the client that you wanted to use later.
It could be the case that someone submits an update message that ends up being proof of misbehaviour (i.e. the update client is just a single header but it conflicts with a consensus state that already exists). If this update message was paired with only redundant receive packet messages it would also be ignored.
Perhaps we should check the update message first before rejecting the redundant messages in the antehandler
| }, | ||
| channeltypes.ErrRedundantTx, | ||
| }, | ||
| { |
There was a problem hiding this comment.
In practice, this is not a huge deal though
…ehaviour-and-corresponding-rpc
gjermundgaraba
deleted the
johnnylarner/ibcgo-2327-remove-msgsubmitmisbehaviour-and-corresponding-rpc
branch
3 participants
Description
Will add to the CHANGELOG and double check the test code again tomorrow.
closes: #3889
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) if anything is changed.godoccomments if relevant.Files changedin the GitHub PR explorer.