feat: implement g1 sign api #25
Conversation
shrimalmadhur
commented
Jan 8, 2025
•
shrimalmadhur
commented
- Implement the g1 sign endpoint
- Updating signature to raw bytes since it is using the underlying gnark lib
internal/services/signing/signing.go
Outdated
| pubKeyHex := common.Trim0x(req.GetPublicKeyG1()) | ||
| password := req.GetPassword() | ||
|
|
||
| if _, ok := s.keyCache[pubKeyHex]; !ok { |
There was a problem hiding this comment.
this isn't thread safe the way this cache is updated and checked, I would wrap the get/set in a mutex
There was a problem hiding this comment.
Agree with you George. Risk is really small here since the store is local and doesn't have eventual consistency but locking is needed for correctness, with this approach.
There was a problem hiding this comment.
The reason this doesn't necessarily need to be thread safe is because the value of the key will never be different. So practically there's no race condition on the value of the key. Give the only downside it has is to load from disk again which is a decent trade off to avoid mutex overhead. Let me know if that makes sense.
There was a problem hiding this comment.
I see. If the key isn't mutable then race condition concern is moot.
There was a problem hiding this comment.
That is correct
There was a problem hiding this comment.
I am pretty sure this could end up panicing even with different keys, since hash map might need to grow when adding a new key. There is a sync.map which is thread safe.
There was a problem hiding this comment.
here is a sample in a go playground: https://go.dev/play/p/mRFKiEikPuc
fatal error: concurrent map writes
There was a problem hiding this comment.
this makes sense - thanks for pointing that out. updated
internal/services/signing/signing.go
Outdated
| pubKeyHex := common.Trim0x(req.GetPublicKeyG1()) | ||
| password := req.GetPassword() | ||
|
|
||
| if _, ok := s.keyCache[pubKeyHex]; !ok { |
There was a problem hiding this comment.
Agree with you George. Risk is really small here since the store is local and doesn't have eventual consistency but locking is needed for correctness, with this approach.
