Changed hashing schema implementation to match provider-NIT-4724#4586
Changed hashing schema implementation to match provider-NIT-4724#4586mahdy-nasr wants to merge 2 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4586 +/- ##
==========================================
+ Coverage 34.22% 34.33% +0.11%
==========================================
Files 498 498
Lines 59094 59085 -9
==========================================
+ Hits 20223 20286 +63
+ Misses 35293 35218 -75
- Partials 3578 3581 +3 |
❌ 14 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
| } | ||
|
|
||
| salt, err := hex.DecodeString(trimHexPrefix(payload.Salt)) | ||
| salt, err := uuid.Parse(payload.Salt) |
There was a problem hiding this comment.
Do we really need to enforce that Salt is an uuid here?
Spec only says it is string.
This can break down the line, salt format can change to not use uuid, which is valid according to the spec, and we will break here.
There was a problem hiding this comment.
there is no detailed actual spec. we have only code example of the hashing. But from the example and from speaking with them. the address is in form of '0x....' with 20bytes but used with delimiter in a string version. and Salt is UUID using with delimiter too in string version.
This implementation is better to catch any deviation and has strong validation that we get the data in right format. we part the 20byte addresses. we parse the 32Byte hashes, we parse the UUID and when do the hashing we use string::string format.
In future, the only change we spoke about that they will do is moving everything to binary bytes for hashing.
There was a problem hiding this comment.
Specification only says that salt is a string.
The fact that they are using a uuid, and providing the uuid in string format is irrelevant to nitro then.
The provider can decide to change how the salt is built on the fly, and we should be able to handle that gracefully if they provide the string representation.
Just use the string, no need to enforce parsing uuid for the salt here.
Parsing the uuid is not compliant with the spec, and make nitro more fragile.
There was a problem hiding this comment.
@mahdy-nasr confirmed, with the salt provider, that despite the document that we have access not mentioning this, the salt will be a uuid.
| } | ||
|
|
||
| salt, err := hex.DecodeString(trimHexPrefix(payload.Salt)) | ||
| salt, err := uuid.Parse(payload.Salt) |
There was a problem hiding this comment.
@mahdy-nasr confirmed, with the salt provider, that despite the document that we have access not mentioning this, the salt will be a uuid.
|
|
||
| // Store hashes to the hashstore so FilteringReady returns true | ||
| execNode.Sequencer.StoreFilterRulesForTest(t, []byte("test-salt"), nil, "test-digest") | ||
| salt, _ := uuid.Parse("3ccf0cbf-b23f-47ba-9c2f-4e7bd672b4c7") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
There was a problem hiding this comment.
Hmm, This is just a test, and I’m already providing a valid format, so this adds no value.
| // After load, should have current time | ||
| before := time.Now() | ||
| store.Store([]byte("salt"), nil, "etag") | ||
| salt, _ := uuid.Parse("2cef04bf-b23f-47ba-9c2f-4e7bd652c1c6") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
|
|
||
| // Create test data | ||
| salt := []byte("test-salt") | ||
| salt, _ := uuid.Parse("2cef04bf-b23f-47ba-9c2f-4e7bd652c1c6") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
| store := NewHashStore(100) | ||
|
|
||
| salt1 := []byte("salt1") | ||
| salt1, _ := uuid.Parse("3ccf0cbf-b23f-47ba-9c2f-4e7bd672b4c7") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
|
|
||
| // Store second set with different salt (simulating hourly rotation) | ||
| salt2 := []byte("salt2") | ||
| salt2, _ := uuid.Parse("2cef04bf-b23f-47ba-9c2f-4e7bd652c1c6") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
| store := NewHashStore(100) | ||
|
|
||
| salt1 := []byte("test-salt") | ||
| salt1, _ := uuid.Parse("3ccf0cbf-b23f-47ba-9c2f-4e7bd672b4c7") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
|
|
||
| // prepare second set for swapping | ||
| salt2 := []byte("new-salt") | ||
| salt2, _ := uuid.Parse("2cef04bf-b23f-47ba-9c2f-4e7bd652c1c6") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
| } | ||
| if string(salt) != "test-salt" { | ||
| t.Errorf("expected salt 'test-salt', got '%s'", string(salt)) | ||
| expectedSalt, _ := uuid.Parse("2cef04bf-b23f-47ba-9c2f-4e7bd652c1c6") |
There was a problem hiding this comment.
nitpick: do not ignore returned errors.
|
@diegoximenes
|
It is about good practices instead of correctness. |
|
@diegoximenes I agree with you totally in general. But for this case I am not convinced : |
I understand your concern, but part of code review is helping each other build habits that pay off over time.
|
I agree. TestHashStore_LoadedAt also does not support this point, since it is not testing salt correctness. Salt here is a dummy placeholder. If salt parsing were wrong, the other relevant tests would already fail. I do not see value in continuing this discussion further. Lets focus on other more priority work now. |
3 participants
As per linear ticket, this PR fixes the current hashing algorithm for address-filtering feature. It is matching with current provider has.