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.

Sign up for GitHub

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

Jump to bottom

feat(new-RPC): connection healthcheck implementation for peers #2194

Open
wants to merge 38 commits into
base: dev
Choose a base branch
from
Open

feat(new-RPC): connection healthcheck implementation for peers #2194

wants to merge 38 commits into from
+629 −18

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Aug 20, 2024 •
edited
Loading

  • Implement a new P2P message type, "hcheck."
  • Implement peer_connection_healthcheck to verify if peer X is currently connected to the KDF network.
  • Make the heathcheck feature configurable.
  • Add unit and integration tests for the new implementations.

@onur-ozkan onur-ozkan added enhancement New feature or request in progress Changes will be made from the author labels Aug 20, 2024
@onur-ozkan
prevent brute-force attacks
0aaa011 
Signed-off-by: onur-ozkan <[email protected]>
@onur-ozkan
handle panics
b73a0f2 
Signed-off-by: onur-ozkan <[email protected]>
@onur-ozkan
add doc-comments
6286299 
Signed-off-by: onur-ozkan <[email protected]>
@onur-ozkan onur-ozkan marked this pull request as ready for review September 3, 2024 04:40
@onur-ozkan onur-ozkan added under review and removed in progress Changes will be made from the author labels Sep 3, 2024
@onur-ozkan onur-ozkan mentioned this pull request Sep 3, 2024
Open
@onur-ozkan
handle our address
14eeaee 
Signed-off-by: onur-ozkan <[email protected]>
Alrighttt
Alrighttt previously approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@Alrighttt Alrighttt left a comment

Choose a reason for hiding this comment

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

LGTM

@onur-ozkan onur-ozkan mentioned this pull request Sep 26, 2024
Open
mariocynicys
mariocynicys reviewed Sep 26, 2024
View reviewed changes
mm2src/mm2_main/src/lp_healthcheck.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_healthcheck.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_healthcheck.rs Outdated Show resolved Hide resolved
borngraced
borngraced reviewed Sep 30, 2024
View reviewed changes
Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

one question?

mm2src/mm2_core/src/mm_ctx.rs Outdated Show resolved Hide resolved
mariocynicys
mariocynicys previously approved these changes Sep 30, 2024
View reviewed changes
mm2src/mm2_main/src/lp_healthcheck.rs Outdated Show resolved Hide resolved
@onur-ozkan onur-ozkan added in progress Changes will be made from the author and removed under review labels Sep 30, 2024
mariocynicys
mariocynicys previously approved these changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks :)

mm2src/mm2_main/src/lp_healthcheck.rs Show resolved Hide resolved
@onur-ozkan onur-ozkan added under review and removed in progress Changes will be made from the author labels Oct 2, 2024
mariocynicys
mariocynicys approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 😂

mm2src/mm2_main/src/lp_healthcheck.rs Show resolved Hide resolved
@shamardy shamardy self-requested a review October 3, 2024 06:12
@shamardy
Copy link
Collaborator

shamardy commented Oct 3, 2024

There seems to be a lot of changes here since my last approval, will give it another check today to make sure everything is fine, if so, will approve and merge.

shamardy
shamardy approved these changes Oct 3, 2024
View reviewed changes
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.

Huge work! Everything looks good to me, the below comments are just some nits, it's up to you if you want to resolve them.

Please merge with dev for failing test_electrum_tx_history to pass.

mm2src/mm2_main/src/lp_healthcheck.rs
Comment on lines +130 to +139
let sender_public_key = keypair.public().encode_protobuf();

let data = HealthcheckData {
sender_public_key,
expires_at_secs: u64::try_from(Utc::now().timestamp()).map_err(|e| e.to_string())?
+ healthcheck_message_exp_secs(),
is_a_reply,
};

let signature = try_s!(keypair.sign(&try_s!(data.encode())));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit strange that we encode the sender_public_key using protobuf but we use messagepack for the whole data. It's fine as it will cause no problems but thought to bring it up. If you wanna encode the whole data using protobuf you can follow the swaps v2 examples, it's up to you @onur-ozkan

komodo-defi-framework/mm2src/mm2_main/src/lp_swap.rs

Line 1706 in 7723b50

let encoded_msg = msg.encode_to_vec();

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO protobuf definitions add unnecessary complexity since we don't need to provide a language-agnostic interface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Protobuf has some backward compatibility benefits if some structs are nested as opposed to messagepack ref. #1017 (comment) , if this struct won't change in this way then no need for it for sure. Please resolve this if it's not needed.

mm2src/mm2_main/src/lp_healthcheck.rs

{
let mut book = ctx.healthcheck_response_handler.lock().await;
book.clear_expired_entries();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This clear_expired_entries will be removed when merging with this #2232 depending on which gets merged first. Just a note :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimxy dimxy dimxy left review comments

@shamardy shamardy shamardy approved these changes

@borngraced borngraced borngraced left review comments

@mariocynicys mariocynicys mariocynicys approved these changes

@Alrighttt Alrighttt Alrighttt left review comments

Assignees
No one assigned
Labels
enhancement New feature or request under review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@onur-ozkan @shamardy @dimxy @Alrighttt @borngraced @mariocynicys