remove panics in StemFromBytes and FromLEBytes #366
Conversation
| if _, err := fillSuffixTreePoly(c1poly[:], values[:NodeWidth/2]); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Asking just in case: deferring GetConfig() to return an error?
There was a problem hiding this comment.
nah, because if you get an error at this stage, there's no point in continuing (and also it would make a huge diff as it's called everywhere)
conversion.go
Outdated
| var wg sync.WaitGroup | ||
| var ( | ||
| wg sync.WaitGroup | ||
| err error |
There was a problem hiding this comment.
This err is accessed by parallel goroutines so it will be a race condition.
I've created a fix for this using errgroup in this PR, you might want to take a look. It's targeting this PR.
Edit: I see that test failed for this reason.
tree.go
Outdated
| err := leafToComms(old[:], n.values[index]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = leafToComms(newH[:], value) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Nit: maybe if err := ...; err != nil { ?
tree.go
Outdated
| err := leafToComms(poly[(idx<<1)&0xFF:], val) | ||
| if err != nil { |
tree.go
Outdated
| if len(val) >= 16 { | ||
| FromLEBytes(&poly[1], val[16:]) | ||
| err = FromLEBytes(&poly[1], val[16:]) |
There was a problem hiding this comment.
Nit: despite this is correct, my paranoid side forces me to warn about: "if tomorrow somebody adds some extra code between lines 1301 and 1303, this can become a bug".
So maybe simply do:
...
if err := FromLEBytes(..., ...); err != nil {
return fmt.Errorf("converting to bytes: %s", err)
}
return nil
}
tree.go
Outdated
| err := StemFromBytes(&poly[1], n.stem) | ||
| if err != nil { |
| if r := recover(); r == nil { | ||
| t.Fatal("didn't catch length error") | ||
| } | ||
| }() | ||
| var value [33]byte | ||
| leafToComms([]Fr{}, value[:]) | ||
| err := leafToComms([]Fr{}, value[:]) | ||
| if err == nil { | ||
| t.Fatal("didn't catch length error") | ||
| } |
Co-authored-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
This is part (but not all) of the fix for #351, stopping here because the diff is already quite large.