Conversation
pkg/common/appdata/types.go
Outdated
| func NewAppData(initialAppState []byte) *AppData { | ||
| return &AppData{ | ||
| version: Version_1, | ||
| version: Version_2, |
There was a problem hiding this comment.
we can keep Version_1 until we will be on mainnet (we don't have to deal with backward compatibility until then)
pkg/common/appdata/types.go
Outdated
|
|
||
| // Serialize converts the AppData struct into a byte slice. | ||
| // The format is: | ||
| // Version 2 format: |
pkg/executor/executor.go
Outdated
|
|
||
| if req.RequestType == common.AssociateKey { | ||
| //request of type associate key: the payload is not encrypted and contains the new key | ||
| //request of type associate key: the payload is not encrypted and contains the new key + seed |
There was a problem hiding this comment.
I would keep the seed optional (so it's up to the app decide if the seed is used or not) => in the event generation the "seed unspecificed" usecase is already covered but here it is not.
The associateKeyPayloadSize could be 133 or 198 length => if 133 no seed is passed and we just store the key as before
There was a problem hiding this comment.
fixed! Since the contract is changed, i ask @saratnt help for bindings regenerations
pkg/common/appdata/types.go
Outdated
| version uint8 | ||
| appNonce uint64 | ||
| appKeys KeyStore | ||
| appSeeds SeedStore |
There was a problem hiding this comment.
we could maybe rename: appEventSeeds for clarity
pkg/executor/executor.go
Outdated
|
|
||
| if req.RequestType == common.AssociateKey { | ||
| //request of type associate key: the payload is not encrypted and contains the new key | ||
| //request of type associate key: the payload is not encrypted and contains the new key + seed |
There was a problem hiding this comment.
Shouldn't the seed be encrypted? Otherwise anybody could generate all the possible EventSubType related to a single user
There was a problem hiding this comment.
correct! fixed
There was a problem hiding this comment.
oh yes, we can encrypt it with the key just generated (the key part must must be in clear)
pkg/executor/executor.go
Outdated
| if seed, hasSeed := seedStore[event.UserID]; hasSeed { | ||
| privSubtype, err := GenerateRandomSubtype(seed, DefaultSubtypeN) | ||
| if err != nil { | ||
| return nil, apperrors.New(apperrors.CodeRequestFuncFailed, "failed to generate random subtype") |
There was a problem hiding this comment.
This error shouldn't mark the request as failed, but it should be a transient one.
In addition, it is impossible that there is no seed for the user but there is the pub key, because the AssociateKey check that the payload is 198 bytes, this means the EventSubType field of the wasm event is never used and it can be removed
| // and returns GenerateSubtype(seed, index). | ||
| func GenerateRandomSubtype(seed []byte, n int) (string, error) { | ||
| randVal, err := rand.Int(rand.Reader, big.NewInt(int64(n))) | ||
| if err != nil { |
There was a problem hiding this comment.
If I have understood correctly, there are at most n different event subtypes for each user and only these n can be used.
If this is correct, it is possible that all the subtypes can be discovered in the long run for a user who performs enough requests. In fact, every time a user sends a request that generates just 1 event, this is a possibility to retrieve 1 of these subtypes. Eg when a user requests just a Deposit, there is just 1 event that we know belongs to the user itself, so we can get the subtype.
What about using the requestId instead of the random number, eg hash(seed+requestId)? The seed is private, so only the user can calculate it, with the requestId it changes everytime and it is never reused
There was a problem hiding this comment.
For the user (and web app) could be harder to retrieve all its used request IDs to get all the events, while it is easy to get all the numbers from 0 to N.
In the design we choose to use a random number between 0 and N as compromise between ease of use and privacy. In the long run only 1/N transaction could be bounded to the same address, so it is hard to build a story of the user operation just from 1/N for an high N (N=50 by default, but it could be increased).
But yours is also a good idea, maybe we can explore it for a next development
cc @paolocappelletti
There was a problem hiding this comment.
You're right, I totally forgot that the problem is that we don't know when was the last event related to that user. The requestId doesn't make sense in this scenario.
There was a problem hiding this comment.
yes, for now let'sstay simple.
another option could be changing the seed from time to time with a specific action by the user
# Conflicts: # pkg/common/appdata/types.go # pkg/common/appdata/types_test.go # pkg/executor/encrypt_events_test.go # pkg/executor/executor.go # pkg/executor/handle_process_request_test.go
# Conflicts: # contracts/contracts/ProcessorEndpoint.sol # pkg/blockchain/contracts/processorendpoint/ProcessorEndpoint.go
| } | ||
|
|
||
| // Write each seed key-value pair (address + 65-byte seed) | ||
| for addr, seed := range s.appEventSeeds { |
There was a problem hiding this comment.
It is not clear to me why for the appKeys map the size of the map keys (ethereum addresses) is checked but not the size of the map values (PublicKeyP521 keys), while for appEventSeeds is the opposite. If we want to be sure the data were not corrupted, both should be checked
There was a problem hiding this comment.
should be fixed now
| } | ||
|
|
||
| func (s *AppData) AddSeed(user ethCommon.Address, seed []byte) { | ||
| seedCopy := make([]byte, len(seed)) |
There was a problem hiding this comment.
Here the length of seed could be checked, to prevent a corrupted seed to be included
pkg/common/appdata/types.go
Outdated
| version: Version_1, | ||
| appNonce: 0, | ||
| wasmFingerprint: [WasmFingerprintSize]byte{}, | ||
| appKeys: make(map[ethCommon.Address]*cryptotypes.PublicKeyP521), |
There was a problem hiding this comment.
This should be make(KeyStore)
pkg/common/appdata/types.go
Outdated
|
|
||
| // Read seed store | ||
| ss := make(SeedStore) | ||
| if version != Version_1 { |
There was a problem hiding this comment.
This should be moved at line 164
pkg/common/appdata/types.go
Outdated
| return nil, fmt.Errorf("insufficient data for seed pairs: need %d, have %d", expectedSeedSize, reader.Len()) | ||
| } | ||
| for i := uint32(0); i < numSeeds; i++ { | ||
| addrBuf := make([]byte, KeyStore_KeySize) |
There was a problem hiding this comment.
For avoiding confusion, it would be better or to duplicate KeyStore_KeySize and to have SeedStore_KeySize const, or to rename KeyStore_KeySize to something more generic like Store_KeySize
There was a problem hiding this comment.
renamed to Store_KeySize
| ) | ||
|
|
||
| // Ensure encryptEvents copies EventSubType and keeps UserID as recipient. | ||
| func TestEncryptEventsCopiesSubType(t *testing.T) { |
There was a problem hiding this comment.
It would be nice to have another test for when the user has a seed, the EventSubType is changed
There was a problem hiding this comment.
added TestEncryptEventsChangesSubTypeWhenSeedPresent
pkg/executor/executor.go
Outdated
| @@ -635,6 +639,27 @@ func (e *StatelessExecutor) HandleProcessRequest(ctx context.Context, req *commo | |||
| totalFuel = totalFuel.Add(totalFuel, big.NewInt(10)) | |||
There was a problem hiding this comment.
This line should be moved at the end, at line 663
https://horizenlabs.atlassian.net/browse/HZN-2797