Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Update weight for im-online #5771

Merged
merged 7 commits into from
Apr 27, 2020
Merged

Update weight for im-online #5771

merged 7 commits into from
Apr 27, 2020

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Apr 24, 2020

The weight must consider all the cost of executing the extrinsic, validate_unsigned included.
To confirm formula a benchmark has been added.

In order to have the length of Keys it is added as argument of the dispatchable.

Thus the offchain worker is also changed. AFAIK changing the offchain worker doesn't require validators to update their nodes.
But old unsigned transaction submitted will fail, but if I understand the code correctly the offchain worker will try at every block if it is not online yet. @tomusdrw

@gui1117 gui1117 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 24, 2020
frame/im-online/src/lib.rs Outdated Show resolved Hide resolved
@gui1117 gui1117 marked this pull request as draft April 24, 2020 12:56
@gui1117 gui1117 force-pushed the gui-im-online-weight branch 3 times, most recently from 60a62a4 to fa32192 Compare April 24, 2020 13:51
frame/im-online/src/lib.rs Outdated Show resolved Hide resolved
_signature: <T::AuthorityId as RuntimeAppPublic>::Signature
_signature: <T::AuthorityId as RuntimeAppPublic>::Signature,
// checked in `validate_unsigned`
_keys_len: u32,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here keys_len isn't part of the signed heartbeat but I think its fine, though one can double-check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since imonline transactions have to be propagated to other authorities (since the ones that send it didn't have a chance to produce a block), there is a potential attack, where other authorities change the key_len inside the payload making the transaction invalid.
This is not super bad, since they can also change say session_index or simply censor the transactions, but with key_len we can't be sure what happened - we don't know if the authority is misbehaving or it was maliciously modified. The possible cases are:

  1. The hearbeat is incorrect, but has correct signature (i.e. the authority is misbehaving).
  2. The heartbeat is incorrect and has incorrect signature (i.e. was modified).
  3. The heartbeat was not seen (i.e. either not sent or censored).
  4. Key_len invalid (i.e. either modified or authority misbehaving).

I'd be for putting that into Heartbeat payload for simplicity, unless there are any reasons why we should not do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re discussions in benchmarking channel, we should probably use saturating ops here, since key_len is coming from the user.

Even though the transaction should be rejected, I think the overflow may have some unforseen consequences (for instance it will panic in debug mode), so better have saturating ops for everything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok for the saturation, params should have been checked in validate_unsigned but better be safe anyway.

I updated to put key_len part of heartbeat. 👍

@gui1117 gui1117 force-pushed the gui-im-online-weight branch from fa32192 to 352a63e Compare April 24, 2020 14:00
@gui1117 gui1117 force-pushed the gui-im-online-weight branch from 352a63e to bd9aa1f Compare April 24, 2020 14:01
@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 24, 2020
@gui1117 gui1117 marked this pull request as ready for review April 24, 2020 14:10
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides the possible improvement with key_len being signed as well.

Changing offchain workers does not require updating the nodes. Indeed old transactions will fail in case runtime update is performed in unlucky moment, but also as Gui mentioned, the OCW code will attempt to send another one after the grace period is over and the previous transaction was not included.

_signature: <T::AuthorityId as RuntimeAppPublic>::Signature
_signature: <T::AuthorityId as RuntimeAppPublic>::Signature,
// checked in `validate_unsigned`
_keys_len: u32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since imonline transactions have to be propagated to other authorities (since the ones that send it didn't have a chance to produce a block), there is a potential attack, where other authorities change the key_len inside the payload making the transaction invalid.
This is not super bad, since they can also change say session_index or simply censor the transactions, but with key_len we can't be sure what happened - we don't know if the authority is misbehaving or it was maliciously modified. The possible cases are:

  1. The hearbeat is incorrect, but has correct signature (i.e. the authority is misbehaving).
  2. The heartbeat is incorrect and has incorrect signature (i.e. was modified).
  3. The heartbeat was not seen (i.e. either not sent or censored).
  4. Key_len invalid (i.e. either modified or authority misbehaving).

I'd be for putting that into Heartbeat payload for simplicity, unless there are any reasons why we should not do that.

@tomusdrw tomusdrw added A5-grumble and removed A0-please_review Pull request needs code review. labels Apr 24, 2020
frame/im-online/src/lib.rs Outdated Show resolved Hide resolved
@gui1117 gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A5-grumble labels Apr 27, 2020
@gui1117 gui1117 requested a review from kianenigma as a code owner April 27, 2020 15:57
@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 27, 2020
@gavofyork gavofyork merged commit 9b23e35 into master Apr 27, 2020
@gavofyork gavofyork deleted the gui-im-online-weight branch April 27, 2020 19:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants