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

[ZIPs 244, 245] Specify an algorithm for non-malleable txid creation. #433

Merged
merged 25 commits into from
Feb 15, 2021

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Jan 7, 2021

No description provided.

@nuttycom nuttycom requested review from daira and str4d January 7, 2021 00:25
@nuttycom nuttycom self-assigned this Jan 7, 2021
@nuttycom nuttycom force-pushed the wip/transaction_malleability branch from 2d544eb to 6266f28 Compare January 8, 2021 01:17
@r3ld3v r3ld3v added this to the Core Sprint 2020-53 milestone Jan 11, 2021
@daira
Copy link
Collaborator

daira commented Jan 11, 2021

The commit message says ZIP 255 but it's actually ZIP 245.

@daira daira changed the title Specify an algorithm for non-malleable txid creation. [ZIPs 244, 245] Specify an algorithm for non-malleable txid creation. Jan 11, 2021
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0245.rst Outdated Show resolved Hide resolved
zip-0245.rst Outdated Show resolved Hide resolved
zip-0245.rst Outdated Show resolved Hide resolved
nuttycom and others added 2 commits January 13, 2021 16:57
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
@nuttycom
Copy link
Contributor Author

nuttycom commented Jan 20, 2021 via email

zip-0244.rst Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
nuttycom and others added 2 commits January 26, 2021 08:47
Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>
@nuttycom nuttycom marked this pull request as ready for review January 26, 2021 21:36
Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

The omission of spend authorization signatures from the authorization hash structure is blocking.

zip-0244.rst Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0245.rst Show resolved Hide resolved
zip-0245.rst Outdated Show resolved Hide resolved
@nuttycom
Copy link
Contributor Author

nuttycom commented Feb 2, 2021

A question about this ZIP arose this morning in a discussion between myself and @str4d. The current draft of this ZIP specifies that the consensus_branch_id be committed to as part of the personalization string for transaction identifier hashes. This could potentially pose a problem for off-chain protocols which require transaction identifiers to remain stable across network upgrade boundaries, such as BOLT.

One possible solution to this is to add the consensus_branch_id as an optional field to the transaction and include it as effecting data, such that a TxId may optionally (and would by default) commit to a consensus branch ID after which it would be considered invalid. This would make it possible to have transaction identifiers that remain stable across network upgrade boundaries, at the cost of replay protection. What do folks think here? Do protocols that depend on the consensus branch ID have to shut down or automatically terminate at network upgrade boundaries, or is there a better way?

cc/ @daira @dconnolly

Also clarify terminology around signature hash flags vs. types.
Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

ACK with suggestions.

zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
nuttycom and others added 2 commits February 9, 2021 08:06
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
@r3ld3v r3ld3v removed this from the Core Sprint 2021-04 milestone Feb 15, 2021
zip-0244.rst Outdated
Comment on lines 370 to 373
S.1: header_digest (32-byte hash output)
S.2: transparent_digest (32-byte hash output)
S.3: sprout_digest (32-byte hash output)
S.4: sapling_digest (32-byte hash output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These fields have the same names as the ones in the txid_digest, which is slightly confusing because this transparent_digest is not the same, in general. Should we use *_sig_digest for the fields that can potentially be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe for transparent_digest alone it should have a different name? The other three are the same as for the txid digest.

zip-0244.rst Outdated Show resolved Hide resolved
zip-0244.rst Outdated Show resolved Hide resolved
@daira daira merged commit feda12f into zcash:master Feb 15, 2021
@nuttycom nuttycom deleted the wip/transaction_malleability branch February 25, 2021 21:36
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