[PAN-2198] Update GetNodeDataFromPeerTask to return a map #931
Conversation
smatthewenglish
changed the title
smatthewenglish
force-pushed
the
get-node-data-task-map
branch
from
dfe380c
to
4786df4
Compare
smatthewenglish
removed
the
work in progress
| @@ -83,6 +87,13 @@ protected ResponseStream sendRequest(final EthPeer peer) throws PeerNotConnected | |||
| // Message contains unrequested data, must not be the response to our request. | |||
There was a problem hiding this comment.
The corresponding ticket for this task was badly written and didn't really mention this, but the main focus for this update should be to minimize the number of hash calculations we do to improve efficiency.
So, I suggest combining this check on line 86 (which checks that the data in the response corresponds to the data requested) with the construction of the map in mapNodeDataByHash. You could do this by writing a for loop that iterates over nodeData. For each element in the list, you can calculate the hash and check if the hash is in hashes. If not, you can return early without calculating all of the hashes. If it is in the list, you can add it to your map, which will be returned only if you make it all the way through the loop.
| for (int i = 0; i < 3; i++) { | ||
| final BlockHeader blockHeader = blockchain.getBlockHeader(10 + i).get(); | ||
| requestedData.add( | ||
| requestedData.put( | ||
| Hash.fromHexStringLenient(Integer.toHexString(i)), |
There was a problem hiding this comment.
This should be the hash of the node data you're requesting rather than a random hash.
| final List<Hash> hashes = requestedData.stream().map(Hash::hash).collect(toList()); | ||
| protected EthTask<PeerTaskResult<Map<Hash, BytesValue>>> createTask( | ||
| final Map<Hash, BytesValue> requestedData) { | ||
| final List<Hash> hashes = requestedData.values().stream().map(Hash::hash).collect(toList()); |
There was a problem hiding this comment.
If you update line 38 above, you can then just use: hashes = requestedData.keySet()
| assertThat(requestedData).containsAll(partialResponse); | ||
| final List<BytesValue> requestData = new ArrayList<>(requestedData.values()); | ||
| final List<BytesValue> resultData = new ArrayList<>(partialResponse.values()); | ||
| assertThat(requestData).containsAll(resultData); |
There was a problem hiding this comment.
You're only checking the values here, but I think its worth checking that the values are mapped with the correct keys. Pending updates to line 38, you should be over to iterate over the partial response keyValue pairs and then look up each value in requestedData and check that it matches.
| assertThat(response.getResult()).containsExactlyInAnyOrderElementsOf(requestedData); | ||
| final List<BytesValue> requestData = new ArrayList<>(requestedData.values()); | ||
| final List<BytesValue> resultData = new ArrayList<>(response.getResult().values()); | ||
| assertThat(resultData).containsExactlyInAnyOrderElementsOf(requestData); |
There was a problem hiding this comment.
Same thing here as above: you should be able to check keys and values are as expected. Should just be an equality check here ...
| } else if (nodeData.size() > hashes.size()) { | ||
| // Can't be the response to our request | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
You want to keep this check here:
if (nodeData.size() > hashes.size()) {
// Can't be the response to our request
return Optional.empty();
}
PR description
Update
GetNodeDataFromPeerTaskto return map to node data by hash.Fixed Issue(s)