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

Fix Duplicated entries in AddressBook 0.0.101 #22

Merged
merged 8 commits into from
Mar 31, 2021

Conversation

anighanta
Copy link
Contributor

@anighanta anighanta commented Mar 22, 2021

Resolves issue: #21
Parent Issue : hashgraph/hedera-services#750

Signed-off-by: anighanta anirudh.ghanta@hedera.com

… a new type NodeAddressNodeAddressAbbreviated to allow backward compatibility

Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
@steven-sheehy
Copy link
Member

I have outstanding questions in the services ticket that I would like to see answered.

@anighanta
Copy link
Contributor Author

I have outstanding questions in the services ticket that I would like to see answered.

hashgraph/hedera-services#750 (comment)

Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
@anighanta anighanta marked this pull request as draft March 23, 2021 15:41
services/BasicTypes.proto Outdated Show resolved Hide resolved
services/BasicTypes.proto Outdated Show resolved Hide resolved
services/BasicTypes.proto Show resolved Hide resolved
services/BasicTypes.proto Outdated Show resolved Hide resolved
services/BasicTypes.proto Show resolved Hide resolved
services/BasicTypes.proto Outdated Show resolved Hide resolved
Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
@anighanta anighanta requested a review from lbaird March 24, 2021 18:42
@anighanta anighanta marked this pull request as ready for review March 24, 2021 18:43
…eviated to NodeAddressForClients and NodeAddressBookAbbreviated to AddressBookForClients
services/BasicTypes.proto Outdated Show resolved Hide resolved
services/BasicTypes.proto Outdated Show resolved Hide resolved
services/BasicTypes.proto Outdated Show resolved Hide resolved
services/BasicTypes.proto Outdated Show resolved Hide resolved

/* All the information required to connect to the network.*/
message NodeAddressForClients {
int64 nodeId = 5; // A non-sequential identifier for the node. This value is the key between the full and abbreviated address books
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on this more please; reading the comments I'm not entirely sure what nodeId is; what are these key's between full and abbreviated address books? Also, what is an abbreviated address book? I understand this field is a int64 and nodeAccountId is an AccountID, but what is the difference between these two fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Idea behind this change is that, as we have multiple IPs for each node, when we get the addressBook 0.0.101 of mainnet for example.. we get a list with repeated entries for each node. To resolve this we want to create a new abbreviated version of the addressBook which will have a repeated Endpoint. So in future, when we get the addressbook we will get just enough information to connect to the network using this abbreviated message.

Note: this abbreviated message is AddressBookForClients.

A nodeId is just a sequential identifier of a node and the nodeAccountID is the AccountID associated with the node. Right now although they represent different things, their value is the same as in if nodeID is N, its account id is 0.0.N

Copy link
Member

Choose a reason for hiding this comment

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

if nodeID is N, its account id is 0.0.N

Correction: if nodeID is N, its account id is 0.0.(N + 2). Example: node 1 is 0.0.3

Copy link
Member

@steven-sheehy steven-sheehy Mar 31, 2021

Choose a reason for hiding this comment

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

This is just the case now, but won't always be the case. Nodes can change their account ID in the future to something else while the node ID has to stay the same. Neither will necessarily stay sequential in the future either. Thus clients shouldn't rely on the account ID and should use the node ID. Later we will need to update the stream bucket paths as well to use the node ID, for example.

Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
@anighanta anighanta requested review from steven-sheehy and janaakhterov and removed request for lbaird March 31, 2021 17:11
Signed-off-by: anighanta <anirudh.ghanta@hedera.com>
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM

@anighanta anighanta merged commit e438df2 into main Mar 31, 2021
@anighanta anighanta deleted the 021-M-FixAddressBookDuplicateEntries branch March 31, 2021 18:27
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.

4 participants