-
Notifications
You must be signed in to change notification settings - Fork 309
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
Refactor Messages Rebroadcast #1403
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #1403 +/- ##
===============================================
+ Coverage 47.84% 48.68% +0.83%
===============================================
Files 50 60 +10
Lines 15622 17782 +2160
===============================================
+ Hits 7475 8657 +1182
- Misses 8147 9125 +978
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
So we are now skipping the verification on these messages. It is more efficient. Can we think of some scenario of how this can be misused by malicious actor? If not I am fine with this faster approach. Seems like its highly difficult to get on the network, gain trust of the peer, try to spam it and overload some particular node with asking for a message
Might be good to conduct throughout testing, benchamrking of it this week
try { | ||
if (myCacheTemp.has(messageHash)) { | ||
const message = myCacheTemp.get(messageHash); | ||
if (message) { |
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.
Should there be some fallback to database?
We surely have the message and client requested it but it may not be in a cache?
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 think there must be a fallback to database, if not we would not be able to receive the message as thats the only way to sync up
const dataObj = { | ||
messageHashPresent: hash(message.data), | ||
}; | ||
messageString = JSON.stringify(dataObj); |
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.
But we are only sending the dataObj which has just messageHashPresent
I think it needs to be proper message, formatted and signed like we have with
const appRemovedMessage = {
type: 'fluxappremoved',
version: 1,
appName,
ip,
broadcastedAt,
};
log.info('Broadcasting appremoved message to the network');
// broadcast messages about app removed to all peers
await fluxCommunicationMessagesSender.broadcastMessageToOutgoing(appRemovedMessage);
```
const { messageHashPresent } = msgObj; | ||
const { requestMessageHash } = msgObj; | ||
if (messageHashPresent) { | ||
if (typeof messageHashPresent !== 'string' || messageHashPresent.length !== 40) { |
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.
is it always 40?
https://github.com/puleos/object-hash
All discussed in private chat :)! |
It's almost like a ping/pong on the websocket.