Skip to content

Add RPCs for checking peer best blocks & target blocks#117

Merged
majecty merged 1 commit intoCodeChain-io:masterfrom
byeongjee:add.rpc.peers.and.target_blocks
Jan 30, 2020
Merged

Add RPCs for checking peer best blocks & target blocks#117
majecty merged 1 commit intoCodeChain-io:masterfrom
byeongjee:add.rpc.peers.and.target_blocks

Conversation

@byeongjee
Copy link
Contributor

@byeongjee byeongjee commented Jan 28, 2020 •
edited
Loading

It resolves #71

@byeongjee byeongjee requested a review from majecty January 28, 2020 05:54
majecty
majecty requested changes Jan 28, 2020
View reviewed changes
sync/src/block/extension.rs Outdated
}
Event::GetPeerBestBlockHashes(channel) => {
for header_downloader in self.header_downloaders.values() {
channel.send(header_downloader.best_hash()).unwrap();
Copy link

@majecty majecty Jan 28, 2020

Choose a reason for hiding this comment

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

We need the information which peer has which best block hash.
Please pass the IP address of the peer also.

@byeongjee byeongjee force-pushed the add.rpc.peers.and.target_blocks branch 2 times, most recently from 6227757 to 0d3a106 Compare January 28, 2020 07:28
@HoOngEe HoOngEe self-requested a review January 29, 2020 02:58
@HoOngEe
Copy link
Contributor

HoOngEe commented Jan 29, 2020

Is there any special reason to select ipAddr as a return value? I think ip address is not enough to be used in peer identification. SocketAddr seems to be great. @majecty what do you think of it?

@majecty
Copy link

majecty commented Jan 29, 2020

Aha, SocketAddr has a port number. I agree with @HoOngEe.

majecty
majecty requested changes Jan 29, 2020
View reviewed changes
Copy link

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Please check #117 (comment)

@byeongjee byeongjee force-pushed the add.rpc.peers.and.target_blocks branch from 8cf6f2e to b0d6b0c Compare January 29, 2020 07:07
@byeongjee
Copy link
Contributor Author

byeongjee commented Jan 29, 2020

I changed IpAddr into SocketAddr and updated JSON-RPC documentation

@byeongjee byeongjee force-pushed the add.rpc.peers.and.target_blocks branch 2 times, most recently from 3b65f7e to bb3390d Compare January 29, 2020 07:15
@HoOngEe HoOngEe self-requested a review January 29, 2020 07:19
HoOngEe
HoOngEe requested changes Jan 29, 2020
View reviewed changes
Copy link
Contributor

@HoOngEe HoOngEe left a comment

Choose a reason for hiding this comment

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

Please change license part.

majecty
majecty requested changes Jan 29, 2020
View reviewed changes
spec/JSON-RPC.md Outdated

### Returns

{ SocketAddress: `string`, BlockHash: `H256`}[]
Copy link

@majecty majecty Jan 29, 2020

Choose a reason for hiding this comment

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

This type does not match the example.
What you said:

[ 
{ "SocketAddress": "xxx", "BlockHash": "yyy"},
{ "SocketAddress": "xxx2", "BlockHash": "yyy2"}, ...
]

However, in the example:

[["1.2.3.4:3485", "0x56642f04d519ae3262c7ba6facf1c5b11450ebaeb7955337cfbc45420d573077"]]

Copy link
Contributor Author

@byeongjee byeongjee Jan 29, 2020

Choose a reason for hiding this comment

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

Okay. Should I introduce new type such as SocketAddressAndHash, or tuple type is sufficient?

Copy link

@majecty majecty Jan 29, 2020

Choose a reason for hiding this comment

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

IMO, tuple type is sufficient.

Copy link
Contributor Author

@byeongjee byeongjee Jan 29, 2020

Choose a reason for hiding this comment

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

Then the return type should be [ 'string', 'H256'][]?

Copy link

@majecty majecty Jan 29, 2020

Choose a reason for hiding this comment

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

It looks good to me.

@byeongjee byeongjee force-pushed the add.rpc.peers.and.target_blocks branch from bb3390d to ae70e6b Compare January 29, 2020 07:43
majecty
majecty reviewed Jan 29, 2020
View reviewed changes
rpc/src/v1/traits/chain.rs Outdated
@@ -1,4 +1,4 @@
// Copyright 2018-2019 Kodebox, Inc.
// Copyright 2018-2020 Kodebox, Inc.
Copy link

@majecty majecty Jan 29, 2020

Choose a reason for hiding this comment

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

Please update the copyrights in the files that you changed.

Copy link
Contributor Author

@byeongjee byeongjee Jan 29, 2020

Choose a reason for hiding this comment

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

Sorry. I changed the copyright of wrong file. I pushed the fixed version.

@byeongjee byeongjee force-pushed the add.rpc.peers.and.target_blocks branch from ae70e6b to 2b202f7 Compare January 29, 2020 07:50
majecty
majecty previously approved these changes Jan 29, 2020
View reviewed changes
@byeongjee
Copy link
Contributor Author

byeongjee commented Jan 30, 2020

There was timeout exceptions in some tests, but they passed successfully in my local machine. @majecty

@HoOngEe HoOngEe self-requested a review January 30, 2020 03:00
HoOngEe
HoOngEe previously approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@HoOngEe HoOngEe left a comment

Choose a reason for hiding this comment

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

LGTM

@byeongjee byeongjee dismissed stale reviews from HoOngEe and majecty via ba1525f January 30, 2020 03:27
@byeongjee byeongjee force-pushed the add.rpc.peers.and.target_blocks branch from 2b202f7 to ba1525f Compare January 30, 2020 03:27
@majecty majecty merged commit c06cc03 into CodeChain-io:master Jan 30, 2020
@majecty majecty mentioned this pull request Jul 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majecty majecty majecty approved these changes

@HoOngEe HoOngEe HoOngEe left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add RPCs for checking sync peers & sync target blocks

3 participants

@byeongjee @HoOngEe @majecty