Add `generate_bls_to_execution_change` by hwwhww · Pull Request #313 · ethereum/staking-deposit-cli

hwwhww · 2023-01-10T14:24:25Z

👉 Tutorial 👈

This PR is in review.

20230210 updated:

Update wording, and parameter descriptions.
Add script tests to CI to test generate_bls_to_execution_change functionality against binary executable files and different platforms.
Binary executable files of d83c312

20230201 updated:

Add Zhejiang testnet setting

Arguments

See README - generate-bls-to-execution-change Arguments.

Testing command:

python ./staking_deposit/deposit.py --language=english generate-bls-to-execution-change \
--chain=mainnet \
--mnemonic="sister protect peanut hill ready work profit fit wish want small inflict flip member tail between sick setup bright duck morning sell paper worry" \
--bls_withdrawal_credentials_list="0x00bd0b5a34de5fb17df08410b5e615dda87caf4fb72d0aac91ce5e52fc6aa8de,0x00a75d83f169fa6923f3dd78386d9608fab710d8f7fcf71ba9985893675d5382" \
--validator_start_index=0 \
--validator_indices="1,2" \
--execution_address='0x3434343434343434343434343434343434343434'

With `--devnet_chain_setting`

{"network_name": "<NETWORK_NAME>", "genesis_fork_version": "<GENESIS_FORK_VERSION>", "genesis_validator_root": "<GENESIS_VALIDATOR_ROOT>"}

python ./staking_deposit/deposit.py --language=english generate-bls-to-execution-change \
--chain=mainnet \
--mnemonic="sister protect peanut hill ready work profit fit wish want small inflict flip member tail between sick setup bright duck morning sell paper worry" \
--bls_withdrawal_credentials_list="0x00bd0b5a34de5fb17df08410b5e615dda87caf4fb72d0aac91ce5e52fc6aa8de,0x00a75d83f169fa6923f3dd78386d9608fab710d8f7fcf71ba9985893675d5382" \
--validator_start_index=0 \
--validator_indices="1,2" \
--execution_address='0x3434343434343434343434343434343434343434' \
--devnet_chain_setting='{"network_name": "MyDevnet", "genesis_fork_version": "0x00001111", "genesis_validator_root": "0x4b363db94e286120d76eb905340fdd4e54bfe9f06bf33ff6cf5ad27f511b9999"}'

Run pytest

pytest tests/test_cli/test_generate_bls_to_execution_change.py

TODOs

clean up intl JSON schema
add more tests (pytest and script tests)
add an alias for "generate_bls_to_execution_change". probably generate_btec?
update docs

…x typo

PizBernina · 2023-01-20T16:16:16Z

+            genesis_validator_root=devnet_chain_setting_dict['genesis_validator_root'],
+        )
+
+    # TODO: generate multiple?


yes, please kind sir allow for the bulk bonus feature mentioned here https://notes.ethereum.org/YGWpjmGHQceeq1gijiZuJg

…nemonic_bls_withdrawal_interactive`

CarlBeek

Still reviewing the logic in-depth, but this like a good start.

james-prysm · 2023-01-25T20:29:26Z

+        num_keys=num_validators,
+        amounts=amounts,
+        chain_setting=chain_setting,
+        start_index=validator_start_index,


this validator_start_index throws an uncaught exception if the user misenters, you can't tell from the response that this was incorrect and user probably might not know what the right answer is.

Hey @james-prysm could you elaborate on this case more? I tried to input random English and the cli did detect my wrong input and asked for re-input. What was the testing input you used?

I broke it :)

https://gist.github.com/infosecual/d7de7005bf54732a863dce21d503e445

using "1,2,3,4,5,6,7,8,9,10"

as the input to: Please enter a list of the old BLS withdrawal credentials of your validator(s). Split multiple items with whitespaces or commas.:

@hwwhww I totally missed this, put in the wrong validator_start_index with the corresponding validator indexs and it broke.

use case is
using a mnemonic of 1800 keys, 0~1799 index
input for validator_start_index: 1
validator_index: 0
withdrawal_credentials: 0x004652a99d8c9278708a98dab2c09b45cf09ea732037a17583ae2513a329fd8a
execution_address:
i get the following error
File "staking_deposit/utils/validation.py", line 246, in validate_bls_withdrawal_credentials_matching staking_deposit.exceptions.ValidationError: The given withdrawal credentials is matching the old BLS withdrawal credentials that mnemonic generated. [76365] Failed to execute script 'deposit' due to unhandled exception!
if i used validator_start_index:0 it works

james-prysm · 2023-01-25T20:31:07Z

+        amounts=amounts,
+        chain_setting=chain_setting,
+        start_index=validator_start_index,
+        hex_eth1_withdrawal_address=execution_address,


can both non hex and hex be handled? or if the error for non hex lets the user know,I believe some users will get the withdrawal pub key from the intial deposit.json which doesn't have 0x

bls_withdrawal_credentials_list can be with 0x prefix or no 0x.
eth1_withdrawal_address requires the checksum form and 0x prefix.

@hwwhww Does this mean if the user pastes an all lowercase address it will be blocked?

@wackerow
in a very edge case, an all-number address is a valid checksum address 😄 but I understand what you mean, yes, the input addresses have to be checksummed and capital letters matter.

james-prysm · 2023-01-26T13:03:26Z

+        list(key for key in ALL_CHAINS.keys() if key != PRATER)
+    ),
+)
+@load_mnemonic_arguments_decorator


can we have a prompt or something to make sure the user is doing this offline? to get the signatures in an airgapped location or something.

…ors_root`

hwwhww · 2023-02-10T17:10:36Z


 if __name__ == '__main__':
    check_python_version()
+    print('\n***Using the tool on an offline and secure device is highly recommended to keep your mnemonic safe.***\n')


@james-prysm, per https://github.com/ethereum/staking-deposit-cli/pull/313/files#r1087833602, I made a warning at the beginning.

Hey @CarlBeek It is printed before we select language now because I'm not sure how to print/echo the message before the user input sensitive data. do you have an idea of how to make it translatable?

I don't see an obvious way of doing so, unfortunately

CarlBeek

Fantastic work on this epic PR @hwwhww!

Having reviewed this again, I think we can move to merge this into dev. The core logic seems good to me.

That said, I do think there we need a bit more polish before putting this in front of mainnet users things like:

clarifying the wording around the different types of indices
- stretch goal, we calculate the start_index from the withdrawal credentials
try remove all the eth1 verbiage
Clean up the warning messages a bit, I think we've taken them overboard and users will get lost/numbed

infosecual · 2023-02-20T20:45:36Z

I have a few nit-picky things I noticed while doing a security review, none enough of an issue to stop merging to dev but I figured this is the best place to drop them as this is where the conversation about these changes is happening.

infosecual · 2023-02-20T20:47:22Z


+###### `generate-bls-to-execution-change` Arguments 
+
+You can use `bls-to-execution-change --help` to see all arguments. Note that if there are missing arguments that the CLI needs, it will ask you for them.


If you try to run the bls-to-execution-change subcommand with --help it will actually skip the help menu and drop right into interactive mode:

user@laptop:~/repos/staking-deposit-cli$ python3 ./staking_deposit/deposit.py generate-bls-to-execution-change --help ***Using the tool on an offline and secure device is highly recommended to keep your mnemonic safe.*** Please choose your language ['1. العربية', '2. ελληνικά', '3. English', '4. Français', '5. Bahasa melayu', '6. Italiano', '7. 日本語', '8. 한국어', '9. Português do Brasil', '10. român', '11. Türkçe', '12. 简体中文']: [English]:

infosecual · 2023-02-20T21:16:14Z

There may be a reason to specify "mnemonic language" vs. "language" since we have these as two separate fields in the initial validator credential creation:

user@laptop:~/Downloads/staking_deposit-cli-d83c312-linux-amd64$ ./deposit new-mnemonic

***Using the tool on an offline and secure device is highly recommended to keep your mnemonic safe.***

Please choose your language ['1. العربية', '2. ελληνικά', '3. English', '4. Français', '5. Bahasa melayu', '6. Italiano', '7. 日本語', '8. 한국어', '9. Português do Brasil', '10. român', '11. Türkçe', '12. 简体中文']:  [English]: 3
Please choose your mnemonic language ['1. 简体中文', '2. 繁體中文', '3. čeština', '4. English', '5. Italiano', '6. 한국어', '7. Português', '8. Español']:  [english]: 8

When we create the bls change message in interactive mode it might be good to specify that we mean mnemonic language here:

user@laptop:~/Downloads/staking_deposit-cli-d83c312-linux-amd64$ ./deposit generate-bls-to-execution-change 

***Using the tool on an offline and secure device is highly recommended to keep your mnemonic safe.***

Please choose your language ['1. العربية', '2. ελληνικά', '3. English', '4. Français', '5. Bahasa melayu', '6. Italiano', '7. 日本語', '8. 한국어', '9. Português do Brasil', '10. român', '11. Türkçe', '12. 简体中文']:  [English]:

I understand that this is not really a change that this PR made and it is technically inheriting this "Please choose your language" string from the existing-mnemonic work flow. It is just a little more obvious in that workflow since the subcommand contains the word "mnemonic" in it. idk if it is worth changing as it will be obvious it isn’t talking about CLI language after a bit. I just figured I would throw it out there. No strong preference here.

We would probably want to change the --language arg to --mnemonic-language too if we did want to specify. It is possible to confuse the two (see the other use of --language in the default help menu):

user@laptop:~/repos/staking-deposit-cli$ ./deposit.sh --help
linux-gnu
Running deposit-cli...

***Using the tool on an offline and secure device is highly recommended to keep your mnemonic safe.***

Usage: deposit.py [OPTIONS] COMMAND [ARGS]...

Options:
  --language TEXT  The language you wish to use the CLI in.

infosecual · 2023-02-20T21:21:15Z

The default help menu has an unintended newline or line wrap and is a little bit harder to read now. You can see the difference between master and bls-to-execution-change branch default help menu.

master branch (same as dev):

user@laptop:~/repos/staking-deposit-cli$ git checkout master 
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
user@laptop:~/repos/staking-deposit-cli$ python3 ./staking_deposit/deposit.py 
Usage: deposit.py [OPTIONS] COMMAND [ARGS]...

Options:
  --language TEXT  The language you wish to use the CLI in.
  --help           Show this message and exit.

Commands:
  existing-mnemonic  Generate (or recover) keys from an existing mnemonic
  new-mnemonic       Generate a new mnemonic and keys

bls-to-execution-change branch:

user@laptop:~/repos/staking-deposit-cli$ git checkout bls-to-execution-change
Switched to branch 'bls-to-execution-change'
Your branch is up to date with 'origin/bls-to-execution-change'.
user@laptop:~/repos/staking-deposit-cli$ python3 ./staking_deposit/deposit.py 

***Using the tool on an offline and secure device is highly recommended to keep your mnemonic safe.***

Usage: deposit.py [OPTIONS] COMMAND [ARGS]...

Options:
  --language TEXT  The language you wish to use the CLI in.
  --help           Show this message and exit.

Commands:
  existing-mnemonic               Generate (or recover) keys from an...
  generate-bls-to-execution-change
                                  Generating the...
  new-mnemonic                    Generate a new mnemonic and keys

Notice the last 3 lines here.

james-prysm · 2023-03-07T18:03:11Z

https://docs.prylabs.network/docs/wallet/withdraw-validator we updated our guide to include how to use some commands.

pablomendezroyo · 2023-03-10T07:59:44Z

Is it planned to support non-interactive usage for the blsToExecutionChange?

fix error message grammar

maeg84 · 2023-03-10T23:53:05Z

Is it planned to support non-interactive usage for the blsToExecutionChange?

There is the --non_interactive flag, had to read the code to find it.
I'm suggesting adding this to the documentation.
Also even with this flag the tool still asks you to press a key when finished, could use some improvement.

…and messages.

hwwhww · 2023-03-13T19:19:50Z

Updated

Address @james-prysm and @infosecual's suggestions on error handling
Modify the error messages.
Avoiding output of the call stack.
Make --non_interactive flag visible and add notes in docs per @maeg84's suggestion
Add prompt confirm requirement to generate-keys module's eth1_withdrawal_address option

Unsolved

The help menu line wrap (Add generate_bls_to_execution_change #313 (comment)): it looks like is deep in click. Might need some overriding work.
Remove all the eth1 verbiage: I have another WIP branch that tries to make --eth1_withdrawal_address be the alias of --execution_address. It works, but I am worried that, in the case that user somehow types in BOTH --eth1_withdrawal_address and --execution_address, click by default will override the previous with the latter. Will open another PR for it.
@CarlBeek's suggestion: Calculate the start_index from the withdrawal credentials.

Add `generate_bls_to_execution_change`

hwwhww added 2 commits January 4, 2023 17:31

Extract load_mnemonic_arguments_decorator func

d3fd1da

[WIP] Add generate_bls_to_execution_change

7404b69

hwwhww changed the base branch from master to dev January 10, 2023 14:24

hwwhww marked this pull request as draft January 10, 2023 14:25

hwwhww mentioned this pull request Jan 11, 2023

Add a process_bls_to_execution_change test case from staking-deposit-cli ethereum/consensus-specs#3202

Merged

hwwhww force-pushed the bls-to-execution-change branch from 34fc753 to 8ee1f9c Compare January 15, 2023 17:40

Always use GENESIS_FORK_VERSION to sign BLSToExecutionChange and fi…

d68a1c7

…x typo

hwwhww force-pushed the bls-to-execution-change branch from 8ee1f9c to 42cd2ce Compare January 15, 2023 18:34

Add 0x prefix to the bytes type fields of the output JSON file

6a4ba11

hwwhww force-pushed the bls-to-execution-change branch from 42cd2ce to 6a4ba11 Compare January 15, 2023 18:58

hwwhww added 6 commits January 16, 2023 17:15

WIP validation

47bf1aa

Add --devnet_chain_setting for devnet testing

63fdc5e

Improve validate_bls_to_execution_change

c5651ea

Add arguments description to README.md

2b5c218

Echo the folder path

1f8bab5

Arrange JSON fields by adding metadata

f5e2803

PizBernina reviewed Jan 20, 2023

View reviewed changes

hwwhww added 4 commits January 22, 2023 13:33

Normalize input bls_withdrawal_credentials and add `test_existing_m…

245ee7b

…nemonic_bls_withdrawal_interactive`

Generate multiple changes

3b7e144

Add checksum address validation

676279e

Add test case of multiple validator indices

75e8ea0

hwwhww marked this pull request as ready for review January 23, 2023 15:23

Update README

0be70aa

CarlBeek reviewed Jan 24, 2023

View reviewed changes

Comment thread staking_deposit/utils/validation.py

Comment thread staking_deposit/settings.py

Comment thread staking_deposit/cli/generate_bls_to_execution_change.py Outdated

Comment thread staking_deposit/cli/generate_bls_to_execution_change.py

james-prysm reviewed Jan 25, 2023

View reviewed changes

james-prysm reviewed Jan 26, 2023

View reviewed changes

Add retype confirmation and fix typo

962ce8c

james-prysm reviewed Jan 26, 2023

View reviewed changes

Comment thread staking_deposit/intl/en/utils/validation.json

Remove shutdown testnets and fill Sepolia and Goerli `genesis_validat…

1912868

…ors_root`

hwwhww commented Feb 10, 2023

View reviewed changes

Strip brackets and extra spaces

2c3fb22

remyroy mentioned this pull request Feb 15, 2023

Full release with binaries for generate-bls-to-execution-change #327

Closed

CarlBeek approved these changes Feb 16, 2023

View reviewed changes

fix error message grammar

db3e628

infosecual reviewed Feb 20, 2023

View reviewed changes

Comment thread staking_deposit/intl/en/cli/generate_bls_to_execution_change.json

PlanetaOTC mentioned this pull request Feb 23, 2023

Add option for "25th word" #270

Closed

Mariospartner mentioned this pull request Feb 27, 2023

Delete. #335

Closed

alex-miller-0 reviewed Feb 28, 2023

View reviewed changes

Comment thread staking_deposit/settings.py

pablomendezroyo mentioned this pull request Mar 10, 2023

BLS to Execution Change dappnode/StakingBrain#153

Open

Merge pull request #332 from JohnWestlund/bls-to-execution-change

2cd1d99

fix error message grammar

hwwhww added 3 commits March 13, 2023 22:13

Apply PR feedback by catching the exceptions. Improve error handling …

aae55a6

…and messages.

Improve error handling

f79bbac

Confirm deposit eth1_withdrawal_address input

9aed027

hwwhww force-pushed the bls-to-execution-change branch from 6968c15 to 36af26e Compare March 13, 2023 15:37

Add more description of mnemonic language per @infosecual's suggestion

ee052fa

hwwhww force-pushed the bls-to-execution-change branch from 36af26e to ee052fa Compare March 13, 2023 15:42

hwwhww added 2 commits March 13, 2023 23:44

Make --non_interactive flag visible and add notes in docs

69f778d

clean folders first

ed092fc

hwwhww merged commit fb39a2f into dev Mar 13, 2023

jerryji mentioned this pull request Mar 21, 2023

generate-bls-to-execution-change fails #342

Closed

sangheraio pushed a commit to earthwallet/earth-wallet-cli that referenced this pull request Aug 3, 2023

Merge pull request ethereum#313 from ethereum/bls-to-execution-change

15603ce

Add `generate_bls_to_execution_change`


		###### `generate-bls-to-execution-change` Arguments

		You can use `bls-to-execution-change --help` to see all arguments. Note that if there are missing arguments that the CLI needs, it will ask you for them.

Conversation

hwwhww commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

👉 Tutorial 👈

Arguments

Testing command:

With --devnet_chain_setting

Run pytest

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarlBeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infosecual Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwwhww Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarlBeek left a comment

Choose a reason for hiding this comment

Uh oh!

infosecual commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

infosecual commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infosecual commented Feb 20, 2023

Uh oh!

Uh oh!

james-prysm commented Mar 7, 2023

Uh oh!

pablomendezroyo commented Mar 10, 2023

Uh oh!

maeg84 commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwwhww commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

hwwhww commented Jan 10, 2023 •

edited

Loading

With `--devnet_chain_setting`

infosecual Feb 17, 2023 •

edited

Loading

hwwhww Feb 10, 2023 •

edited

Loading

infosecual commented Feb 20, 2023 •

edited

Loading

infosecual commented Feb 20, 2023 •

edited

Loading

maeg84 commented Mar 10, 2023 •

edited

Loading

hwwhww commented Mar 13, 2023 •

edited

Loading