Custom verification function for Log entries #123

fazo96 · 2018-04-10T14:54:26Z

Closes #117

Content:

small refactor of how entry verification is done to allow multiple different verifications (for now signature verification and custom verification)
setter to allow setting a custom verification function in the log
updated existing tests. I had to switch all join calls to await because before these changes, if the log was not signed there were no async operations in the join. However, the way I structured the code now there is always an async call to verifyEntries that makes join always happen asynchroniously. This broke many tests
docs
tests to see that the custom verification is invoked correctly

@haadcode I'm not good at naming stuff at all, so tell me if you want me to rename anything or change the way the custom verification function is passed.

I also saw some TODOs about moving verification stuff from the Log to the Entry. Maybe I can fit that in this PR

fazo96 · 2018-04-11T08:30:45Z

Updated with a test and docs. I noticed the replication test is failing for me even without my edits

haadcode · 2018-04-12T09:06:22Z

Thank you @fazo96 for the PR! Looks great and sounds like all tests are passing as expected. I quickly ran the tests yesterday against OrbitDB and they're all green.

Will try to give some feedback on the code later this week but by the quick looks, it's fine.

haadcode · 2018-04-12T09:12:48Z

src/log.js

+   * 
+   * @memberof Log
+   */
+  set customVerificationFunction (verifyEntry) {


Let's pass in the verify function through constructor options as we do with other "dependencies"? This way the user doesn't need to do log.customVerificationFunction = () ... afterwards. I feel that's an easier (safer?) way to do it. What do you think?

Good idea, but then if the log is part of a store how do you pass the custom verification function? Maybe we should let users pass it both through the setter and the constructor. Or we can pass it through the options when store/db is created and have it passed down to the log

I feel we should do the latter, ie. pass it to Store and then have Store pass it to the log.

I tried adding a constructor argument, but in the tests for examples many Logs are created using Log.fromEntry or other factory functions like that.

I'm not sure what the best approach is, do you think we should add this option to every factory function? I ended up keeping the setter as well as the constructor argument for now.

I also noticed there was some weird jsdoc syntax before the constructor, I changed it, let me know if that works

@haadcode ping

Hey sorry for the delay!

You're absolutely right, we need to add the verification function passing to the factory functions too in order to make this work. Fortunately it's only three places (afaict):

https://github.com/orbitdb/ipfs-log/blob/master/src/log.js#L446

https://github.com/orbitdb/ipfs-log/blob/master/src/log.js#L463

https://github.com/orbitdb/ipfs-log/blob/master/src/log.js#L480

I would add it as the last argument (like you did in the constructor).

(And on that note, I feel we should generally pass the various "options" through an options object and not individual argument, but that's something for future, not for this PR)

I still think we should not add a setter but rather pass it in through function calls in order to avoid accidental usage/mutation (note that we don't use setter anywhere in the code).

What do you think?

Sounds like the best solution, I'll update the PR today 👍 Thanks for pointing out the factory functions

haadcode · 2018-04-12T09:18:14Z

test/entry-io.spec.js

@@ -128,15 +128,15 @@ apis.forEach((IPFS) => {
        if (i % 10 === 0) {
          log2 = new Log(ipfs, log2.id, log2.values, log2.heads)
          await log2.append('hi' + i)
-          log2.join(log)
+          await log2.join(log)


👍 to all these, they should've been async in the first place :) Thank you!

haadcode · 2018-04-12T09:24:15Z

Was able to do a quick review now. Looking good! I think we should pass the verification function in the constructor is my biggest feedback, so let me know what you think!

Thanks! ❤️ 😄

haadcode · 2018-04-20T05:38:00Z

test/log.spec.js

+        } catch (e) {
+          console.error(e)
+        }
+        // log B (containing entries from items3) not joined due to it containing an invalid entry


Great that you added a test for this! ❤️

fazo96 · 2018-04-20T10:20:44Z

@haadcode PR updated with all the things! I think it's ready for merging now

Unrelated things:

replication tests aren't passing for me, is this a known issue? They don't pass even without this PR's commits so it's not introduced here
I agree that using an options object for optional arguments would be best, but is not in scope here
It'd be great to get this into orbit-db 0.20 ⭐

haadcode · 2018-04-22T05:23:24Z

src/log.js

+   * to the log itself and the verification function should return true
+   * if the entry is valid and false if it is not. Logs containing entries
+   * marked as invalid by this function will not be joined
+   * 


This is great documentation! Thanks for adding it 👍

haadcode · 2018-04-22T05:31:57Z

src/log.js

+   * 
+   * @memberof Log
+   */
+  setCustomVerificationFunction (verifyEntry) {


What I meant with my previous comment about "no setter" was that we shouldn't have a function/setter at all that the user should/can call to set the verification function AFTER creating the Log instance, but that the only way to use a custom verifier is to pass it through the constructor (at creation time). Adding a new function to do that changes the API and doesn't fit the general idea of the design of the API as "operations on the log" as it changes the behaviour of the instance "dynamically". However, if we pass it in the constructor, it's explicit to the user and usage (ie. the only way to use "custom behaviour" is to pass it at instance creation time). Does that make sense?

If that makes sense, let's remove this function (but keep the documentation, it's awesome! perhaps move it to the constructor docs) as to not introduce it to the API of the log and we're good to merge 👍

Sorry for being picky about this one, I believe it's an important design decision even though it may be a seemingly small issue :) You've done fantastic job on the PR! 👍😃

No problem @haadcode, I just misunderstood your comment about no setter 😄 I can see now that as you said, there are no other functions that make the Log a dynamically changeable structure so there is no place for this one either.

haadcode · 2018-04-22T05:36:18Z

Added one more comment. Looking very good and close being able to merge. Regarding your other comments:

replication tests aren't passing for me, is this a known issue? They don't pass even without this PR's commits so it's not introduced here

Tests on mater seem to run, so not sure why they're not running for you. I'll take a look and see what I can find.

I agree that using an options object for optional arguments would be best, but is not in scope here

Let's put that on the long list of "to-refactor at some point" :) 👍

It'd be great to get this into orbit-db 0.20

Yup, agreed! Will hold off releasing 0.20.0 until this is finished and we can pull it in.

haadcode · 2018-04-22T05:43:16Z

replication tests aren't passing for me, is this a known issue? They don't pass even without this PR's commits so it's not introduced here

Tests on mater seem to run, so not sure why they're not running for you. I'll take a look and see what I can find.

I just ran the tests locally and they seem to run fine. I also noticed that we don't have CI on for forked PRs which I enabled now and I believe if you commit to this branch again, CI will run automatically and let you know if the tests run.

If you're seeing some error for the tests locally, can you pass the errors/log and we'll take a look at what's going on?

haadcode · 2018-04-22T05:45:21Z

Oh, one more thing before merge: can you squash the commit into one and add the individual commit descriptions into that one commit?

fazo96 · 2018-04-23T09:35:59Z

@haadcode squashed the commits, but circle failed the tests. Looks like it failed with the same identical problem I get on my machine

If you can, try triggering a build on master by circle. It should fail the replication test with the same error, like on my machine

haadcode · 2018-04-24T04:34:11Z

@fazo96 thanks for squashing it! One more thing to change (see the comment re. the setter function) and we're ready to merge :)

As for the tests, I re-triggered CI for this PR and it's now passing. Same for master. However, from the previous failed build, I can see how it fails:

replicates logs deterministically
Waiting for peers...
Found peers, running tests...

> Buffer1: 127 - Buffer2: 128
All messages sent
Waiting for all to process

> Buffer1: 128 - Buffer2: 129
1) replicates logs

Which sounds to me like a possible race condition in pubsub. Buffer1 and Buffer2 should both be 128, so there's one extra message sent... Really not sure how to catch this (and not something we need to figure out for this PR). @fazo96 does this fail on your computer on every run or just some times?

fazo96 · 2018-04-24T08:06:45Z

Removed the setter and moved the docs into the constructor 👍

The replication test always failed for me, but the numbers change so it's probably some kind of nasty race condition

* added docs and tests for custom verification * fix sync join calls in replication test * added verifyEntry constructor argument * add verifyEntry param to Factory functions. Don't use setter for it

aphelionz · 2019-01-22T20:10:34Z

@fazo96 @haadcode In the months since this PR, Iv'e fixed the replication tests. It would be great to rekindle this and get it up to date with master

haadcode · 2020-03-31T07:15:45Z

Closing the PR as obsolete.

The custom sorting function was implemented and can be passed in to a log through the constructor as the sortFn parameter. See usage examples in https://github.com/orbitdb/ipfs-log/blob/ba2ec2d1c1b07728fab851efe431aa62c95fe8b1/test/log-load.spec.js#L651.

If you feel this PR was not addressed and solved, feel free to re-open it 👍

fazo96 mentioned this pull request Apr 10, 2018

OrbitDB validation and Index improvements ChluNetwork/chlu-ipfs-support#76

Closed

15 tasks

haadcode reviewed Apr 12, 2018

View reviewed changes

haadcode reviewed Apr 20, 2018

View reviewed changes

fazo96 changed the title ~~[WIP] Custom verification function for Log entries~~ Custom verification function for Log entries Apr 20, 2018

haadcode reviewed Apr 22, 2018

View reviewed changes

fazo96 force-pushed the custom-verification branch from 57185c5 to f655d87 Compare April 23, 2018 09:27

fazo96 force-pushed the custom-verification branch from f655d87 to 46a1848 Compare April 24, 2018 08:05

implement custom verification function for Log entries

46a1848

* added docs and tests for custom verification * fix sync join calls in replication test * added verifyEntry constructor argument * add verifyEntry param to Factory functions. Don't use setter for it

coyotespike mentioned this pull request Jun 2, 2018

Custom verification and custom signing #127

Closed

haadcode mentioned this pull request Jun 25, 2018

Refactor access controller #128

Closed

thiagodelgado111 mentioned this pull request Jul 10, 2018

WIP: Move signing and verification implementation out of ipfs-log #131

Closed

11 tasks

thiagodelgado111 mentioned this pull request Aug 10, 2018

Custom signing, verification and permission checking for at the log #145

Closed

9 tasks

thiagodelgado111 mentioned this pull request Aug 30, 2018

Custom signing, verification and permission checking at the log #159

Merged

2 tasks

haadcode closed this Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom verification function for Log entries #123

Custom verification function for Log entries #123

fazo96 commented Apr 10, 2018 •

edited

Loading

fazo96 commented Apr 11, 2018

haadcode commented Apr 12, 2018 •

edited

Loading

haadcode Apr 12, 2018

fazo96 Apr 12, 2018

haadcode Apr 12, 2018

fazo96 Apr 12, 2018

fazo96 Apr 19, 2018

haadcode Apr 20, 2018

fazo96 Apr 20, 2018

haadcode Apr 12, 2018

haadcode commented Apr 12, 2018

haadcode Apr 20, 2018

haadcode Apr 20, 2018

fazo96 commented Apr 20, 2018

haadcode Apr 22, 2018

haadcode Apr 22, 2018

fazo96 Apr 24, 2018 •

edited

Loading

haadcode commented Apr 22, 2018

haadcode commented Apr 22, 2018

haadcode commented Apr 22, 2018

fazo96 commented Apr 23, 2018

haadcode commented Apr 24, 2018

fazo96 commented Apr 24, 2018

aphelionz commented Jan 22, 2019

haadcode commented Mar 31, 2020

Custom verification function for Log entries #123

Custom verification function for Log entries #123

Conversation

fazo96 commented Apr 10, 2018 • edited Loading

fazo96 commented Apr 11, 2018

haadcode commented Apr 12, 2018 • 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

Choose a reason for hiding this comment

haadcode commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fazo96 commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fazo96 Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

haadcode commented Apr 22, 2018

haadcode commented Apr 22, 2018

haadcode commented Apr 22, 2018

fazo96 commented Apr 23, 2018

haadcode commented Apr 24, 2018

fazo96 commented Apr 24, 2018

aphelionz commented Jan 22, 2019

haadcode commented Mar 31, 2020

fazo96 commented Apr 10, 2018 •

edited

Loading

haadcode commented Apr 12, 2018 •

edited

Loading

fazo96 Apr 24, 2018 •

edited

Loading