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

Add signature v2 format #983

Merged
merged 7 commits into from
Dec 7, 2022
Merged

Add signature v2 format #983

merged 7 commits into from
Dec 7, 2022

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Nov 9, 2022

Summary

References #976

This PR introduces a new signature format for requests, in order to avoid situations in which validator requests are fulfilled by nodes which are not targeted by the RPC.

This PR is based on #976 in order to avoid merge conflicts - only the last two commits are relevant here.

Changes

  • Remove the static header check. Receptors will still keep adding it, but it is ignored from now on.
  • Add v2 signature format, which also takes into account the target axon hotkey. The v2 signature ensures that the signature cannot be faked by an intermediary that is not a validator.
  • Ensure that nonces cannot be replayed by disallowing equality. Allowing nonce equality renders the nonce moot.
  • On chain parameters of an axon are now always updated. Previously they would be updated only on IP/port changes.

@camfairchild
Copy link
Collaborator

camfairchild commented Nov 9, 2022

why not just up the network version itself?

edit: then use the version to decide on the parse method

@adriansmares
Copy link
Contributor Author

adriansmares commented Nov 9, 2022

why not just up the network version itself?

edit: then use the version to decide on the parse method

That wouldn't be backward compatible - new dendrites cannot talk to old axons (since old axons expect the old format). In this way there can be a transition period in which new dendrites can speak with old axons, and old axons allow traffic from old dendrites. In some future version, the v1 signature format can be completely removed and only v2 would remain.

@camfairchild
Copy link
Collaborator

why not just up the network version itself?

edit: then use the version to decide on the parse method

That wouldn't be backward compatible - new dendrites cannot talk to old axons (since old axons expect the old format). In this way there can be a transition period in which new dendrites can speak with old axons, and old axons allow traffic from old dendrites. In some future version, the v1 signature format can be completely removed and only v2 would remain.

I'm not suggesting you get rid of this. I'm suggesting you use the wire protocol version to do this instead of adding metadata

@adriansmares
Copy link
Contributor Author

I'm not suggesting you get rid of this. I'm suggesting you use the wire protocol version to do this instead of adding metadata

So I've ended up doing this based on the advertised version on chain. The dendrite/receptor decides based on the endpoint version which signature to use.

Note that this is a bit awkward right now - we cannot use version 3.4.2 as a version cutoff point, since 3.4.2 axons cannot parse this signature, but also nobunaga is still on version 3.4.2, so in order to test this, one must manually bump his __version_as_int__. Unit tests do try 3.4.3 as a receiving version however.

Copy link
Contributor

@unconst unconst left a comment

Choose a reason for hiding this comment

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

LGTM

@unconst unconst marked this pull request as ready for review November 9, 2022 23:00
@unconst
Copy link
Contributor

unconst commented Nov 9, 2022

I think we can safely begin the review process on this PR. @eduardogr I would like your take as well.

@unconst unconst added enhancement New feature or request feature new feature added labels Nov 9, 2022
Copy link
Contributor

@Eugene-hu Eugene-hu left a comment

Choose a reason for hiding this comment

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

Nice job! I like the new signature function. There are a few things that we need to fix before merging

bittensor/_axon/__init__.py Show resolved Hide resolved
bittensor/_axon/__init__.py Outdated Show resolved Hide resolved
bittensor/_receptor/receptor_impl.py Show resolved Hide resolved
@adriansmares adriansmares requested review from Eugene-hu and removed request for eduardogr November 10, 2022 10:16
Copy link
Contributor

@Eugene-hu Eugene-hu left a comment

Choose a reason for hiding this comment

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

LGTM, Nice job!

bittensor/_receptor/receptor_impl.py Show resolved Hide resolved
Copy link
Contributor

@eduardogr eduardogr left a comment

Choose a reason for hiding this comment

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

LGTM,
(commented already, i miss a couple of unit tests but everything good)

Thanks for this!

@Eugene-hu
Copy link
Contributor

@adriansmares Merging this PR today, it will be released in the next version

@Eugene-hu Eugene-hu merged commit 504704a into opentensor:nobunaga Dec 7, 2022
@adriansmares adriansmares deleted the feature/signature-v2 branch December 7, 2022 18:43
@eduardogr eduardogr mentioned this pull request Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature new feature added release/3.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants