Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[NC-1344] Add GetReceiptsFromPeerTask #598

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Jan 18, 2019

PR description

Adds a GetReceiptsFromPeerTask to support getting transaction receipts from a peer. Receipts are actually requested based on the block hash, not the receipt root, but then we have to match the returned receipts back up to the headers using the receipt root. The task deduplicates headers with the same receipts root to minimise data transferred.

The task returns a map of BlockHeader to List<TransactionReceipt>. It could have just used the block hash as the key but that could easily be confused for the receipt root which is also of type Hash.

Fixed Issue(s)

NC-1344

.values()
.stream()
.map(headers -> headers.get(0).getHash())
.collect(toList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to filter out requests for empty receipt sets. Fine to follow-up with that optimization if you want to go ahead and merge this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but if everything in the list winds up being an empty receipt we wouldn't send a request at all and then we don't have a ResponseStream to return and everything gets weird. So I think it's actually better to handle that case back in the calling code - the downside being that it doesn't automatically get re-used if the task winds up called from a second place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm - wonder if we could have this return an Optional<ResponseStream> or something? In any case, I say we make a ticket to look into it and just merge this as-is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's sensible - then just need to have a way for the task to know it should insert the empty result, which probably boils down to just having a flag set or a condition in the process results section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added NC-2148 as a subtask so I don't forget this as I work through the actual downloading.

@ajsutton ajsutton merged commit 6421548 into PegaSysEng:master Jan 18, 2019
@ajsutton ajsutton deleted the NC-1344 branch January 18, 2019 21:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants