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

vqhuy · 2016-09-21T09:58:20Z

I have made an improvement in commit 84fc324bc27fee50c0619a9538c0d91421b804d1. This implementation's performance is better than #37 now.

Update (description below)

This pull uses encoding/gob to encode/decode the merkletree structs to/from a buffer, and it implements the db backend hook for the merkletree package.

The package merkletree is now exporting some functions for encoding/decoding the PAD, STR, Merkletree and Policies from a buffer (any types that implement the io.Writer and io.Reader interface). This way, I think we can hide the detail implementations of the encoding/decoding from the developers.

Move to the hook implementation, there are two cases when we want to use a persistent storage:

Store the current directory (pad).
Store the old STRs.

This hook exports APIs for store/load the PAD, STR to/from the db.

liamsi

Looking good. I added a few questions. Hope to do a more detailed review before our hangouts session later.

liamsi · 2016-09-22T10:15:55Z

storage/kv/merkletreekv/storage.go

+	STRIdentifier          = 'S'
+	PoliciesIdentifier     = 'P'
+	EpochIdentifier        = 'O'
+	TreeNonceIdentifier    = 'T'


Arent' those constants the same as in https://github.com/coniks-sys/coniks-go/blob/84fc324bc27fee50c0619a9538c0d91421b804d1/merkletree/storage.go ?

Oh, this is a old commit that has been deleted.

liamsi · 2016-09-22T10:20:38Z

merkletree/node.go

+	Key        string
+	Value      []byte
+	Index      []byte
+	Commitment *crypto.Commit


I guess all this values need to public to correctly encode now?

Yes, since we didn't export the structs so I think it still ensures our preservation, i.e., there's no way to change or read the node's fields.
Btw, the reason is (mostly) because I didn't want to implement the GobEncode/GobDecode interface manually.

liamsi

Some suggestions/questions.

liamsi · 2016-09-22T13:55:42Z

merkletree/policy.go

@@ -30,8 +33,6 @@ func NewPolicies(epDeadline TimeStamp, vrfPrivKey vrf.PrivateKey) *Policies {
 	}
 }

-// Serialize encodes the policy to a byte array with the following format:
-// [lib version, cryptographic algorithm in use, epoch deadline, vrf public key]


Why is the comment not relevant anymore?

liamsi · 2016-09-22T14:00:22Z

merkletree/merkletree.go

-	EmptyBranchIdentifier = 'E'
-	LeafIdentifier        = 'L'
+	EmptyNodeKey = 'E'
+	LeafNodeKey  = 'L'


Nitpicking: We would need this naming to be consistent. I liked the identifier (we already have so many different "keys" ;-) Here, we still stick with identifier: https://github.com/coniks-sys/coniks-go/pull/88/files#diff-7e689735c327b0bd737cd6e956e20b4aR4
Anyways, choose what you prefer. I just think it should be consistent.

Oops, it must be my mistake. I have no intention to change this! Thanks!

liamsi · 2016-09-22T14:01:48Z

storage/kv/merkletreekv/strkv.go

+	return merkletree.DecodeSTR(buff)
+}
+
+func STRKey(epoch uint64) []byte {


This could be unexported or not? If this should stay exported it would need a more speaking name.

Do you have any suggestion for this? :D

If it is unexported the name is OK.

liamsi · 2016-09-22T14:52:31Z

storage/kv/merkletreekv/strkv.go

+	"github.com/coniks-sys/coniks-go/utils"
+)
+
+func StoreSTR(wb kv.Batch, str *merkletree.SignedTreeRoot) error {


This and LoadSTR currently aren't used. I guess this will change?

They would be used to load the old STRs which were deleted from the pad.snapshot.

Cool, thanks. Can we have a test for that?

liamsi · 2016-09-22T14:59:44Z

From my point of view this PR looks very good (just a few questions and nitpicks).

vqhuy · 2016-09-22T15:00:59Z

Thanks!
Update: I made some changes in e8e9169c14efeb33810e1053b8f92d9a160d691e according to @liamsi's comments.

masomel

The encoding, decoding and reconstruction functions aren't part of the core data structure, they are only relevant to the DB, and they are ultimately only used in EncodePAD, which is really only used by StorePAD in padkv.go. As such, all gob encoding and decoding functions related to the DB should be part of the storage/kv/merkletreekv package, not the core merkletree package.

I agree that the encoding format is an implementation detail we should hide from the developer, but I don't see the reason why these functions need to be part of the core data structure if they are used only for a separate feature (the DB) of coniks-go.

vqhuy · 2016-12-21T14:31:25Z

PTAL. I need to use this in extensions implementation ;)

- Export fields of nodes struct for gob encoding - Add gob encoding/decoding for PAD, STR, MerkleTree, Policies (use internally)

vqhuy added this to the 0.1.0 milestone Sep 21, 2016

vqhuy self-assigned this Sep 21, 2016

vqhuy added the storage-backend label Sep 21, 2016

vqhuy force-pushed the kvdb branch 2 times, most recently from 84fc324 to 0203249 Compare September 22, 2016 10:32

vqhuy mentioned this pull request Sep 22, 2016

[WIP] DB backend hooks #37

Closed

liamsi reviewed Sep 22, 2016

View reviewed changes

vqhuy force-pushed the kvdb branch 2 times, most recently from ecf430d to 28a53ac Compare September 22, 2016 12:12

liamsi requested changes Sep 22, 2016

View reviewed changes

liamsi reviewed Sep 22, 2016

View reviewed changes

vqhuy mentioned this pull request Oct 6, 2016

Add hooks for storage backend #4

Open

masomel requested changes Oct 9, 2016

View reviewed changes

liamsi previously approved these changes Oct 16, 2016

View reviewed changes

masomel force-pushed the master branch from 46a16c4 to 058c6fd Compare October 25, 2016 19:05

vqhuy force-pushed the kvdb branch from e0c629a to cf2b790 Compare October 27, 2016 09:32

masomel modified the milestones: 0.2.0, 0.1.0 Nov 3, 2016

vqhuy force-pushed the kvdb branch from cf2b790 to 4d64d8f Compare November 16, 2016 06:26

vqhuy force-pushed the kvdb branch from 4d64d8f to b1bb05d Compare December 21, 2016 14:28

vqhuy requested a review from arlolra December 21, 2016 14:30

vqhuy force-pushed the kvdb branch 3 times, most recently from 74739cb to 91590e5 Compare December 21, 2016 14:40

Add db backend hook for merkletree package

21709e9

- Export fields of nodes struct for gob encoding - Add gob encoding/decoding for PAD, STR, MerkleTree, Policies (use internally)

vqhuy force-pushed the kvdb branch from 91590e5 to 21709e9 Compare December 25, 2016 10:20

vqhuy added this to the 0.3.0 milestone Apr 12, 2017

vqhuy removed this from the 0.2.0 milestone Apr 12, 2017

vqhuy dismissed liamsi’s stale review via 21709e9 June 12, 2023 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vqhuy commented Sep 21, 2016 •

edited

Loading

liamsi left a comment

liamsi Sep 22, 2016

vqhuy Sep 22, 2016

liamsi Sep 22, 2016

vqhuy Sep 22, 2016

liamsi left a comment

liamsi Sep 22, 2016

liamsi Sep 22, 2016

vqhuy Sep 22, 2016

liamsi Sep 22, 2016 •

edited

Loading

vqhuy Sep 22, 2016

liamsi Sep 22, 2016

liamsi Sep 22, 2016

vqhuy Sep 22, 2016

liamsi Sep 22, 2016

vqhuy Sep 22, 2016

liamsi commented Sep 22, 2016

vqhuy commented Sep 22, 2016 •

edited

Loading

masomel left a comment

vqhuy commented Dec 21, 2016

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

Are you sure you want to change the base?

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

Conversation

vqhuy commented Sep 21, 2016 • edited Loading

liamsi left a comment

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

Choose a reason for hiding this comment

liamsi left a comment

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

liamsi Sep 22, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Sep 22, 2016

vqhuy commented Sep 22, 2016 • edited Loading

masomel left a comment

Choose a reason for hiding this comment

vqhuy commented Dec 21, 2016

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

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

vqhuy commented Sep 21, 2016 •

edited

Loading

liamsi Sep 22, 2016 •

edited

Loading

vqhuy commented Sep 22, 2016 •

edited

Loading