Merge cosi into cothority #665

liamsi · 2016-11-03T15:19:58Z

Early pull request to see if all the tests pass (currently, they do :-)
Resolves #650

Coding-wise this done (besides some nice-to-have things)

Open tasks for this pull:

remove duplicate code (reuse the methods in app/lib)
Search for references to the cosi repo in documentation
update documentation here and in the cosi repository accordingly
manually test how installing & deploying cosi changed due to merging the repo

All tests work (locally)

nikkolasg · 2016-11-03T16:52:09Z

app/cosi/server.go

+	os.Exit(1)
+}
+
+func getDefaultConfigFile() string {


Many of these functions are already implemented in app/lib/{client,server} if I remember correctly.

Thanks! Added to the TODO list ...

…ker.com/lCrcn5p2 for diff)

@ineiti

…nt to cothority repo (thx @ineiti)

liamsi · 2016-11-08T12:48:21Z

There seem to be different issues like deadlocks, timeouts, and data races. They seem unrelated to this pull. Although this pull is still marked as WIP, it can already be reviewed. What is left to do is only documentation and testing.

nikkolasg · 2016-11-08T15:58:44Z

protocols/cosi/cosi.go

+		return err
+	}
+
+	if c.responseHook != nil {


@ineiti could you push your changes that you did in #664 (snp17) here so it's compatible with randhound ?

What I did in snp17 for cosi.go is quite hackish. I wouldn't want to include that as-is here. Anyway, look at it and tell me if you want me to propose it:

https://github.com/dedis/cosi/blob/randhoundco/protocol/cosi.go#L275

Together with https://github.com/dedis/cosi/blob/randhoundco/protocol/cosi.go#L346-L354 I find it rather clear and not so hackish. But I also think, this is out-of scope of this pull. Let's see if @nikkolasg insists on pushing the changes.

Yeah Ok let's not do that :D

…ching the right dedis/cosi branch

liamsi · 2016-11-09T09:12:55Z

I tested this manually:
Doing go get -u github.com/cothority/app/cosiand running
$GOPATH/bin/cosi sign -g /Users/khoffi/Library/cosi/group.toml -o README.sig README.md
and $GOPATH/bin/cosi verify -g /Users/khoffi/Library/cosi/group.toml -s README.sig README.md still works as expected (OK, instead of go get -u I did go install ./app/cosi/ ... because the code necessary isn't in master, yet).

liamsi · 2016-11-09T09:36:16Z

Should be reviewed together with dedis/cosi#68

nikkolasg · 2016-11-09T10:08:02Z

From discussion with @liamsi there's still one refactoring to do regarding the "runServer" method which should be in app/lib/server. After that I'm OK to merge.

… from source (which should use the sources in cothority)

ineiti · 2016-11-09T13:37:14Z

.travis.yml

@@ -4,7 +4,7 @@ go:
  - 1.7

 install:
-  - ./install.sh
+  - go get -t ./...


You finally got rid of install.sh? Great!

Yeah :-) I think it was mostly there to get right branch in the other repo, which isn't necessary anymore.

Riiighhttt . that's so nice.

So coveralls.sh takes care of any dependencies with dedis/crypto?

No, only with cosi, which

should not be needed anymore
OR

should be used with crypto as @ineiti suggests

Let's keep it clean for now and say that if we want to do a R4M, we need to have crypto updated first...

nikkolasg · 2016-11-09T13:42:40Z

I'm OK to merge.

ineiti

README.md comments

ineiti · 2016-11-09T13:37:36Z

README.md

@@ -66,23 +66,21 @@ from time to time, as all dedis-dependencies change quite often.
 There are three apps available:

 * [cothorityd](https://github.com/dedis/cothority/app/cothorityd) - which is the server-part that you can run to add a node
-* [cosi](https://github.com/dedis/cosi) - the cosi-protocol, service, and app,
-in its own repository
+* [cosi](https://github.com/dedis/cothority/app/cosi) - the cosi-app


I propose we keep CoSi capitalisation.

You mean not keep but instead change to CoSi, right?

ineiti · 2016-11-09T13:39:11Z

app/cosi/README.md

@@ -0,0 +1,338 @@
+[![Build Status](https://travis-ci.org/dedis/cothority.svg?branch=master)](https://travis-ci.org/dedis/cothority)


I started to update the wiki on a non-regular basis - should this be kept as a README or kept only in the wiki at https://github.com/dedis/cothority/wiki/CoSi ?

~~The Readme is the first thing people see (not the Wiki). I propose to keep this information in the Readme.~~ OK, I see what you mean now. The other packages also contain Readmes like this... I'm OK with both options but it should be consistent.

I'm OK with having the instructions in the Wiki and a link pointing to the wiki in the app/cosi/README.md. We could do that for all apps, so we have a central place where all instructions / docs can be read.

Or having the wiki point to the README for specific instructions? Anyway, that's probably also a later issue.

ineiti · 2016-11-09T13:39:39Z

app/cosi/README.md

+The signatures that CoSi produces convey the same information
+as a list of conventional signatures from all participants,
+but CoSi signatures are far more compact and efficient for clients to verify.
+In practice, a CoSi collective signature is close to the same size as &mdash;


can we replace — with -?

Of course, I don't know where this came from (must have been already there)

ineiti · 2016-11-09T13:41:16Z

app/cosi/README.md

+## Installing from Source
+
+To build and run CoSi from source code you will need to install
+[Go](https://golang.org/) version 1.5.2 or later.


I'm not sure it compiles with 1.5.2 due to the Context. Did you verify that?

No, I don't have 1.5.2 installed anymore. We could let travis do runs with every version we support, or I can simply change it to 1.7 instead.

+1 for 1.6 or 1.7 (there's a PR for going to 1.7 that @liamsi started, I dont know what's the status of it)

I dont know what's the status of it

There were problems in medco with 1.7. But medco was delelted, so ... It should be fine!

ineiti · 2016-11-09T13:42:28Z

app/cosi/README.md

+
+For convenience we provide self-contained x86-64 binaries
+for Linux and Mac OS X.
+[Download the latest release](https://github.com/dedis/cosi/releases/latest),


I'd remove the binary-release for the moment and only keep this section in the v0-branch at dedis/cosi.

OK! Makes sense (as the binaries are referenced in the cosi-repo Readme)

ineiti · 2016-11-09T13:43:56Z

app/cosi/README.md

+mac systems. If CoSi did not find anything, the default mechanism is to search in the current
+directory.
+
+Once you have a valid group definition, you can sign a file using the `cosi sign’


Wrong backtick after cosi sign

Interesting, I didn't even change that. Fixed in next commit.

ineiti · 2016-11-09T13:44:50Z

app/cosi/README.md

+before signing off on them,
+e.g., to validate a [collectively signed blockchain](http://arxiv.org/abs/1602.06997).
+
+## Running Your Own CoSi Witness Server


This can reference to a more general How to run a cothorityd.

This section already exists, here: https://github.com/dedis/cothority/blob/master/app/cothorityd/README.md

Again, is there already a wiki page for that ?

I think the decision where we will put all current and future documentation is orthogonal to this pull. Let's discuss this in a separate issue (if we want to remove all Readme files from sub-packages and use the wiki instead).

yes, but you can remove information that is found elsewhere.

ineiti · 2016-11-09T13:45:31Z

app/cosi/README.md

+you can concatenate their individual `group.toml` outputs
+to define your own cosigning group.
+You may optionally use any or all of our experimental
+[default CoSi servers](https://github.com/dedis/cosi/blob/master/dedis_group.toml)


Again - this should reference our new (not yet existing) group.toml.

Maybe this can wait until it actually exists?

So we should remove them (the links) for the time being then, since it does not exists yet.

ineiti · 2016-11-09T13:46:05Z

app/cosi/README.md

+Description = "My Test group"
+
+[[servers]]
+  Addresses = ["127.0.0.1:2000"]


I think the default-ports are different from 2000, can you take those?

Why? 2000 also works (locally) without a problem and we use it all over the place. But yes, I can do it (DefaultPort is 6879).

This isn't related to this pull but I would like to keep it consistent: In the cothorityd package the ports in the Readme are the same.

OK, I'll change those later.

ineiti · 2016-11-09T13:46:25Z

app/cosi/README.md

+```bash
+cat > servers.toml <<EOF
+
+[[servers]]


Take the new, not-yet-existing, group.toml.

Maybe this can wait until it actually exists?

ineiti

Not all relevant, but nice to have any way...

ineiti · 2016-11-09T13:49:05Z

app/cosi/client.go

+// it always returns nil as an error
+func signFile(c *cli.Context) error {
+	if c.Args().First() == "" {
+		printErrAndExit("Please give the file to sign", 1)


log.Fatal

ineiti · 2016-11-09T13:49:24Z

app/cosi/client.go

+	groupToml := c.String(optionGroup)
+	file, err := os.Open(fileName)
+	if err != nil {
+		printErrAndExit("Couldn't read file to be signed: %v", err)


and all further references to printErrAndExit: log.Fatal

Done in next commit

ineiti · 2016-11-09T13:49:42Z

app/cosi/client.go

+		printErrAndExit("Couldn't read file to be signed: %v", err)
+	}
+	sig, err := sign(file, groupToml)
+	printErrAndExit("Couldn't create signature: %v", err)


log.ErrFatal

Done in next commit

ineiti · 2016-11-09T13:50:54Z

app/cosi/client.go

+	if len(c.Args().First()) == 0 {
+		printErrAndExit("Please give the 'msgFile'", 1)
+	}
+	log.SetDebugVisible(c.GlobalInt("debug"))


Shouldn't that go somewhere in the initialisation, like in https://github.com/dedis/cothority/blob/master/app/cisc/cisc.go#L57

There is already an app.Before, so you can delete this line.

Didn't add that ... Deleted in next commit

ineiti · 2016-11-09T13:51:04Z

app/cosi/client.go

+// verifyPrintResult prints out OK or what failed.
+func verifyPrintResult(err error) {
+	if err == nil {
+		fmt.Println("[+] OK: Signature is valid.")


log.Print

ineiti · 2016-11-09T13:56:33Z

app/test_cothorityd.sh

@@ -53,7 +53,7 @@ build(){
    cd $DIR
    echo "Building in $DIR"

-    for appdir in $BUILDDIR/cothorityd $GOPATH/src/github.com/dedis/cosi; do
+    for appdir in $BUILDDIR/cothorityd $GOPATH/src/github.com/dedis/cothority/app/cosi; do


for appdir in $BUILDDIR/cothorityd $BUILDDIR/cosi; do

ineiti · 2016-11-09T13:56:54Z

coveralls.sh

@@ -7,16 +7,6 @@ DIR_SOURCE="$(find . -maxdepth 10 -type f -not -path '*/vendor*' -name '*.go' |
 BRANCH=$TRAVIS_PULL_REQUEST_BRANCH
 echo "Using branch $BRANCH"

-pattern="refactor_*"


Would we want to do this to crypto anyway?

I thought we only use this for cosi & cothority; was there a case where I missed that ( cc @nikkolasg )

As I wrote earlier, we can say that any R4M into master that touches crypto needs to get the crypto-part merged first.

Makes more sense to me.

ineiti · 2016-11-09T13:58:43Z

protocols/cosi/cosi.go

+
+// Dispatch will listen on the four channels we use (i.e. four steps)
+func (c *CoSi) Dispatch() error {
+	for {


Probably for later: this should be more something like in https://github.com/dedis/cothority/blob/master/protocols/bftcosi/bftcosi.go#L182 where the non-for-loop channels impose an order on the messages. As this is a protocol that is looked at by grad students, it'll be nice to have it somewhat canonical.

OK, we should open an issue (or add it to some refactoring issue)

ineiti · 2016-11-09T14:00:06Z

protocols/cosi/cosi.go

+	out := &Challenge{
+		Chall: challenge,
+	}
+	log.Lvl3(c.Name(), "Starting Chal=", fmt.Sprintf("%+v", challenge), " (message =", string(c.Message))


log.Lvlf3("%s Starting Chal=%+v (message =%x)", c.Name(), challenge, string(c.Message))

ineiti · 2016-11-09T14:00:45Z

protocols/cosi/cosi.go

+// handleChallenge dispatch the challenge to the round and then dispatch the
+// results down the tree.
+func (c *CoSi) handleChallenge(in *Challenge) error {
+	log.Lvl3(c.Name(), "chal=", fmt.Sprintf("%+v", in.Chall))


log.Lvl3("%s chal=%+v", c.Name(), in.Chall)

I guess you mean log.Lvlf3 -> done

liamsi added 3 commits October 17, 2016 15:57

WIP: bringing cosi back home (first commit)

bbbc7f4

WIP: bringing cosi back home (first commit) see #650

6ac2341

WIP: bringing cosi back home

cbf4939

All tests work (locally)

nikkolasg reviewed Nov 3, 2016

View reviewed changes

ineiti assigned liamsi Nov 4, 2016

liamsi force-pushed the merge_cosi branch from 492df64 to c5e0dc8 Compare November 6, 2016 17:07

WIP: reduce duplicate code

692ca2f

liamsi force-pushed the merge_cosi branch from c5e0dc8 to 692ca2f Compare November 6, 2016 22:03

liamsi added 3 commits November 8, 2016 10:19

WIP: remove more duplicate code

8ceb9d3

WIP: remove more duplicate code (tryConnect; see https://www.diffchec…

630e83e

…ker.com/lCrcn5p2 for diff)

WIP: and remove even more duplicate code

dca6efb

liamsi force-pushed the merge_cosi branch from 2baa0f0 to dca6efb Compare November 8, 2016 09:56

liamsi added 2 commits November 8, 2016 11:44

Started cleaning comments and docs

67f1bab

Remove unused scripts from cosi, move integration tests for cosi-clie…

b818a08

…nt to cothority repo (thx @ineiti)

nikkolasg reviewed Nov 8, 2016

View reviewed changes

Some documentation, remove special install.sh solely dealing with fet…

e8c7014

…ching the right dedis/cosi branch

liamsi changed the title ~~WIP: Merge cosi into cothority~~ Merge cosi into cothority Nov 8, 2016

liamsi added 2 commits November 8, 2016 18:48

simplified methods and added FIXME comments

6dbcb82

fixed some documentation

1a9f3f5

liamsi mentioned this pull request Nov 9, 2016

Merge cosi dedis/cosi#68

Merged

nikkolasg closed this Nov 9, 2016

nikkolasg reopened this Nov 9, 2016

liamsi added 3 commits November 9, 2016 13:24

Added hint to cosi's Readme and removed references to master/building…

713745e

… from source (which should use the sources in cothority)

Updated cothority and cosi readme

5e1f951

@nikkolasg's comments & some cleanup

ed91f96

ineiti reviewed Nov 9, 2016

View reviewed changes

liamsi added 2 commits November 9, 2016 15:11

review comments

18e85f4

more comments

2532884

ineiti reviewed Nov 9, 2016

View reviewed changes

liamsi added 5 commits November 9, 2016 15:42

more review comments

17b9a75

Use log instead of fmt

125cd61

Fix build errors

1c812ee

Increment version

1498883

dedis servers (group-toml)

60291df

ineiti merged commit 2603180 into master Nov 9, 2016

ineiti deleted the merge_cosi branch November 9, 2016 15:30

		@@ -0,0 +1,338 @@
		[![Build Status](https://travis-ci.org/dedis/cothority.svg?branch=master)](https://travis-ci.org/dedis/cothority)

Merge cosi into cothority #665

Merge cosi into cothority #665

Conversation

liamsi commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Nov 8, 2016

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 Nov 9, 2016 • edited Loading

liamsi commented Nov 9, 2016

nikkolasg commented Nov 9, 2016

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

nikkolasg commented Nov 9, 2016

ineiti 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 Nov 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Nov 9, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ineiti 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

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

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

Choose a reason for hiding this comment

liamsi commented Nov 3, 2016 •

edited

Loading

liamsi commented Nov 9, 2016 •

edited

Loading

liamsi Nov 9, 2016 •

edited

Loading

liamsi Nov 9, 2016 •

edited

Loading