Skip to content
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

Bugfix pubkey keepalive overflow and "forever orders" #1668

Merged

Conversation

Alrighttt
Copy link
Member

This PR addresses two issues caused by trusting peers to report a valid timestamp for PubkeyKeepAlive network messages.

First, this can cause an integer overflow during this operation:
https://github.com/KomodoPlatform/atomicDEX-API/blob/4c66f02ea78bd097c5313bcec938229346073099/mm2src/mm2_main/src/lp_ordermatch.rs#L2986

Second, it prevents a malicious node from setting a timestamp very far into the future. A timestamp set very far into the future would result in the orders staying in a node's orderbook even after the peer that broadcasted it has gone offline.

What this does is ignore the timestamp value sent from the peer and instead set it to the current time at which the message was received.

@cipig
Copy link
Member

cipig commented Feb 20, 2023

What this does is ignore the timestamp value sent from the peer and instead set it to the current time at which the message was received.

What do we do when nodes have the wrong time set? Can we make sure that seed nodes have the correct time? Can we drop seed nodes with wrong clock from the network? Or better all nodes with wrong clock? There is also this issue related to wrong clock: #1115

@onur-ozkan
Copy link
Member

What this does is ignore the timestamp value sent from the peer and instead set it to the current time at which the message was received.

What do we do when nodes have the wrong time set? Can we make sure that seed nodes have the correct time? Can we drop seed nodes with wrong clock from the network? Or better all nodes with wrong clock? There is also this issue related to wrong clock: #1115

We design the solution, but it's not implemented yet.

@Alrighttt
Copy link
Member Author

Alrighttt commented Feb 21, 2023

What this does is ignore the timestamp value sent from the peer and instead set it to the current time at which the message was received.

What do we do when nodes have the wrong time set? Can we make sure that seed nodes have the correct time? Can we drop seed nodes with wrong clock from the network? Or better all nodes with wrong clock? There is also this issue related to wrong clock: #1115

I wouldn't consider misaligned clocks relevant to this issue.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix!
I agree with @Alrighttt, misaligned clocks is not relevant to this issue since the order receiving node local time is now used for calculating the received time for the last keep alive msg, it was actually more of an issue before this fix since a node/seednode with a misaligned clock might have deleted non timed out maker orders straight away.

@ca333 ca333 requested a review from DeckerSU February 24, 2023 07:49
@ca333
Copy link

ca333 commented Feb 24, 2023

What this does is ignore the timestamp value sent from the peer and instead set it to the current time at which the message was received.

What do we do when nodes have the wrong time set? Can we make sure that seed nodes have the correct time? Can we drop seed nodes with wrong clock from the network? Or better all nodes with wrong clock? There is also this issue related to wrong clock: #1115

we had this topic in the past and DEX/mm2 (or any other blockchain based dapp/SW) shouldn't (purely) orientate towards local/"central" timestamps as they can always be off. Also comparing against some online source isn't reliable neither and won't ensure sync across the network. We did in the past solve similar (time-related) issues by sourcing timestamp from the blockchain stack (ensures sync / eq across network).

Added additional small comment for clarification on issue close: #1115 (comment)

@ca333 ca333 merged commit bf43cc8 into KomodoPlatform:dev Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants