Skip to content
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

spec: Add penalize notifications. #537

Merged
merged 10 commits into from
Aug 10, 2020

Conversation

JoeGruffins
Copy link
Member

part of #444

It was discussed that the notification be tried on reconnect, and to add it to connect, but I thought just adding it to the connect response would be enough/better.

Also:

message (e.g. state connected to complete swaps or risk further penalty or loss of reg fee!)

I think can be hard coded into the client, since it will be true for all bans

Can be viewed here

spec/community.mediawiki Outdated Show resolved Hide resolved
spec/comm.mediawiki Outdated Show resolved Hide resolved
@chappjc chappjc requested a review from buck54321 July 14, 2020 19:10
@chappjc chappjc added the spec label Jul 14, 2020
spec/community.mediawiki Outdated Show resolved Hide resolved
spec/comm.mediawiki Outdated Show resolved Hide resolved
Comment on lines 72 to 80
| accountid || string || hex encoding of the client's account ID
|-
Copy link
Member

Choose a reason for hiding this comment

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

A connected websocket connection is specific to an account, so the accountid field is superfluous.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that the signature would be meaningless after the fact unless the accountid was included. While you could prove that a ban was dealt out from a particular server, you couldn't prove to whom it was directed. Although for simply verifying the server and communicating, it is not needed. I will take it out since there may never be need for the ban receiver to prove they were banned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess someone could reuse one of these messages though, if it didn't have the to whom. The signature will check out, only the timestamp will be old. Can that in any way be used as an attack vector? OK because wss is safe?

Copy link
Member

Choose a reason for hiding this comment

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

My thinking was that the signature would be meaningless after the fact unless the accountid was included.

That's a good point. We can leave it for now and decide on later updates what's needed for the new system.

spec/community.mediawiki Outdated Show resolved Hide resolved
spec/community.mediawiki Outdated Show resolved Hide resolved
spec/community.mediawiki Outdated Show resolved Hide resolved
spec/community.mediawiki Outdated Show resolved Hide resolved
@JoeGruffins JoeGruffins force-pushed the penaltynotification branch 2 times, most recently from 0829803 to e9aa238 Compare July 16, 2020 07:45
Copy link
Member

@dnldd dnldd 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.


<code>result</code>
{|
! field !! type !! description
|-
| matches || &#91;object&#93; || list of [[orders.mediawiki/#Match_negotiation|Match objects]]
|-
| penalty || object || the client's most recent [[community.mediawiki/#Rules_of_Community_Conduct|broken rule]]. omitted if in good standing
Copy link
Member

Choose a reason for hiding this comment

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

If this is going to be an object, the object will need to be defined. Can you define the object separately and just reference it here and in the penalty notification?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this how you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the sig would not be a field in the object. It would stay as a field in the penalty payload.

| details || string || a message describing the penalization
|-
| sig || string || hex-encoded server's signature of the serialized penalization data
|}
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to describe the serialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

As in how many bytes per field and the order?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a few different patterns, but will just adding the size of the fields be sufficient in this case? Should there be a separate Penalty serialization?

Copy link
Member

Choose a reason for hiding this comment

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

I'd personally prefer a serialization for the Penalty object. For the penalty payload you wouldn't need a whole serialization description table, because you could just say sig || string || hex-encoded signature of the serialized Penalty object.

I found a few different patterns, but will just adding the size of the fields be sufficient in this case?

In this case, since all of the fields of the Penalty object are included in the serialization, you could try to combine them into one. I think the only place that's done so far is for the order prefix description, and I have mixed feelings on how well it works. The descriptions needed for serialization is often quite different than a description suited for the JSON field, so you'll need to combine the information carefully to avoid confusion.

@JoeGruffins
Copy link
Member Author

The test failure happened because I pushed a commit with an unintentional change. I rebased after that but the tests did not re-test and the failure remained.

spec/community.mediawiki Outdated Show resolved Hide resolved
Copy link
Member

@buck54321 buck54321 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. A couple last touchups though.

Would you please add entries in the table of contents in README.mediawiki.

** [[community.mediawiki/#Penalization_Notification|Penalization Notification]]
*** [[community.mediawiki/#Penalty_Object|Penalty Object]]

spec/community.mediawiki Outdated Show resolved Hide resolved
@chappjc chappjc merged commit 0cb4410 into decred:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants