Add missing -race flag to build-race make target

[Vervious]

we were compiling without golang race detector where we intended the opposite.

[zeldovich]

Thanks for catching this -- the -race flag got lost in some Makefile shuffling!

It looks like this now catches some races in kmd..

[tsachiherman]

The change is good, unfortunately, it fails during the unit tests. You might need to fix these before submitting this fix. ( if you feel that it goes beyond the scope of this task, please open a separate tickets for each individual failure )

[Vervious]

(working on the races)

[zeldovich]

If this is helpful -- I started looking at the failures that show up with this patch, and got as far as tracking at least one of the failures down to kmd's exit timeout, and some strange interaction with the race detection binaries.

Specifically, I get a quick reproducible failure on my laptop when I change defaultKMDTimeoutSecs = 1 in libgoal/libgoal.go and then running NODEBINDIR=$HOME/go/bin-race go test -v -run=TestAccountsCanSendMoney\$ ./test/e2e-go/features/transactions/

[Vervious]

(this might not be the only race - I haven't rerun the tests - but it's at least one race)

The other change is motivated by me accidentally crashing my machine running the e2e tests, since failures weren't cleaning up the dozens of algod processes that got spawned, which stole my cpu.

(as a whole, still wip)

[Vervious]

Ok - this still doesn't fix the tests. (Also, thank you Nickolai for the reproducible tip! Very helpful so far). Still investigating.

[Vervious]

It appears that doing something like

package main

import (
"fmt"
"golang.org/x/crypto/scrypt"
)

func main()  {
    password := []byte{10, 145, 184, 122, 252, 127, 236, 174, 244, 174, 64, 102, 14, 68, 244, 45, 43, 201, 124, 195, 192, 242, 69, 245, 67, 155, 71, 96, 15, 245, 243, 54}
    keySlice, err := scrypt.Key(password, []byte{}, 65536, 1, 32, 32)
    if err != nil {
        panic(err)
    }
    fmt.Println(keySlice)
}

with -race takes much longer than we expect. So this is why the failure with defaultKMDTimeoutSecs = 1 occurs - we kill kmd before it finishes encrypting the master encryption password.

I'm now looking at what happens when defaultKMDTimeoutSecs is larger

[zeldovich]

Interesting. One possibility is to change the scrypt default params (like defaultScryptN in daemon/kmd/config/config.go) for // +build race to a lower value so that it doesn't take so long.

[Vervious]

It's possible though I'm wondering if we should muck around with kmd's minScryptN parameter (in kmd/wallet/driver/sqlite_crypto.go). Or we can exclude -race from kmd entirely... none of the solutions that I can think of sound great.

(This is all assuming Scrypt is actually the issue for larger timeouts.)

[algoradam]

I say exclude -race from kmd entirely, at least when run on travis (except maybe on builds that run overnight). Mucking around with the kmd codebase to let tests use different scrypt parameters sounds like complexity we don't want inside kmd. We want kmd code to have as little complexity as possible; if kmd race tests require nontrivial messing with kmd internals then they're doing more harm than good.

[Vervious]

So locally, with timeout = 60 seconds, the same problem occurs. It looks like (in sqlite_crypto.go:81:deriveEncryptionkeyWithSalt

keySlice, err := scrypt.Key(password, salt[:], cfg.ScryptN, cfg.ScryptR, cfg.ScryptP, masterKeyLen)

takes at least a minute locally for me when running make integration

[zeldovich]

Sure, let's exclude kmd from -race for now