protocol pkg documentation

[masomel]

Part of #32

[masomel]

TODO:

• Add ErrorMalformedClientMessage case to all directory operations doc
• Finish message.go doc
• Minor formatting changes in error.go doc
• Add message module to doc.go
• Add consistency checks module to doc.go

[vqhuy]

s/sserver/server

[vqhuy]

nit-picky: s/called/ called.

[vqhuy]

I'm not sure how you feel about this but golint reports all warnings in the form comment on exported function NewDirectory should be of the form "NewDirectory ...".
It is described aslo in CoreReviewComments

Comments should begin with the name of the thing being described and end in a period.

[masomel]

I'm not sure how you feel about this but golint reports all warnings in the form comment on exported function NewDirectory should be of the form "NewDirectory ...".

Thanks for pointing this out! This helps with consistent formatting, which I'm definitely in favor of :) So I'll run golint on my documentation.

[vqhuy]

I would like to change this to "ErrorResponses contains error codes indicating the client should omit the consistency checks. These errors indicate that either a client request could not be processed due to a malformed client request, an internal server error or due to a malformed server response." to make thing a bit clearer.
Actually ErrorResponses is currently being used in the client side to omit the consistency check (it should include ErrorMalformedDirectoryMessage as well), and I think we should have a better name for this. What do you think?

[masomel]

Actually ErrorResponses is currently being used in the client side to omit the consistency check

Ok. I didn't realize that this was an explicit purpose of these error responses. I'll definitely make this clear in the doc, thanks!

it should include ErrorMalformedDirectoryMessage as well

Yes, and similarly, ErrorCouldNotVerify is another good candidate, I think.

we should have a better name for this.

Agreed. I was thinking about this earlier and all possible responses (except passed and success) start with the "Error-" prefix, but only a handful of them are actually treated as errors. Maybe it makes sense to treat the non-errors more as different results, e.g. change the prefix to something like "Req-" for all request results (including Success?) and "Check-" for all consistency check results? So we'd be dealing with error codes such as "ReqNameNotFound" or "CheckBadCommitment". This also makes it clearer on which end of the protocol each "error" is returned. What do you think?

[vqhuy]

ErrorCouldNotVerify is another good candidate

It would be removed in https://github.com/coniks-sys/coniks-go/pull/113/files#diff-78429462c41fd4a6aab3cc13baa05919L28.

Maybe it makes sense to treat the non-errors more as different results, e.g. change the prefix to something like "Req-" for all request results (including Success?) and "Check-" for all consistency check results? So we'd be dealing with error codes such as "ReqNameNotFound" or "CheckBadCommitment".

Yes, it makes sense to me, maybe we can keep Error- prefix for error codes in ErrorResponses.

[masomel]

It would be removed

I see, ok this makes sense.

we can keep Error- prefix for error codes in ErrorResponses.

Yes, definitely, sorry if this wasn't clear. My idea was to distinguish between actual errors and results.

[vqhuy]

I will submit a pull after this is merged.

[masomel]

I just created issue #114 for this, and I think this is something we should push back to our next release. This pull is just for the documentation, so I think this pull should be merged after all other protocol-related pulls are merged. What do you think of the following merge order: #74, #113, #105, #114?

[vqhuy]

SGTM. I'm waiting for others' review on #74. :)

[arlolra]

lowercase "constructs"

[arlolra]

maybe add "to be used in the next epoch"?

[arlolra]

In the EpochDeadline() comment, it says "current" and here "latest". We should probably just stick to one.

[masomel]

Good point. I've noticed that we're using the two interchangeably and that could be confusing. I think "current" makes more sense if we consider that when the server updates the directory and increments the epoch counter to t, the system has "entered" epoch t and remains in epoch t until the next directory update. Might even be worth changing the code to say CurrentSTR() etc. where "latest" is used right now. What do you think? cc @c633 @liamsi

[vqhuy]

the system has "entered" epoch t and remains in epoch t until the next directory update

I think it should be t+1. Since it is signed tree root and in "current" epoch t+1, the tree root haven't been signed yet, "latest" makes more sense to me.

[masomel]

But lookups in the "current" epoch if "current" is t+1 search the tree for t because the tree root hasn't been signed yet. So while t+1 hasn't been signed yet, doesn't that really make t the current epoch since all operations are performed on the tree for t?

[vqhuy]

But lookups in the "current" epoch if "current" is t+1 search the tree for t

Then maybe we should use lookups in the "latest" epoch instead. I think "latest" clearly indicates that we actually perform all operations on the tree which has been signed, whereas "current" makes me a little confuse.

[masomel]

I think "latest" clearly indicates that we actually perform all operations on the tree which has been signed

This makes sense to me. My argument for "current" is that I think it would capture the user's intent better, since we don't expect user's to really have a notion of the underlying PAD. But if the three of you feel stronger about using "latest", let's go with that.

[liamsi]

I OK with either "current" or "latest" as long as it used consistently and we somewhere clearly state what it means (the lastest STR is the one that is ...)

[arlolra]

Should this be (registration proof, ErrorNameExisted)?

[masomel]

It should, thanks.

[arlolra]

The above assumes useTBs is false, right? We should at least mention that, if we aren't going to enumerate the TB cases.

[masomel]

No, it does assume that useTBs is true. I've added in the TB cases to make this explanation more complete.

[arlolra]

Again, "latest snapshot" or "current epoch"

[arlolra]

s/snapshot/epoch/

[masomel]

"Epoch" works, though, I was trying to use PAD-terminology here, since KeyLookupInEpoch() is really looking in the snapshot for the indicated epoch. I'll clarify this.

[arlolra]

Maybe we use "latest" more often.

[arlolra]

Yeah, "latest"

[arlolra]

Overall, looks pretty good. Two general comments is the consistency of "latest (snapshot)?" vs "current epoch". And, I'm not sure if godoc recognizes it, but wrapping argument names in backticks, likeThis.

[vqhuy]

And, I'm not sure if godoc recognizes it, but wrapping argument names in backticks, likeThis.

Unfortunately, godoc doesn't recognize backticks format :(

[liamsi]

"[...] or a cryptographic verification**.**" (?)

[liamsi]

The most interesting fact about ErrorCode isn't that it is internally an int, but rather that it is a integer representation of (specific) error-messages (those that travel through the wire??)
Also, it's interesting (for an overview) that they can be mapped to human readable string representations.

[masomel]

Makes sense. I wasn't sure exactly what to write for this type declaration. Thanks!

[vqhuy]

they can be mapped to human readable string representations.

I would like to add something like "... it implements the built-in error interface".

[liamsi]

That's even better! ("the built-in error interface type")

[liamsi]

Nitpicking: should be consistent with the description in doc.go "message format" (singular or plural)

[liamsi]

And just in case this isn't intentional: this comment won't show up in the rendered godoc documentation.

[masomel]

Nope, it's intentional, I know this won't show up. I just think it's good practice to have a summary at the top of every class/module/file.

[liamsi]

"The error code is set to Success." <- That's confusing. But also in combination of what the constructor is doing: One can pass it an ErrorCode as a parameter (can be Success or anything else). This ErrorCode e is incorporated into the freshly created Response (OK) and returned (why)?

[masomel]

"The error code is set to Success." <- That's confusing. But also in combination of what the constructor is doing: One can pass it an ErrorCode as a parameter (can be Success or anything else)

If you look at directory.Monitor(), which is the only place where NewMonitoringProof is called, the error code is set to Success, so that's what I was referring to. But you're definitely right that ErrorCode could be set to anything. I think it would make sense to change the signature of the constructor and to set the ErrorCode internally to Success if that's the only "error" the constructor can ever return.

This ErrorCode e is incorporated into the freshly created Response (OK) and returned (why)?

I had the same question a while back. The error is returned so it can be propagated up to the server for logging: #71 (comment)

[liamsi]

I think it would make sense to change the signature of the constructor and to set the ErrorCode internally to Success if that's the only "error" the constructor can ever return.

Yes, I agree.

[masomel]

Cool, I'll go ahead and make those tweaks, too.

[liamsi]

Super nitpicky: We use "key" for many different concepts. Maybe, public-key is better suited here? Nah, I guess that's clear in this context. Sorry for the noise.

[liamsi]

sserver -> server

[liamsi]

lastest -> latest

[liamsi]

Thinking aloud here: I wonder if it wouldn't make more sense to move the actual behaviour of the server to methods where it actually happens (for instance KeyLookupInEpoch creates the list of STRs which are, then, passed to this constructor)

[masomel]

I'm a bit confused, what do you mean by "behavior of the server"? Are you suggesting KeyLookupInEpoch shouldn't generate the list of STRs but that this should be done in the server? The list of STRs is part of the KeyLookupInEpoch protocol response, though. Maybe my wording and use of the term "server" here is misleading?

[liamsi]

Oh sorry, reading my comment, I was confused myself...

What I meant is that descriptions of behaviour of the server should be moved to places where this behaviour happens; for instance, comments like the above: "In either case, the server returns a list of STRs for the epoch range" should appear above methods where the STRs are actually generated (the comment of KeyLookupInEpoch should talk about the STRs and not the necessarily this constructor where they are just passed as an argument and aren't generated/returned).

Does this make more sense?

[masomel]

descriptions of behaviour of the server should be moved to places where this behaviour happens

Gotcha. I'll go through the docs and see if I can make the descriptions more precise.

[liamsi]

Some minor comments, questions, and nitpicks/typos. In overall: looks good to me!

[liamsi]

LGTM (feel free to ignore my confusing comment here)

[vqhuy]

Since we have moved tb.go to protocol/, this doc should cover the temporary binding documentation as well. Do you want me to do a copy, @masomel ?

[masomel]

Since we have moved tb.go to protocol/, this doc should cover the temporary binding documentation as well. Do you want me to do a copy, @masomel ?

I'll do a rebase. I need to the go through some of the descriptions again anyway, so I'll make sure to check that they still make sense with the new changes.