Fix checking storage over randomly ordered list #1156
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1156 +/- ##
==========================================
+ Coverage 53.37% 53.40% +0.03%
==========================================
Files 324 324
Lines 21944 21955 +11
==========================================
+ Hits 11712 11726 +14
+ Misses 8639 8634 -5
- Partials 1593 1595 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
fvm/state/state.go
Outdated
| for k := range s.updatedAddresses { | ||
| addresses = append(addresses, k) | ||
| i := sort.Search(len(addresses), func(i int) bool { return addresses[i].Hex() >= k.Hex() }) |
There was a problem hiding this comment.
I'm afraid this will be used a lot (for every transaction?) and using .Hex() isn't best performance-wide. bytes.Compare instead?
There was a problem hiding this comment.
I was going to suggest similar thing, using bytes.Compare over slices or conversion to uint64 and compare considering the number of bytes of addresses
There was a problem hiding this comment.
I would rather do byte comparison... converting to uint introduces an element that could fail.
There was a problem hiding this comment.
converting to uint introduces an element that could fail.
Not really - one of the Go quirks I learned recently, it will silently truncate the value https://golang.org/ref/spec#Conversions
…rating-over-unordered-list
…rating-over-unordered-list
…onflow/flow-go into janez/fix-iterating-over-unordered-list
| } | ||
|
|
||
| sort.Sort(addresses) |
There was a problem hiding this comment.
Is there any performance concern with the Sort?
There was a problem hiding this comment.
The list is usually small and the comparison is with byte slices so the performance cost should be small. however, the list needs to be sorted...
This also only happens once per transaction.
|
bors merge |
1156: Fix checking storage over randomly ordered list r=huitseeker a=janezpodhostnik When looping through touched addresses for the purposes of checking storage, the code returns early. The list of touche addresses is acquired from a map of addresses, and thus randomly sorted. This caused that when we were checking storage and there was a error before the end of the loop, some random addresses were not touched. This could potentially fail verification at the same step. Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Ramtin M. Seraj <ramtinms@users.noreply.github.com>
|
Build failed: |
|
bors merge |
1156: Fix checking storage over randomly ordered list r=janezpodhostnik a=janezpodhostnik When looping through touched addresses for the purposes of checking storage, the code returns early. The list of touche addresses is acquired from a map of addresses, and thus randomly sorted. This caused that when we were checking storage and there was a error before the end of the loop, some random addresses were not touched. This could potentially fail verification at the same step. Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Ramtin M. Seraj <ramtinms@users.noreply.github.com>
|
Build failed: |
|
bors merge |
1156: Fix checking storage over randomly ordered list r=janezpodhostnik a=janezpodhostnik When looping through touched addresses for the purposes of checking storage, the code returns early. The list of touche addresses is acquired from a map of addresses, and thus randomly sorted. This caused that when we were checking storage and there was a error before the end of the loop, some random addresses were not touched. This could potentially fail verification at the same step. Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Ramtin M. Seraj <ramtinms@users.noreply.github.com>
|
Build failed: |
When looping through touched addresses for the purposes of checking storage, the code returns early. The list of touche addresses is acquired from a map of addresses, and thus randomly sorted.
This caused that when we were checking storage and there was a error before the end of the loop, some random addresses were not touched.
This could potentially fail verification at the same step.