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 ability to generate a signed exit transaction #355

Closed
wants to merge 11 commits into from
Closed

Add ability to generate a signed exit transaction #355

wants to merge 11 commits into from

Conversation

valefar-on-discord
Copy link

@valefar-on-discord valefar-on-discord commented Apr 21, 2023

This is a work in progress as I am looking for preliminary feedback on implementation and guidance on improvements
I feel this PR is in a usable state. The last improvement I am unsure of how to achieve is improving the file prompt experience.

As discussed here, with the launch of beaconchain's broadcasting tool it is now very easy for users to broadcast messages without the need for a node.

After helping dozens of individuals with the BLS change, I am now fielding questions on how to exit a validator. Telling the user to read the guides for their client or providing them ethdo commands can prove difficult for non-technical users especially if they don't have a running node.

Though there is little difference between ethdo and this cli to a non-technical user, my ultimate goal is to integrate this feature with wagyu-key-gen to provide an easy interface for users to make this modification. That tool is a wrapper of staking-deposit-cli making this change a prerequisite.

Implementation decisions

In order to sign a voluntary exit, there are a number of requirements:

  • Keystore: Required to sign the message
  • Validator index: A required parameter of the message. This validator index must correspond to the keystore or the transaction will be rejected on submission
  • Epoch: The epoch at which this exit transaction would be considered valid. I have made this an optional parameter and defaulted to 0. I would be interested to hear more about the consequence Danny is referring to here
  • Fork Version: Exit transactions will currently expire after a fork version, also discussed in the above issue. If we keep usability in mind, it is unrealistic to expect a user to provide this value. To get around this, I have hardcoded a current fork version to Capella for each network. I'd be interested in hearing alternatives.

VoluntaryExit
SignedVoluntaryExit

Example usage:
./deposit.sh exit-transaction-keystore --keystore keystore-m_***.json --chain zhejiang --keystore_password 12341234 --validator_index 4620 --epoch 1234

Example file output:

{
    "message": {
        "epoch": "1234",
        "validator_index": "4620"
    },
    "signature": "0x8dc1b1901f28b2c78363b6673e4daeae27638d21000c67719023dcaafa84335a2876d31bcb16fbb61449b009750f6d6d18c6e74f4942f1201a41f1023d3211e21dbd2d18a1b6af7a1e8d77550fdc7736c469f428feff1ea494ca984bfe55b7ec"
}

I also fixed a very minor parameter/readme discrepancy with mnemonic_password as mentioned here

@Nicky-2000
Copy link

Love this! It worked when I tried it out.

@remyroy
Copy link

remyroy commented Apr 26, 2023

This is also related to #316 .

@remyroy
Copy link

remyroy commented Apr 26, 2023

When calling the generate-exit-transaction interactively without supplying any option, it fails with a error message similar to:

Error: Invalid value for '--keystore': File '/home/remyroy/Projects/valefar-staking-deposit-cli' is a directory.

This could be improved by interactively asking for the various needed values similar to the current behavior when you call the new-mnemonic command for instance.

@remyroy
Copy link

remyroy commented Apr 26, 2023

When entering the wrong keystore password, I get this traceback. The error should be catched and a proper error message should be displayed instead.

Traceback (most recent call last):
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/staking_deposit-2.5.0-py3.8.egg/staking_deposit/cli/generate_exit_transaction.py", line 105, in generate_exit_transaction
    secret_bytes = saved_keystore.decrypt(keystore_password)
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/staking_deposit-2.5.0-py3.8.egg/staking_deposit/key_handling/keystore.py", line 159, in decrypt
    raise ValueError("Checksum message error")
ValueError: Checksum message error

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "staking_deposit/deposit.py", line 65, in <module>
    cli()
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/click/decorators.py", line 21, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/remyroy/VirtalEnvs/ValefarSignedSDC/lib/python3.8/site-packages/staking_deposit-2.5.0-py3.8.egg/staking_deposit/cli/generate_exit_transaction.py", line 107, in generate_exit_transaction
    raise ValidationError(load_text(['arg_generate_exit_transaction_keystore_password', 'mismatch']))
staking_deposit.exceptions.ValidationError: Error: The entered password doesn't match. Please try again.

@remyroy
Copy link

remyroy commented Apr 26, 2023

It would be nice to be able to use the mnemonic + index in addition to keystore + password to sign the exit and be able to create that exit file. This would make this process more flexible.

@remyroy
Copy link

remyroy commented Apr 26, 2023

My initial tests confirm that this PR generate valid exit signatures that can be broadcasted to perform a voluntary exit.

@valefar-on-discord
Copy link
Author

@remyroy
I have modified the prompt to include one for the keystore.
Worth noting that it is not a great UX... I investigated for a bit and couldn't figure out how to improve it to provide tab completion for files. It looks possible though would require quite a bit more work. I'll return to it when I have time.

With error handling it looks like most instances do a full exception raise which would provide the stack trace such as if you provide more/less validator indices than credentials when doing a BLS change. I have absorbed the error and I do think it looks better, just wanted to make a note.

With mnemonic support, this is going to be a bit of work but I see where you are going with adding support for it and it makes sense. Going to start working on that.

Thanks for taking a look!

@valefar-on-discord
Copy link
Author

I've implemented the ability to create exit transactions with a mnemonic.
At this point I am thinking this is a relatively solid implementation. My next step will be to reinvestigate proper file input and then write tests.

@valefar-on-discord valefar-on-discord changed the title [wip] Add ability to generate a signed exit transaction Add ability to generate a signed exit transaction Apr 29, 2023
@valefar-on-discord
Copy link
Author

I recently discovered that, unlike BLS changes, voluntary exits are not expected to be an array but individual signatures: https://ethereum.github.io/beacon-APIs/#/Beacon/submitPoolVoluntaryExit

The only modification this is going to make is the need to generate separate files in the case of the mnemonic instead of a single array output

@remyroy
Copy link

remyroy commented May 2, 2023

What do you guys think about this change @CarlBeek @hwwhww ? It would be a nice feature for the staking community.

@hwwhww
Copy link
Contributor

hwwhww commented May 12, 2023

@valefar-on-discord @remyroy

Hi! Thank you so much for making this PR!

My reasons for not providing this feature were

  1. VoluntaryExit operation is fork-specific: We have to maintain current_fork_version in some ways. Either we (i) update the hardcoded settings.py at each fork boundary or (ii) providing new --current_fork_version option.
  2. Clients already have such access to initiate VoluntaryExit with validator key:

(1) was a stronger pain.

However, I may have overlooked the requirements of non-technical users that you highlighted. I’d love to give this PR a thorough review and attempt to include it.
(But I'm still sad to see the hard-coded current_fork_version 😭)

@hwwhww
Copy link
Contributor

hwwhww commented May 18, 2023

One potential Deneb spec change of locking VoluntaryExit domain on capella: ethereum/consensus-specs#3288. It would fix the current_version UX issue here.

@valefar-on-discord
Copy link
Author

Yeah the hardcoded fork version is pretty horrendous, I agree.

I am trying to optimize for usability and the difficulty I had trying to figure out the current fork version was enough for me to know that non-technical users it would be impossible unless instructions hardcoded their own fork version which is pretty much the same problem just distributed. I wouldn't be opposed to adding the parameter as an optional override but I would not recommend removing the hardcoded value in favor of it.

@valefar-on-discord
Copy link
Author

valefar-on-discord commented Jun 23, 2023

@hwwhww with ethereum/consensus-specs#3288 being merged my guess is we would like to update this request to account for that change and then hold off until deneb to land it? (On second though, due to the backwards compatibility of the change we wouldn't need to wait)

@RohitAudit
Copy link

RohitAudit commented Jun 28, 2023

I am trying to use the code to generate the exits. It generates a file and I am broadcasted it to beacon chain network. But the validator is not getting exited. How can I verify that the exits that are generated are correct?
Below is the api endpoint I am using for beacon chain:
/eth/v1/beacon/pool/voluntary_exits

{
  "message": {
    "epoch": "185773",
    "validator_index": "502284"
  },
  "signature": "0xb1252ca77a99c16a9564c03559f995125680d30646e10562ed63443d30450accb6cf0cc44a5bc5829cade0a7c1fbd8750f7105d10747ffee5620a862947eaef9bd04e6554df416afe3a9c794a8ae3ee980beadd34241befd7daba4812adc4e5e"
}

@valefar-on-discord
Copy link
Author

@RohitAudit 👋
You can use ethdo with the same parameters and see if the signature matches.

When you used this tool, did you generate the signature by using your mnemonic or validator keystore?

I'll test today to see if I can figure out what happened but please let me know if you discover anything.

@RohitAudit
Copy link

@RohitAudit 👋 You can use ethdo with the same parameters and see if the signature matches.

When you used this tool, did you generate the signature by using your mnemonic or validator keystore?

I'll test today to see if I can figure out what happened but please let me know if you discover anything.

I used the validator keystore for signature generation. Let me try with ethdo and will inform you on the finding

@hwwhww
Copy link
Contributor

hwwhww commented Jun 28, 2023

@valefar-on-discord

@hwwhww with ethereum/consensus-specs#3288 being merged my guess is we would like to update this request to account for that change and then hold off until deneb to land it? (On second though, due to the backwards compatibility of the change we wouldn't need to wait)

Yes! We can pin the fork version to CAPELLA_FORK_VERSION in CLI now. 🎉

@valefar-on-discord
Copy link
Author

@RohitAudit

I created an exit message using this feature with a keystore and then created an exit message using latest ethdo with the corresponding mnemonic. The messages and signatures were identical.

Very curious what you find.

@valefar-on-discord
Copy link
Author

@valefar-on-discord

@hwwhww with ethereum/consensus-specs#3288 being merged my guess is we would like to update this request to account for that change and then hold off until deneb to land it? (On second though, due to the backwards compatibility of the change we wouldn't need to wait)

Yes! We can pin the fork version to CAPELLA_FORK_VERSION in CLI now. 🎉

Great to hear. I'll spend some time today to look through the code to see what I can cleanup given that change.

@RohitAudit
Copy link

RohitAudit commented Jun 28, 2023

@RohitAudit

I created an exit message using this feature with a keystore and then created an exit message using latest ethdo with the corresponding mnemonic. The messages and signatures were identical.

Very curious what you find.

I did generate and compared the files and they are identical. I think the problem is with my depositing it to the beacon chain.
I generated the validator exit with a past epoch initially.
Also, I have a question regarding the beacon api though. If my consensus client is not exiting the validator but it is present in the node, is there a way to force it?

staking_deposit/exit_transaction.py Outdated Show resolved Hide resolved
staking_deposit/exit_transaction.py Outdated Show resolved Hide resolved
staking_deposit/settings.py Outdated Show resolved Hide resolved
staking_deposit/utils/ssz.py Show resolved Hide resolved
staking_deposit/utils/ssz.py Outdated Show resolved Hide resolved
tests/test_cli/test_exit_transaction_keystore.py Outdated Show resolved Hide resolved
@valefar-on-discord
Copy link
Author

valefar-on-discord commented Jul 1, 2023

@hwwhww Did a pass on updating the code and fixing unit tests. Successful build would lead me to believe this should be ready to go for review.

The way I tested was using ethdo to validate message and signatures match up.

@valefar-on-discord
Copy link
Author

Following up on this.

Please let me know if there is more needed.

@VVander
Copy link

VVander commented Oct 26, 2023

Hello @hwwhww, is there any chance of merging this?

We would like to use this to generate pre-signed exit messages to ensure the safety of our decentralized operator group. Using ethdo would work, but the UX provided in this PR is far better for users simply trying to generate a message.

@VVander
Copy link

VVander commented Oct 31, 2023

@CarlBeek should I be pinging you instead? Looks like you're the one who has been maintaining the project recently

@valefar-on-discord
Copy link
Author

Closing this as I've been notified this isn't going to be landed because:

  • This repo has been auditing and future changes may require an additional audit
  • Don't want to normalize putting mnemonic in the interface

Glad this was helpful for some. 🎉

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.

6 participants