Get block hashes in one HyperSync query - #431
Conversation
| let res = await queryBlockData( | ||
| ~fromBlock=fromBlock.contents, | ||
| ~toBlock=toBlock.contents, | ||
| ~serverUrl, | ||
| ~logger, | ||
| ) |
There was a problem hiding this comment.
Can we add a check for a reasonable block range here?
There was a problem hiding this comment.
Actually that's not really the behaviour I'm interested in. More along the lines of if say - block [10, 5_000_000, 5_000_001] is passed then this would be inefficient.
We don't necessarily need an alternative approach for the one case we use this for but we should probably just validate somewhere that we're getting reasonable numbers on this.
| res->Result.map(datas => { | ||
| datas->Array.keep(data => set->Utils.Set.has(data.blockNumber)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
And it would probably be good to validate that all block numbers that are queried, are in fact returned
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Added some suggestions for improvement but I'll leave it up to you. Thanks!
|
@JonoPrest Made the fixes in the commit. I'll appreciate a rereview b81fd58 |
| Belt.Array.concat(acc, [v.blockNumber]) | ||
| let getThresholdBlockNumbers = (self: t, ~currentBlockHeight) => { | ||
| let blockNumbers = [] | ||
| self.lastBlockScannedDataList->Belt.List.reduceReverseU((), ((), v) => { |
There was a problem hiding this comment.
I think probably better to use forEach here. Maybe List.reverse->List.forEach
Maybe we don't even need to worry about ordering here.
There was a problem hiding this comment.
We need to rewrite the module to use a hash-map data structure, otherwise some reorg detections might be lost for merge queries. I'll leave it for then.
| let getBlockHashes = switch getBlockHashesMock { | ||
| | Some(getBlockHashes) => getBlockHashes | ||
| | None => chainFetcher.chainConfig.source.getBlockHashes | ||
| } |
There was a problem hiding this comment.
Not sure if it makes a difference to the compiled code but you could default the param instead of to ? rather chainFetcher.chainConfig.source.getBlockHashes
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Awesome thanks Dmitry 💪🏼
Co-authored-by: Jono Prest <65739024+JonoPrest@users.noreply.github.com>
8ac8714 to
e8cdab5
Compare
2 participants
No description provided.