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

ZIP-244 & ZIP-245: creation of non-malleable txids + TZE signature hashing #319

Closed
wants to merge 15 commits into from

Conversation

nuttycom
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #319 (4d2bd72) into master (3a8e729) will increase coverage by 0.56%.
The diff coverage is 19.06%.

❗ Current head 4d2bd72 differs from pull request most recent head dad336a. Consider uploading reports for the commit dad336a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   64.64%   65.21%   +0.56%     
==========================================
  Files          69       65       -4     
  Lines        7079     6965     -114     
==========================================
- Hits         4576     4542      -34     
+ Misses       2503     2423      -80     
Impacted Files Coverage Δ
zcash_primitives/src/transaction/sighash.rs 31.96% <9.55%> (-50.28%) ⬇️
zcash_primitives/src/transaction/blake2b_256.rs 33.33% <33.33%> (ø)
zcash_primitives/src/transaction/mod.rs 44.29% <39.70%> (-4.95%) ⬇️
zcash_primitives/src/transaction/tests.rs 88.46% <100.00%> (-9.54%) ⬇️
zcash_client_sqlite/src/error.rs 12.96% <0.00%> (-3.17%) ⬇️
zcash_history/src/node_data.rs 62.03% <0.00%> (-2.78%) ⬇️
zcash_client_sqlite/src/chain.rs 95.20% <0.00%> (-2.36%) ⬇️
zcash_primitives/src/keys.rs 63.04% <0.00%> (-2.18%) ⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a8e729...dad336a. Read the comment docs.

@nuttycom nuttycom force-pushed the nonmalleability branch 2 times, most recently from 0ae177e to 4d2bd72 Compare December 8, 2020 01:10
@r3ld3v r3ld3v added this to the Core Sprint 2020-49 milestone Dec 8, 2020
@nuttycom nuttycom self-assigned this Jan 7, 2021
CompactSize::write(&mut ch, precondition.extension_id.try_into().unwrap()).unwrap();
CompactSize::write(&mut ch, precondition.mode.try_into().unwrap()).unwrap();
Vector::write(&mut ch, &precondition.payload, |w, e| w.write_u8(*e)).unwrap();
ch.write_all(&value.to_i64_le_bytes()).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@str4d now that I'm revisiting this signature hash code that we originally worked on together, I'm puzzled here - why is there a value associated with the TZE input?

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic for adding it was the same as for transparent inputs: without it, there's nothing in the spend that directly commits to the value of the spent output, so a hardware wallet can't verify the fee without being provided those input coins. By having the value be part of the signature digest, the hardware wallet can present accurate information without needing the coin being spent, and the resulting signature can only be used if the client wasn't lying about the coin's value.

This is also the thing I was saying we should check against the hardware wallet fee vulnerability from last year, which IIRC said that this defense was ineffective if the adversary could ask the user to sign multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. I understand this better now than the last time we had this dicussion.

@nuttycom nuttycom changed the title WIP: creation of non-malleable txids ZIP-244 & ZIP-245: creation of non-malleable txids + TZE signature hashing Jan 27, 2021
@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

@nuttycom
Copy link
Contributor Author

nuttycom commented Feb 2, 2021

Oops, that comment was supposed to go on the ZIP.

@daira
Copy link
Contributor

daira commented Feb 2, 2021

It was a deliberate design decision to prioritize replay protection ahead of allowing transactions to remain valid across a network upgrade, when we designed Overwinter. I'm not saying we have to maintain that decision for all time; just pointing out that it wasn't something that happened accidentally.

Restructure txid generation via a hash tree structured
such that all possible txid hash data is reused in the
signature hashes.
@nuttycom
Copy link
Contributor Author

nuttycom commented May 4, 2021

subsumed by #375

Copy link

@Baiju311 Baiju311 left a comment

Choose a reason for hiding this comment

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

Deleted due to spam comment.

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.

5 participants