[WIP] DB backend hooks by vqhuy · Pull Request #37 · coniks-sys/coniks-go

vqhuy · 2016-07-27T02:20:18Z

Resolve Add hooks for storage backend #4
This pull uses a key-value db interface to store PAD, STR, Policies, MerkleTree. The storing data is formatted as the comments of StoreToKV methods.
All source files related to storage backend have postfix kv
The library provides APIs to reconstruct the PAD from the latest STR from the db (e.g., when the key server restarts), and to reconstruct a part of tree when looking through the db for specific epoch in lookup function.

* Part of #4

# Conflicts: # merkletree/pad.go # merkletree/pad_test.go

vqhuy · 2016-07-27T08:18:35Z

There're some issues with this db hooks implementation:

We allow the developers to supply their own policy which is implemented Policies interface (Allow the developer to supply her own policy #6). Thus, the developers have to use exactly the policies type that they have been used before to reconstruct the PAD or STR, for example. If the developers misused in this case, the reconstructed PAD or STR would break the hash chain.
When looking through the db for specific STR, if we want to construct the tree partially based on the lookup index, then we have to somehow reconstruct the policies first (to compute the lookup index). Thus, the developers also need to provide exactly the policies type which he is used in that epoch. It may lead to another issue of policies management?

I would like to have your advices @masomel @arlolra @liamsi !
Thanks!

masomel · 2016-07-27T18:12:17Z

Looking through the ac93761 code, I don't quite understand why the policies need to be passed into str.LoadFromKV() for the reconstruction. Maybe I'm missing something, but why can't policies.LoadFromKV() not just find the appropriate policy entry in the DB with policies := new(Policies) as is done with str? It seems that the policy.serializeKvKey() only needs the policy identifier and the epoch to find the policy entry in the DB. I think we can also add more mandatory fields to the policy interface, e.g. the LibVersion and the hashID to leave less room for developers to do weird things when they're customizing the policy. My idea for a developer-supplied policy was really more about allowing them to set the policy fields we give them to their own values, and less about letting them implement new policy types.

But coming back to your second point, could we give the developer the option to store the policy type itself in the DB?

Looking at the code, I'm also wondering if all of the Load() and Store() methods need to be interface methods, or if it would make more sense to have methods such as LoadStrFromKV(), LoadPolicyFromKV() etc and make these part of the kv package instead. Looking through the code, it seemed odd to me to have a str.LoadFromKV() call when the str isn't really anything yet.

vqhuy · 2016-07-28T01:07:00Z

My idea for a developer-supplied policy was really more about allowing them to set the policy fields we give them to their own values, and less about letting them implement new policy types.

It's much more clear. Thanks!

Maybe I'm missing something, but why can't policies.LoadFromKV() not just find the appropriate policy entry in the DB with policies := new(Policies) as is done with str?

Because the Policies is just an interface.

I think we can also add more mandatory fields to the policy interface, e.g. the LibVersion and the hashID to leave less room for developers to do weird things when they're customizing the policy.

Yes. I already did it in af4ea71051d25191b555d96820b3fe9d964fd219, but it doesn't belong to this branch. Hmm, I guess we should review and merge #35 first.

Looking at the code, I'm also wondering if all of the Load() and Store() methods need to be interface methods, or if it would make more sense to have methods such as LoadStrFromKV(), LoadPolicyFromKV() etc and make these part of the kv package instead.

Yes. I'm just waiting for you to say that. I would like to do it, too.

masomel · 2016-07-28T15:54:23Z

Maybe I'm missing something, but why can't policies.LoadFromKV() not just find the appropriate policy entry in the DB with policies := new(Policies) as is done with str?

Because the Policies is just an interface.

Ok, so how the Policies are set from the DB is determined by the specific Load() function of the interface. Got it.

Yes. I already did it in af4ea71, but it doesn't belong to this branch. Hmm, I guess we should review and merge #35 first.

Ok, I'll go take a look. Agreed, let's get that done first.

Yes. I'm just waiting for you to say that. I would like to do it, too.

Cool! Feel free to just suggest it next time if you would like to make changes like these :)

# Conflicts: # merkletree/policy.go

masomel · 2016-08-04T16:26:17Z

Looking at the code, I'm also wondering if all of the Load() and Store() methods need to be interface methods, or if it would make more sense to have methods such as LoadStrFromKV(), LoadPolicyFromKV() etc and make these part of the kv package instead.

I've been thinking more about this, and maybe we should move all *kv.go modules to the kv package.

vqhuy · 2016-08-04T16:37:44Z

I've been thinking more about this, and maybe we should move all *kv.go modules to the kv package.

Yup, totally agree. I'm doing it :)

vqhuy · 2016-08-08T12:02:46Z

I've been thinking more about this, and maybe we should move all *kv.go modules to the kv package.

I spent last few days to work on this. But it seems not to be feasible. If we want to go this way, we have to export fields that we really don't want to do. (e.g., see https://github.com/coniks-sys/coniks-go/blob/cc8b74db472f53a621ac89c05d41ba4121b891a1/merkletree/nodekv.go). What do you think?

masomel · 2016-08-08T22:06:44Z

I spent last few days to work on this. But it seems not to be feasible. If we want to go this way, we have to export fields that we really don't want to do.

Hmm, this is a good point. At least for node it seems like the main issue comes from the serialization in storeToKV() that needs access to those fields. Would it be possible to export Serialize() and Deserialize() functions that could be used in the KV module instead?

vqhuy · 2016-08-09T01:43:43Z

Would it be possible to export Serialize() and Deserialize() functions that could be used in the KV module instead?

But storeToKV uses a specific scheme of Serialize() and Deserialize(), thus we cannot have a general Serialize() and Deserialize() at all.

masomel · 2016-09-19T17:56:12Z

@c633 Is there anything I can help with here to get this moving again?

vqhuy · 2016-09-20T02:13:22Z

@masomel I'm working on this. I'm trying to implement it in several ways and do benchmark to compare them. Code will be pushed today.

masomel · 2016-09-20T15:17:43Z

I saw your email, I didn't realize you were benchmarking the various implementations. I don't want to rush you. But I have some time this week to help with coding, so if there was any task that you haven't gotten to yet and would want help on, I'd be available work on it.

vqhuy · 2016-09-20T20:07:54Z

But I have some time this week to help with coding, so if there was any task that you haven't gotten to yet and would want help on, I'd be available work on it.

Thanks! Maybe we can start reviewing this pull.

I tried to implement the encoding/decoding using encoding/gob (see branch kvdb). Here are the output of the Benchmarks of 2 implementations:

# db branch
⇒  go test -bench=BenchmarkStorePAD100K -benchmem -cpuprofile cpu.out -memprofile mem.out
BenchmarkStorePAD100K-4            1    2528018579 ns/op    1535363120 B/op  2461414 allocs/op
PASS
ok      github.com/coniks-sys/coniks-go/merkletree  93.763s

⇒  go test -bench=BenchmarkLoadPAD100K -benchmem -cpuprofile cpu.out -memprofile mem.out
BenchmarkLoadPAD100K-4             1    2135054242 ns/op    376952096 B/op   5977039 allocs/op
PASS
ok      github.com/coniks-sys/coniks-go/merkletree  96.130s

# kvdb branch (using gob)
⇒  go test -bench=BenchmarkStorePAD100K -benchmem -cpuprofile cpu.out -memprofile mem.out
BenchmarkStorePAD100K-4            1    6552359956 ns/op    3667490784 B/op 11409518 allocs/op
PASS
ok      github.com/coniks-sys/coniks-go/merkletree  98.109s

⇒  go test -bench=BenchmarkLoadPAD100K -benchmem -cpuprofile cpu.out -memprofile mem.out
BenchmarkLoadPAD100K-4             1    15619991406 ns/op   2927011456 B/op 69655546 allocs/op
PASS
ok      github.com/coniks-sys/coniks-go/merkletree  114.171s

So I think I will stick with this implementation and start re-organizing the source code (I still have no idea how to put *kv.go into a separate package).

masomel · 2016-09-20T20:49:42Z

 - ``client``: Client-side protocol cgo hooks
 - ``crypto``: Cryptographic algorithms and operations
 - ``merkletree``: Merkle prefix tree and related data structures
+- ``storage``: DB hooks for storage backend (currently the library supports key-value db only)


Seems like this is duplicate (see L27)

masomel · 2016-09-20T21:01:50Z

+	return m, nil
+}
+
+func (m *MerkleTree) reconstructTree(db kv.DB, parent MerkleNode, epoch uint64, prefixBits []bool) (MerkleNode, error) {


Is there a reason why reconstructTree needs to be a method of MerkleTree? None of m's fields are used in this function.

masomel · 2016-09-20T21:11:21Z

+		} else {
+			parent.(*interiorNode).leftChild = n
+		}
+		switch n.(type) {


I think this switch needs a default case to throw an error in case we encounter some malformed data.

masomel · 2016-09-20T21:14:13Z

+
+		wb := db.NewBatch()
+		m1.StoreToKV(1, wb)
+		err = db.Write(wb)


Is there a reason why db.Write(wb) isn't called from within m.StoreToKV()?

masomel · 2016-09-20T21:16:01Z

+
+		wb := db.NewBatch()
+		m1.StoreToKV(1, wb)
+		err = db.Write(wb)


Same as L34.

masomel · 2016-09-20T21:18:15Z

+		}
+
+		ap := m2.Get(index1)
+		if ap.Leaf.Value() == nil {


Is this the only condition that indicates that key1 cannot be found in the tree? I'm wondering if we should also be checking the leaf node type and the leaf's index here.

Same goes for L58, L105, and L120.

masomel · 2016-09-20T21:26:19Z

 }

-func (n *userLeafNode) isEmpty() bool {
+func (n *interiorNode) isEmpty() bool {


Any reason the node types were flipped between here and L156?

masomel · 2016-09-20T21:32:53Z

+
+func deserializeNode(buf []byte) MerkleNode {
+	switch buf[0] {
+	case InteriorNodeIdentifier:


This is a modularity comment: Might be worth moving each of these cases into a helper function to clean this function up.

masomel · 2016-09-20T21:38:25Z

+		wb := db.NewBatch()
+		enWant.storeToKV(1, []bool{false}, wb)
+		inWant.storeToKV(1, []bool{true}, wb)
+		lnWant.storeToKV(1, util.ToBits(lnWant.index)[:lnWant.level], wb)


Is util.ToBits(lnWant.index)[:lnWant.level] how the developer is going to have to call ln.storeToKV()? If so, I think the interface should abstract this detail away.

Same goes for loadNode() below in L94.

masomel

This is looking pretty good, though some of my comments do require changes. Should we also add db hooks for the TB?

masomel · 2016-09-20T21:59:15Z

+	// return nil
+	// }
+	// return str
+	return nil // util we have a better way to construct the tree partially based on the lookup index.


Is this comment still relevant? It seems like str.LoadFromKV() doesn't do anything tree-related.

masomel · 2016-09-20T22:34:10Z

I think I will stick with this implementation

The differences between the two benchmarks aren't huge, but I agree that the current implementation works fine.

I still have no idea how to put *kv.go into a separate package

I'm thinking it might make sense to structure the db hooks in a similar fashion to how we've structured our library: server calls protocol calls merkletree. So one possible solution would be to move all of the *kv.go files to a storage/kv/merkletree which imports merkletree, and create a higher-level storage/kv/protocol package that abstracts away the merkletree hooks. I think the only place where the developer should be aware of the DB is at the server level since it's not really part of the protocol or the data structure.

I also think that the hooks should only be for deserializing/loading and serializing/storing. If we don't make the serialization functions etc methods of the corresponding types, we also shouldn't have to add new interface methods (e.g. in policy), right? If this is possible, this should allow us to de-tangle the DB hooks from the actual data structure. Addditionally, if we remove the hooks in functions like GetSTR(), this will make the design a bit simpler, and a higher-level wrapper (say, at the protocol level) can decide to load from the db if GetSTR returns nil (as opposed to GetSTR needing to figure out that we've reached the limit of loaded STRs and load from the DB). Does this make sense?

vqhuy · 2016-09-21T04:49:26Z

The differences between the two benchmarks aren't huge, but I agree that the current implementation works fine.

I'm trying to optimize the gob-encoding implementation. Hopefully it can improve the benchmarks.

I'm thinking it might make sense to structure the db hooks in a similar fashion to how we've structured our library: server calls protocol calls merkletree. So one possible solution would be to move all of the *kv.go files to a storage/kv/merkletree which imports merkletree, and create a higher-level storage/kv/protocol package that abstracts away the merkletree hooks.

Maybe I got your idea. Thanks for your suggestions! It totally makes sense!

I think the only place where the developer should be aware of the DB is at the server level since it's not really part of the protocol or the data structure.

Yeah, it's right! It makes things clearer. Let's see if I can make this work.

vqhuy · 2016-09-22T10:33:39Z

Close and move to #88.

vqhuy and others added 14 commits July 25, 2016 02:25

Add key-value storage interface

134c6d7

Implement db storage

47f3999

* Part of #4

Reorganize KV-related code

48ecb95

* Part of #4

Reorganize repo

11afb2e

Add description into README

a9f5710

Fix imports & fix tests

e486e31

Merge branch 'master' into db

be7fbbb

# Conflicts: # merkletree/pad.go # merkletree/pad_test.go

Add storage interfaces for Policies

4248857

Rewrite node's storage interface & tests

41310aa

Fix import

2cf8bf2

Fix bugs and rewrite STR storage interfaces

7ee58c0

Implement PAD storage

26b661b

Rename to KV uppercase for consistency

1626e77

Look through persistent storage for STRs

c0c0c49

Use the policies type supplied by the developers instead

ac93761

This was referenced Jul 28, 2016

Allow the developer to supply her own policy #6

Open

Rename DefaultPolicies to ConiksPolicies #41

Merged

Merge branch 'master' into db

328d7dd

# Conflicts: # merkletree/policy.go

masomel force-pushed the master branch 3 times, most recently from 25a6925 to 2f704cf Compare July 29, 2016 20:03

vqhuy force-pushed the master branch from 4ccce23 to 6f72c64 Compare August 2, 2016 21:27

vqhuy changed the title ~~DB backend hooks~~ [WIP] DB backend hooks Aug 5, 2016

Merge branch 'master' into db

e7496e3

vqhuy force-pushed the db branch from 09cee8f to e7496e3 Compare August 8, 2016 09:38

vqhuy mentioned this pull request Aug 9, 2016

Implement registration function #29

Closed

masomel added this to the 0.1.0 milestone Sep 1, 2016

[WIP] Fix tests

c0f86c3

vqhuy force-pushed the db branch from cc8b74d to c0f86c3 Compare September 20, 2016 19:59

masomel reviewed Sep 20, 2016

View reviewed changes

masomel requested changes Sep 20, 2016

View reviewed changes

vqhuy mentioned this pull request Sep 21, 2016

Key-value DB using gob for encoding/decoding #88

Open

vqhuy closed this Sep 22, 2016

masomel deleted the db branch October 9, 2016 23:15

Conversation

vqhuy commented Jul 27, 2016

Uh oh!

vqhuy commented Jul 27, 2016

Uh oh!

masomel commented Jul 27, 2016

Uh oh!

vqhuy commented Jul 28, 2016

Uh oh!

masomel commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masomel commented Aug 4, 2016

Uh oh!

vqhuy commented Aug 4, 2016

Uh oh!

vqhuy commented Aug 8, 2016

Uh oh!

masomel commented Aug 8, 2016

Uh oh!

vqhuy commented Aug 9, 2016

Uh oh!

masomel commented Sep 19, 2016

Uh oh!

vqhuy commented Sep 20, 2016

Uh oh!

masomel commented Sep 20, 2016

Uh oh!

vqhuy commented Sep 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masomel Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masomel Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masomel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masomel commented Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vqhuy commented Sep 21, 2016

Uh oh!

vqhuy commented Sep 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

masomel commented Jul 28, 2016 •

edited

Loading

masomel Sep 20, 2016 •

edited

Loading

masomel Sep 20, 2016 •

edited

Loading

masomel commented Sep 20, 2016 •

edited

Loading