Skip to content

Use digests for proposal ids, rather than incrementing integers#2104

Merged
achamayou merged 9 commits into
microsoft:masterfrom
achamayou:switch_proposal_ids_to_digests
Jan 25, 2021
Merged

Use digests for proposal ids, rather than incrementing integers#2104
achamayou merged 9 commits into
microsoft:masterfrom
achamayou:switch_proposal_ids_to_digests

Conversation

@achamayou
Copy link
Copy Markdown
Member

@achamayou achamayou commented Jan 22, 2021

This is part of #1649, and a chunk of #2087.

Proposal ids are updated to become hex-encoded digest strings, rather than integers. The digest only covers the proposal request itself at the moment, and votes are therefore no more replay-proof than under the previous integer scheme unless the user includes a nonce in their proposal.

Once necessary changes have been made to the history/KV/frontend, this digest will be combined with the root of the transactions' Merkle Tree as of the transaction read_version. Users will no longer need to include a nonce in their proposal to ensure votes cannot be reused.

  • Update proposals included in documentation to user new ids

@achamayou achamayou requested a review from a team as a code owner January 22, 2021 13:32
@achamayou achamayou mentioned this pull request Jan 22, 2021
@ghost
Copy link
Copy Markdown

ghost commented Jan 22, 2021

switch_proposal_ids_to_digests@17904 aka 20210125.5 vs master ewma over 50 builds from 17274 to 17901
images

Comment thread src/node/rpc/member_frontend.h Outdated
Comment thread doc/governance/open_network.rst
Comment thread src/node/rpc/member_frontend.h Outdated

if (!set_jwt_public_signing_keys(
ctx.tx, INVALID_ID, parsed.issuer, issuer_metadata, parsed.jwks))
ctx.tx, "", parsed.issuer, issuer_metadata, parsed.jwks))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Would prefer if "" was INVALID_PROPOSAL_ID, set to something like INVALID_PROPOSAL_ID = "INVALID" in proposals.h, so we can catch if this ever leaks somewhere unexpected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok

@achamayou achamayou merged commit 32cc0ad into microsoft:master Jan 25, 2021
@letmaik
Copy link
Copy Markdown
Member

letmaik commented Feb 1, 2021

Once necessary changes have been made to the history/KV/frontend, this digest will be combined with the root of the transactions' Merkle Tree as of the transaction read_version. Users will no longer need to include a nonce in their proposal to ensure votes cannot be reused.

@achamayou Is this tracked somewhere?

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.

Re-evaluate the use of KV-driven incrementing IDs for users, members, nodes and proposals

4 participants