-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of Fluffy state network gossip. #2210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, fine with adding tests in follow-up PR also. As you see fit.
ContractTrieNodeOffer.init(parentProof, offer.accountProof, offer.blockHash) | ||
|
||
(parentKey, parentOffer) | ||
|
||
proc gossipOffer*( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdeme By the way I opted to leave the name as gossipOffer because in the case of bytecode there is no recursive gossip. The more generic name makes sense to me for this reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fine. The call is practically also doing more than just the recursive gossip as it also does the regular gossip.
) {.async.} = | ||
asyncSpawn p.neighborhoodGossipDiscardPeers( | ||
srcNodeId, ContentKeysList.init(@[keyBytes]), @[offerBytes] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdeme In this code I'm gossiping out the offer without await and then gossiping the parent offer below. Is there reason that you can think of to try and enforce the order of gossip by using an await here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. They will both end up in the offerQueue, so that's fine.
But eventually we might have to think about the effects when the offerQueue is full.
E.g. In case of a lot of gossip started from a bridge, there can come a build-up futures that are being awaited to add to the queue. And there wouldn't be a "feedback" of this possible due to the asyncSpawn afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll leave it for now and review later if and when needed once we have the state network up and running.
let (parentKey, parentOffer) = getParent(key, offer) | ||
asyncSpawn p.neighborhoodGossipDiscardPeers( | ||
srcNodeId, | ||
ContentKeysList.init(@[parentKey.toContentKey().encode()]), | ||
@[parentOffer.encode()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a little comment in front here that indicates that this is the recursive gossip part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do
Summary of changes:
getParent
helper functions to calculate the parent path and proof for recursive gossip.getAccountTrieNode
,getContractTrieNode
andgetContractCode
Tests still in progress.