Remove Policies interface #78

vqhuy · 2016-09-01T03:38:47Z

Remove Policies interface and use ConiksPolicies struct instead.

arlolra · 2016-09-01T03:59:14Z

Looks good, but are there any other policies besides CONIKS policies? If not, maybe just call this Policies?

vqhuy · 2016-09-01T04:30:44Z

Yes, it makes more sense.

masomel · 2016-09-01T04:45:30Z

LGTM (For some reason I thought we had taken care of this in #41, but I guess that was just a renaming).

arlolra · 2016-09-01T05:55:30Z

merkletree/str.go

@@ -21,10 +21,10 @@ type SignedTreeRoot struct {
 	PreviousEpoch   uint64
 	PreviousSTRHash []byte
 	Signature       []byte
-	Policies        Policies
+	Policies        *Policies


This is a late night comment, so take it for what it's worth. Is there any chance that changing this to a pointer is going to lead to the Policies in the STR being mutated?

I think it's still safe by far. Changing this to pointer absolutely makes the Policies being mutated but it's the same for other fields of slice type.

Sort of ... there's an expectation that policies are to be able to be altered between epochs. With this change, we're asking developers to be aware that they need to allocate a new Policies structure each time, rather than manipulate the one they may be retaining since startup. It just feels like a copy is less error prone.

After writing that sentence, I looked at directory.go and saw that we're doing this right,
https://github.com/coniks-sys/coniks-go/blob/master/protocol/directory.go#L43
but still feel uncomfortable with the pointer. Maybe others have an opinion?

~~Also, I think we have a bug #80~~

With this change, we're asking developers to be aware that they need to allocate a new Policies structure each time, rather than manipulate the one they may be retaining since startup

~~I don't think that we're asking developers to allocate a new Policies struct each time. They just need to allocate a new struct when the policies change, much like when we using the interface.~~ Nevermind, I got your idea.
@masomel @liamsi : What do you think?

Currently, the policies are accessed in the merkletree package only. Couldn't we even make the field unexported? So we hide the inner workings and still use a pointer here (which was introduced for performance reasons, I guess?)

Thank for the clarification. Then I'm also slightly in favour of making this a "copy" instead of a pointer. Simply because it is slightly more explicit.

I think I agree with @arlolra and @liamsi, a copy seems safer than a pointer.

I fixed it in ceb0a58cdd139cb212c07dccabfb2f07be51520a. But a part of me still feels uncomfortable when considering the memory use between a copy and a pointer, in this case.

Maybe another solution is to ask developers to be aware that they need to allocate a new Policies structure each time, in the documentations.

Isn't the overhead massively dominated by the cloned trees in memory? The tradeoff is the potential for a corrupted state, which seems worth it.

Unfortunately, documenting that sort of expectation is unreliable and I would caution to err on the side of programming defensively.

Unfortunately, documenting that sort of expectation is unreliable and I would caution to err on the side of programming defensively.

Yup, I will take it as a lesson. Thanks!
Btw, what do you think about ceb0a58cdd139cb212c07dccabfb2f07be51520a?

liamsi · 2016-09-07T09:39:21Z

LGTM (no strong opinion on this discussion: #78 (comment))

masomel · 2016-09-08T16:11:31Z

Btw, what do you think about ceb0a58?

LGTM.

arlolra · 2016-09-10T19:46:13Z

merkletree/pad.go

@@ -21,12 +21,12 @@ type PAD struct {
 	snapshots    map[uint64]*SignedTreeRoot
 	loadedEpochs []uint64 // slice of epochs in snapshots
 	latestSTR    *SignedTreeRoot
-	policies     Policies // the current policies in place
+	policies     *Policies // the current policies in place


pad.policies is in use, so the same mutation comment applies here.

I think you should revert everything in this patch that isn't strictly about removing the interface, sorry.

pad.policies is in use, so the same mutation comment applies here.

Yup, that's why I have this bug bc36c45e36c9eb51bc7510034ac43f80c44f3b30 (fixed in this commit). Thanks!

I think you should revert everything in this patch that isn't strictly about removing the interface, sorry.

Oops, my bad. I split it into a separate commit (af99421)

Btw, I'm so sorry because of these late commits and response.

arlolra · 2016-09-14T01:02:00Z

protocol/directory.go

 }

+// EpochDeadline returns the EpochDeadline of the current policies
+// If you want to query the EpochDeadline before the new policies
+// is updated, use LatestSTR().Policies.EpochDeadline


But that's exactly what you want to use in EpochUpdate.

https://github.com/coniks-sys/coniks-go/blob/master/keyserver/server.go#L192

The timer needs to fire based on LatestSTR's deadline.

Again, I really think you should revert everything here that isn't about removing the interface. The new test is good to add, but orthogonal.

The timer needs to fire based on LatestSTR's deadline.

Yes, you're completely right.

Again, I really think you should revert everything here that isn't about removing the interface. The new test is good to add, but orthogonal.

I didn't get your idea yet :( What should be removed?

Sorry for not being clear. Just the UpdatePolicies stuff. The shallow copy in pad.policies = *policies is all we were after.

Let me update this, one sec.

arlolra · 2016-09-14T16:06:27Z

@c633 See d0aff9a. If you're happy with that, please merge. Thanks!

vqhuy · 2016-09-14T17:01:23Z

Thanks!

vqhuy force-pushed the policies branch 2 times, most recently from ab17498 to 33815cd Compare September 1, 2016 03:52

vqhuy force-pushed the policies branch from 33815cd to c641637 Compare September 1, 2016 04:30

arlolra reviewed Sep 1, 2016
View reviewed changes

masomel added this to the 0.1.0 milestone Sep 1, 2016

vqhuy force-pushed the policies branch 2 times, most recently from 6e242f1 to ceb0a58 Compare September 8, 2016 04:49

arlolra reviewed Sep 10, 2016
View reviewed changes

vqhuy force-pushed the policies branch from ceb0a58 to bc36c45 Compare September 13, 2016 23:52

arlolra reviewed Sep 14, 2016
View reviewed changes

Remove Policies interface

d0aff9a

arlolra force-pushed the policies branch from bc36c45 to d0aff9a Compare September 14, 2016 16:04

vqhuy merged commit d0aff9a into master Sep 14, 2016

vqhuy deleted the policies branch September 14, 2016 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Policies interface #78

Remove Policies interface #78

vqhuy commented Sep 1, 2016

arlolra commented Sep 1, 2016

vqhuy commented Sep 1, 2016

masomel commented Sep 1, 2016

arlolra Sep 1, 2016

vqhuy Sep 1, 2016

arlolra Sep 1, 2016 •

edited

vqhuy Sep 7, 2016 •

edited

liamsi Sep 7, 2016

liamsi Sep 7, 2016 •

edited

masomel Sep 7, 2016

vqhuy Sep 8, 2016

arlolra Sep 8, 2016

vqhuy Sep 8, 2016 •

edited

liamsi commented Sep 7, 2016

masomel commented Sep 8, 2016

arlolra Sep 10, 2016

vqhuy Sep 13, 2016

arlolra Sep 14, 2016

vqhuy Sep 14, 2016 •

edited

arlolra Sep 14, 2016

arlolra commented Sep 14, 2016

vqhuy commented Sep 14, 2016

Remove Policies interface #78

Remove Policies interface #78

Conversation

vqhuy commented Sep 1, 2016

arlolra commented Sep 1, 2016

vqhuy commented Sep 1, 2016

masomel commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra Sep 1, 2016 • edited

Choose a reason for hiding this comment

vqhuy Sep 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Sep 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Sep 8, 2016 • edited

Choose a reason for hiding this comment

liamsi commented Sep 7, 2016

masomel commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Sep 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra commented Sep 14, 2016

vqhuy commented Sep 14, 2016

arlolra Sep 1, 2016 •

edited

vqhuy Sep 7, 2016 •

edited

liamsi Sep 7, 2016 •

edited

vqhuy Sep 8, 2016 •

edited

vqhuy Sep 14, 2016 •

edited