-
Notifications
You must be signed in to change notification settings - Fork 27
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
Voting Key generation using typescript and multi file support #177
Conversation
|
Thanks. We also use bootstrap for private networks, epoch length would be large in this case. I can add the limit the public/testnet nodes |
Update, I've added multi implementation (all implementations pass the vector tests). The fastest one by far is noble-ed25519. It's the current default. |
Added multi implementation, best one is noble-ed25519 Updated VotingService.ts to use the native one instead of the catapult tools
c735043
to
c631e13
Compare
# Conflicts: # cmds/config-testnet-supernode.sh # package-lock.json # package.json # presets/testnet/network.yml # src/model/Addresses.ts # src/service/BootstrapUtils.ts # src/service/ConfigLoader.ts # test/service/BootstrapUtils.test.ts # test/supernode.yml
# Conflicts: # presets/shared.yml
…otstrap into native_voting_key_file # Conflicts: # presets/shared.yml
e3a8623
to
3b62d86
Compare
# Conflicts: # CHANGELOG.md # src/service/LinkService.ts # src/service/RewardProgramService.ts
# Conflicts: # CHANGELOG.md # presets/shared.yml # src/service/ConfigService.ts # src/service/VotingService.ts
1f65bd3
to
8e8d422
Compare
8e8d422
to
45a9dac
Compare
docs/upgradeVotingKeys.md
Outdated
|
||
Voting file upgrade: | ||
- If the node's current voting file has an end epoch close to the current epoch ("close to expiring") this command creates a new 'private_key_treeX.dat' that continues the current file. | ||
- "Close to expiring" happens when the epoch is in the upper half of the voting file. If the file's epoch length is 720, close to expiring will be 360+. |
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 the default should be to allow the user to upgrade 1 month before keys expire.
Allow them to override with a warning.
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 at this stage allowing the user to upgrade 3 months in advance is fine, especially because this is the first upgrade and people need to get the process in place (like all nodes). We could reduce the default values in a later voting key upgrade
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 a month is more than enough time to run the update command.
Allnode could create a cron job that runs every 6 months to generate new key files. If they want to run it early, then nothing stopping them since there is an override option.
We are kind thinking similar except your suggestion would require an update later to the bootstrap.
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.
Ok, I will upgrade the preset. Defaults will be:
votingKeyDesiredLifetime: 6 months in epochs (how long the file last)
votingKeyDesiredFutureLifetime: 1 month in epochs (1 month before the current file expires bootstrap will create a new file)
README.md
Outdated
@@ -277,6 +277,7 @@ General users should install this tool like any other node module. | |||
* [`symbol-bootstrap run`](docs/run.md) - It boots the network via docker using the generated `docker-compose.yml` file and configuration. The config and compose methods/commands need to be called before this method. This is just a wrapper for the `docker-compose up` bash call. | |||
* [`symbol-bootstrap start`](docs/start.md) - Single command that aggregates config, compose and run in one line! | |||
* [`symbol-bootstrap stop`](docs/stop.md) - It stops the docker-compose network if running (symbol-bootstrap started with --detached). This is just a wrapper for the `docker-compose down` bash call. | |||
* [`symbol-bootstrap upgradeVotingKeys`](docs/upgradeVotingKeys.md) - It upgrades the voting keys and files when required. |
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.
upgradeVotingKeys
not sure if command name. Not really upgrading the voting keys. Maybe registerVotingKeys
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.
Unsure, the command creates the file when necessary, it doesn't register in the network. For that, the link command is required (or symbol cli)
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.
hehe...maybe register
is not a good name either.
upgradeVotingKeys
doesnt make much sense to me since voting key/files are not upgradable.
@segfaultxavi thoughts?
maybe addVotingKeys
or createVotingKeys
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.
renewVotingKeys
?
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.
Options:
- updateVotingKeys
- createVotingKeys
- addVotingKeys
- renewVotingKeys
maybe 3 ? @segfaultxavi @Wayonb
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 command, together with link
, does two things, right? Creates new keys AND removes (or unregisters) the old ones. I think add
and create
both fail to convey the second action. I can see @Wayonb's point that the keys are not actually "updated", so I suggested the renew
option :)
But this command will be very seldom used (twice a year!) so let's go with whatever :D
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've talked to @Wayonb , updateVotingKeys is the name.
src/service/VotingService.ts
Outdated
const votingUtils = new VotingUtils(); | ||
await BootstrapUtils.deleteFile(join(votingKeysFolder, 'metadata.yml')); | ||
let newFileCreated = false; | ||
while (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.
Not sure why while(true)
? maybe I am missing something.
Are multiple files getting created? I would assume when the loop is finish either it creates a voting key file or not.
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 you have a short votingKeyDesiredLifetime and a large votingKeyDesiredFutureLifetime in your custom preset, the service generates multiple files. Most people would not change this, but you have the option of having smaller files at once if you like
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.
ok. I would suggest not generating keys that are not been used.
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.
Fair enough. I have removed the while and only one file can be created at a given time. Also, votingKeyDesiredFutureLifetime cannot be greater than votingKeyDesiredLifetime.
# Conflicts: # CHANGELOG.md # package-lock.json
src/commands/updateVotingKeys.ts
Outdated
|
||
Voting file update: | ||
- If the node's current voting file has an end epoch close to the current epoch ("close to expiring") this command creates a new 'private_key_treeX.dat' that continues the current file. | ||
- By default, "Close to expiring" happens the voting file in the . By default, bootstrap will allow creating new files once the current file reaches its last month. |
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 know what are you trying to say here, the first sentence makes no sense.
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.
Yeap, what about now?
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.
👍
@@ -7,6 +7,9 @@ The changelog format is based on [Keep a Changelog](https://keepachangelog.com/e | |||
## [1.0.7] - NEXT | |||
**Milestone**: Mainnet(1.0.1.0) | |||
|
|||
- Added multi voting key file support. | |||
- Added `upgradeVotingKeys` command. | |||
|
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.
updateVotingKeys?
Update:
This PR now supports both the original catapult tools' voting key file generation and the native typescript. The original is the one used by default. You can enable the new native typescript with the useExperimentalNativeVotingKeyGeneration: true custom preset.
I'm also working on multi-voting file support. The voting file metadata (public key, start, and end epoch) is loaded directly from the .dat files.
Voting file upgrade idea:
For the end-user, the upgrade process is pretty much the same:
We can also add a new command "upgradeVotingFiles" that does the --upgrade of voting files explicitly
TODOs:
Slack chat: https://nemgroup-workspace.slack.com/archives/C015AEQ9ZL2/p1616410897191200
Related to #149
Original:
This is my first implementation of voting key file generation done in Javascript/Typescript.
I think the code is ok, the main issue is that the SDK is really slow generating keys and signing data. The SDK uses tweetnacl, which is small but slow. These are some numbers on my dev machine when creating the voting key file:
(votingKeyEndEpoch - votingKeyStartEpoch + 1 -> TIME)
@gimer , could you validate the logic in https://github.com/nemtech/symbol-bootstrap/blob/native_voting_key_file/src/service/VotingUtils.ts#L24?
As unit testing, I'm comparing files generates with catapult tools against files generated using the native implementation. Checking that the headers and sizes are the same. I would add more assertions validating the internal items (private key and signatures).
To speed up the implementation we could:
Before doing any improvement, I want to double-check the logic is fine.
I'm pushing hard for this feature not because I want to reimplement the tool. This will allow me to make Bootstrap config offline without docker and OS independent (catapult tools wouldn't need to be compiled and installed in the offline machine). Supernodes will have fewer excuses to not use Bootstrap to setup the configuration (when runing barebone or with docker compose in the final online node machine)
Fixes #169
Related to #170
Update:
noble-ed25519 takes ~40 seconds for the 26280 epochs