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

Support threshold custody for validators #4037

Merged
merged 19 commits into from
Mar 26, 2024
Merged

Conversation

plaidfinch
Copy link
Collaborator

@plaidfinch plaidfinch commented Mar 16, 2024

This PR finishes #3813, adding support for signing validator definitions and votes to the threshold custody backend, as well as widening the general interface for custody backends to permit any backend to support these operations.

Status: @cronokirby and I walked through this and determined that it does work after all, the UX is just a little unfortunate when multiple calls to threshold custody backend are required to broadcast a single transaction (as in the case with validator custody). But it works!

@plaidfinch plaidfinch requested a review from cronokirby March 16, 2024 22:02
@plaidfinch plaidfinch force-pushed the validator-threshold-custody branch from 832baaf to 8b97a81 Compare March 22, 2024 21:55
@plaidfinch plaidfinch self-assigned this Mar 23, 2024
@plaidfinch plaidfinch marked this pull request as ready for review March 23, 2024 01:20
@plaidfinch
Copy link
Collaborator Author

plaidfinch commented Mar 23, 2024

The protobuf lint failure is because I made CoordinatorRound1 a true oneof, which I believe should be a backwards compatible change because it's a new oneof with one field moved into it, but because the proto lint doesn't know it's a new oneof, I think it's complaining. This is per suggestion of @hdevalence.

Here are all the scenarios I'd ideally like to test:

  • softkms, no separate governance key
  • threshold, no separate governance key
  • softkms, softkms governance key
  • threshold, softkms governance key
  • softkms, threshold governance key
  • threshold, threshold governance key

Ideally, we would have an integration test for each, exercising all of:

  • send a normal transaction
  • vote as a validator
  • update a validator definition

@cratelyn how feasible do you think it would be to set up such tests?

@plaidfinch plaidfinch force-pushed the validator-threshold-custody branch from 4f5dd6e to e034bdf Compare March 23, 2024 01:45
@plaidfinch plaidfinch marked this pull request as draft March 23, 2024 21:13
@cratelyn
Copy link
Contributor

Ideally, we would have an integration test for each, exercising all of:

* send a normal transaction 
* vote as a validator
* update a validator definition

@cratelyn how feasible do you think it would be to set up such tests?

@plaidfinch that sounds very feasible! check out https://github.com/penumbra-zone/penumbra/blob/main/crates/core/app/tests/mock_consensus_staking.rs#L29-L52

that test creates a validator, and (un)delegates to it.

#3913 and #3973 track the work to integrate the view server into these tests, which should further streamline the ergonomics of creating transactions in tests. you can check out the penumbra-mock-consensus library if you'd like to take a look around. it's still a work-in-progress (#3588) but this sounds absolutely like a use-case we should cover.

i'm happy to talk sometime next week, or if you'd like, feel free to file a ticket and i can take a look as part of my ongoing work in #3588.

@cronokirby cronokirby force-pushed the validator-threshold-custody branch from f99dc65 to 893ede9 Compare March 26, 2024 19:38
This will no longer prompt for signatures if none are required for a
transaction.
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

Paired on this with @plaidfinch.

@plaidfinch plaidfinch marked this pull request as ready for review March 26, 2024 21:39
@plaidfinch
Copy link
Collaborator Author

@hdevalence I think this is ready to merge, but I need an administrative override to merge with the failing protobuf lint, which is showing that I'm making a breaking change to the threshold custody struct CoordinatorRound1: Field "1" on message "CoordinatorRound1" moved from outside to inside a oneof. This is, I believe, a spurious error, because of #3813 (comment), and moreover because the CoordinatorRound1 message is not used in consensus or in any third-party tooling yet. Further changes to support any other kind of signing (unlikely but possible) would not cause any more breakage, because you can expand the options on a oneof freely. After examining to confirm my conclusion, would you mind permitting the merge?

@hdevalence hdevalence merged commit 26a82e0 into main Mar 26, 2024
6 of 7 checks passed
@hdevalence hdevalence deleted the validator-threshold-custody branch March 26, 2024 22:33
@hdevalence
Copy link
Member

De-activated, merged, then re-activated the rule. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants