-
Notifications
You must be signed in to change notification settings - Fork 679
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
Introduce ShortNodeID #3313
base: master
Are you sure you want to change the base?
Introduce ShortNodeID #3313
Conversation
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 we should be removing
ids.NodeIDLen
entirely. Every usage of it right now seems like it will break in the future. "vms/platformvm/txs".Validator#NodeID
should be aids.ShortNodeID
because it is serialized."vms/platformvm/txs/fee.intrinsicValidatorBandwidth"
should be usingids.ShortNodeIDLen
"vms/platformvm/txs/fee.IntrinsicRemoveSubnetValidatorTxComplexities"
should be usingids.ShortNodeIDLen
"vms/platformvm/txs.RemoveSubnetValidatorTx".NodeID
should be aids.ShortNodeID
because it is serialized.
tests/e2e/p/workflow.go
Outdated
@@ -100,12 +100,12 @@ var _ = e2e.DescribePChain("[Workflow]", func() { | |||
|
|||
// Use a random node ID to ensure that repeated test runs will succeed | |||
// against a network that persists across runs. | |||
validatorID, err := ids.ToNodeID(utils.RandomBytes(ids.NodeIDLen)) | |||
validatorID, err := ids.ToShortNodeID(utils.RandomBytes(ids.NodeIDLen)) |
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.
validatorID, err := ids.ToShortNodeID(utils.RandomBytes(ids.NodeIDLen)) | |
validatorID, err := ids.ToShortNodeID(utils.RandomBytes(ids.ShortNodeIDLen)) |
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.
done.
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.
Why does this file use ids.ShortNodeID
rather than keeping it generic as ids.NodeID
?
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.
done
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.
Why does this file use ids.ShortNodeID
rather than keeping it generic as ids.NodeID
?
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.
done
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.
marshalDiffKey
and unmarshalDiffKey
seem to make the assumption that ids.NodeID
has the same length as ids.ShortNodeID
. This doesn't seem safe in the long term
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.
good point. let me address that.
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.
done
vms/rpcchainvm/vm_server.go
Outdated
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.
Why does this file use ids.ShortNodeID
rather than keeping it generic as ids.NodeID
?
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.
done.
ids/test_generator.go
Outdated
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.
Not for this PR - but we should move this file out into an idstest
package.
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.
created #3343 to track this.
for _, shortNodeID := range DefaultNodeIDs { | ||
c.NodeIDs = append(c.NodeIDs, shortNodeID.NodeID()) | ||
} |
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 don't think we should be transforming the types here. I think the users of this util should be specifying the NodeID
type they expect to have used.
@@ -623,7 +623,7 @@ func TestGetCurrentValidators(t *testing.T) { | |||
found := false | |||
for i := 0; i < len(response.Validators); i++ { | |||
gotVdr := response.Validators[i].(pchainapi.PermissionlessValidator) | |||
if gotVdr.NodeID != nodeID { | |||
if gotVdr.NodeID.Compare(nodeID) != 0 { |
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.
Why change from using !=
to .Compare
(here and below)?
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.
given that the NodeID
is now a struct, using a comparison method allow us to ensure we can retain the comparison functionality even if we were to add, let's say, a non-comparable variable to the struct ( i.e. pointer ).
I figured that since we already having a comparison method, we should use it instead of relying on a mix of comparison techniques.
func (id *NodeID) UnmarshalText(text []byte) error { | ||
return id.UnmarshalJSON(text) | ||
type NodeID struct { | ||
ShortNodeID `serialize:"true"` |
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.
If this diff is "forwards safe" I think we should be able to add an additional field (or somehow else change the serialized format of this struct) without breaking a ton of tests.
packer := wrappers.Packer{ | ||
Bytes: make([]byte, preimageLen), | ||
} | ||
packer.PackFixedBytes(ip.NodeID[:]) | ||
packer.PackFixedBytes(ip.NodeID.Bytes()) | ||
packer.PackLong(timestamp) | ||
ip.GossipID = hashing.ComputeHash256Array(packer.Bytes) |
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.
This looks dubious. If ip.NodeID.Bytes()
started returning 32
bytes, it seems like this may behave unexpectedly.
I need to look into this a bit more carefully.
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.
in the context of this PR, I believe that this change is safe. The ip.NodeID
is populated few lines above as NodeID: ids.ShortNodeIDFromCert(cert).NodeID()
. Hence, guarantee that it's a 20 bytes slice.
Future PRs would need to update this logic, naturally.
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Why this should be merged
This PR is the first step toward ACP-20.
The existing
NodeID
was renamed intoShortNodeID
, and a new, backward compatibleNodeID
was added.Given that the ACP-20 changes are expected to be massive, I'd like to merge this backward-compatible change
before moving with the rest of the changes.
How this works
For most of the places, the usages of
NodeID
would remain as is.Places that need to retain the classic
NodeID
were modified to be using theShortNodeID
.The change is expected to be in a single direction only. i.e.
ShortNodeID
can be converted intoNodeID
by calling it'sNodeID()
function. No conversion fromNodeID
toShortNodeID
should be needed.How this was tested
Tested against existing unit tests.