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

Fix sapling binding signature errors #1939

Closed
teor2345 opened this issue Mar 25, 2021 · 6 comments · Fixed by #2459
Closed

Fix sapling binding signature errors #1939

teor2345 opened this issue Mar 25, 2021 · 6 comments · Fixed by #2459
Assignees
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-bug Category: This is a bug I-consensus Zebra breaks a Zcash consensus rule S-needs-investigation Status: Needs further investigation

Comments

@teor2345
Copy link
Contributor

Motivation

In #1377, we discovered that Zebra's sapling binding signature verification fails on valid transactions.

Solution

Diagnose the errors in the binding signature code, and re-enable async binding signature validation:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-consensus/src/transaction.rs#L271

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage S-needs-investigation Status: Needs further investigation P-Medium I-consensus Zebra breaks a Zcash consensus rule labels Mar 25, 2021
@teor2345 teor2345 added this to the 2021 Sprint 7 milestone Mar 25, 2021
@teor2345
Copy link
Contributor Author

@dconnolly feel free to fill in the rest of this ticket, I opened it because I needed a ticket number for a PR.

teor2345 added a commit to teor2345/zebra that referenced this issue Mar 25, 2021
But keep ignoring those errors until we fix binding sigs in ZcashFoundation#1939
dconnolly pushed a commit that referenced this issue Mar 25, 2021
But keep ignoring those errors until we fix binding sigs in #1939
@mpguerra mpguerra added NU-5 Network Upgrade: NU5 specific tasks and removed NU-5 Network Upgrade: NU5 specific tasks S-needs-triage Status: A bug report needs triage labels Apr 6, 2021
@dconnolly dconnolly self-assigned this Apr 19, 2021
@teor2345 teor2345 modified the milestones: 2021 Sprint 8 , 2021 Sprint 9 May 3, 2021
@mpguerra mpguerra mentioned this issue May 4, 2021
53 tasks
@dconnolly
Copy link
Contributor

Related? #1895

@dconnolly
Copy link
Contributor

OK I think I've isolated a test case:

ShieldedData { 
    value_balance: Amount(-500018759), 
    transfers: JustOutputs { 
        outputs: AtLeastOne { inner: [
            Output { 
                cv: ValueCommitment { 
                   u: "2ac3feae78daf100771f47b4e4b424391a0c3317e16b2d3d1c0d1b5b280d8667", 
                   v: "f9e9a64065c43fbc60c602dc3b58cdecbbb49ad43a3dd9ae49b48a61c493545e" 
                }, 
                cm_u: 0x4645f1361f733851c8e772e1f65cbe78d8d95b8e76aa04892a1d07b42ceca80b, 
                ephemeral_key: EphemeralPublicKey { 
                    u: "3ef344ba7c4a8f358c709280a7e4fcde76e66cf9f2a34cae81a59e11a233873e", 
                    v: "dfd77cabcfeece274285111a796b2334fe7a924240327705bb9215c842866303" 
                }, 
                enc_ciphertext: EncryptedNote("599b366203fa9ca7232f34be1fdc1778b919350976f0b9cb05ed1653b284060cdba7d093da43e19c1e725b0e21de993f3ad8f84194c4e58b128109673fdf9c4d8220f32f22ee17a4616c128b41ff2cb47bcc603b5200d0fa77ce8df9356fa99864ba26f45c6d27f9c4c35cb143293348eb6d9f650be7dc23a78b9682d72cbc0ad42de4f6f81101eb8a8a3eaea1699f7d81c7cee82c76c33f7161a97f6327a8e4befe2cde0ddff7be8373c7609092ad360c0c756863ff4136e0a4d66d0ac511de63e8b9bfd8810a26733df9a34c0e4509b60d1ab3e428dc3a31760a1c316c26610893f92ce6e23c6796aacd4b0c49ab4b151dbec0fcd857a1d0a4d257a4e5f7d9034db2382a51e02df810379e7b9f82d92f34a5dcac5cd1f40f04c9c3923b7fb7d0eb3ae8c1ed6349b772ba79979c9c0a6fb6d88784c6244a50f88ee53974dffae55641021ddfa93a147dbe26c6e3769b5f16a5c3dee1e437aebd0b333640aa9f689a57628d84608461fd749b4f154776d54c0d79585ea34d2423ce3eb110e8c521fb77ec40f1c5ae6dbf6f463ed1cde946c6a8e528731104b895dc65dc0abe0485df3a2136a7c8ac76807f3e4dd4c968bf11d6de2e5d1d0ea40b5f2e8e3b35b5945e28c08ecb68b5146fe7dcd047692e7726a3b0e099925123e5952df5a8a6ba69429f38d7e89a7a95ae0f9525d09d7703963b9b4d2fd9a523bb46cb42779253c86cc1073cf9251ddd97386b871566dff7ec5804c7b8220277341150600ee99e5a04831bec0e1dcc1aca4189226e20292920589a1192e6e10962a27344b15625ff76da58"), 
                out_ciphertext: WrappedNoteKey("d39536dfef75708aa4ab4a4bb1ceed6c87f297c6891e5ddc3c45379d6f991459b28bee977db2ea9cdf1e386648c339a118f0d891bb7fd5423074cceb2da4b1a248754fa2f304c8c77db4e01956e6742a"), 
                zkproof: Groth16Proof("9115cf83db3f2c5b0d86eb8e3784deb51e888f861249a05554f568f81d314654abf3a649526ac13f21de1a300d55f4699455b9829b81b3cc24a1ceb500a16c1b5912d5defef285604e4f489ba18ca3893525869e4102f0c5ca2855a39ba696ca14864896595e53ba498295f8cdb04235664b1268b19ec98f32f0cef611005aae828ad35c2415ffd6abca16b741a4971f926e839f329e71be02c98c5c2d99bf8f7b2b9718e8f07c6754f1b169057fc69618487ed0b419878d2e5ca5f7f6d94acc") 
}] } }, 

    binding_sig: Signature { r_bytes: [227, 234, 21, 188, 20, 27, 146, 202, 173, 230, 200, 177, 156, 28, 249, 78, 93, 228, 159, 11, 48, 28, 5, 161, 147, 11, 240, 208, 154, 43, 193, 205], s_bytes: [100, 63, 208, 29, 158, 8, 25, 152, 173, 56, 235, 126, 232, 168, 245, 201, 253, 169, 37, 69, 151, 236, 80, 123, 79, 8, 72, 6, 105, 163, 128, 6], 
_marker: PhantomData 
   } 
}

@teor2345 teor2345 changed the title Fix binding signature errors Fix sapling binding signature errors May 20, 2021
@dconnolly
Copy link
Contributor

Next steps:

  • investigate inputs to the sighash computation for passing and non-passing test cases

@teor2345
Copy link
Contributor Author

It looks like we haven't made much progress on this task, so I'm un-assigning it and moving it to sprint 14.

@dconnolly
Copy link
Contributor

OK I think I've isolated a test case:

ShieldedData { 
    value_balance: Amount(-500018759), 
    transfers: JustOutputs { 
        outputs: AtLeastOne { inner: [
            Output { 
                cv: ValueCommitment { 
                   u: "2ac3feae78daf100771f47b4e4b424391a0c3317e16b2d3d1c0d1b5b280d8667", 
                   v: "f9e9a64065c43fbc60c602dc3b58cdecbbb49ad43a3dd9ae49b48a61c493545e" 
                }, 
                cm_u: 0x4645f1361f733851c8e772e1f65cbe78d8d95b8e76aa04892a1d07b42ceca80b, 
                ephemeral_key: EphemeralPublicKey { 
                    u: "3ef344ba7c4a8f358c709280a7e4fcde76e66cf9f2a34cae81a59e11a233873e", 
                    v: "dfd77cabcfeece274285111a796b2334fe7a924240327705bb9215c842866303" 
                }, 
                enc_ciphertext: EncryptedNote("599b366203fa9ca7232f34be1fdc1778b919350976f0b9cb05ed1653b284060cdba7d093da43e19c1e725b0e21de993f3ad8f84194c4e58b128109673fdf9c4d8220f32f22ee17a4616c128b41ff2cb47bcc603b5200d0fa77ce8df9356fa99864ba26f45c6d27f9c4c35cb143293348eb6d9f650be7dc23a78b9682d72cbc0ad42de4f6f81101eb8a8a3eaea1699f7d81c7cee82c76c33f7161a97f6327a8e4befe2cde0ddff7be8373c7609092ad360c0c756863ff4136e0a4d66d0ac511de63e8b9bfd8810a26733df9a34c0e4509b60d1ab3e428dc3a31760a1c316c26610893f92ce6e23c6796aacd4b0c49ab4b151dbec0fcd857a1d0a4d257a4e5f7d9034db2382a51e02df810379e7b9f82d92f34a5dcac5cd1f40f04c9c3923b7fb7d0eb3ae8c1ed6349b772ba79979c9c0a6fb6d88784c6244a50f88ee53974dffae55641021ddfa93a147dbe26c6e3769b5f16a5c3dee1e437aebd0b333640aa9f689a57628d84608461fd749b4f154776d54c0d79585ea34d2423ce3eb110e8c521fb77ec40f1c5ae6dbf6f463ed1cde946c6a8e528731104b895dc65dc0abe0485df3a2136a7c8ac76807f3e4dd4c968bf11d6de2e5d1d0ea40b5f2e8e3b35b5945e28c08ecb68b5146fe7dcd047692e7726a3b0e099925123e5952df5a8a6ba69429f38d7e89a7a95ae0f9525d09d7703963b9b4d2fd9a523bb46cb42779253c86cc1073cf9251ddd97386b871566dff7ec5804c7b8220277341150600ee99e5a04831bec0e1dcc1aca4189226e20292920589a1192e6e10962a27344b15625ff76da58"), 
                out_ciphertext: WrappedNoteKey("d39536dfef75708aa4ab4a4bb1ceed6c87f297c6891e5ddc3c45379d6f991459b28bee977db2ea9cdf1e386648c339a118f0d891bb7fd5423074cceb2da4b1a248754fa2f304c8c77db4e01956e6742a"), 
                zkproof: Groth16Proof("9115cf83db3f2c5b0d86eb8e3784deb51e888f861249a05554f568f81d314654abf3a649526ac13f21de1a300d55f4699455b9829b81b3cc24a1ceb500a16c1b5912d5defef285604e4f489ba18ca3893525869e4102f0c5ca2855a39ba696ca14864896595e53ba498295f8cdb04235664b1268b19ec98f32f0cef611005aae828ad35c2415ffd6abca16b741a4971f926e839f329e71be02c98c5c2d99bf8f7b2b9718e8f07c6754f1b169057fc69618487ed0b419878d2e5ca5f7f6d94acc") 
}] } }, 

    binding_sig: Signature { r_bytes: [227, 234, 21, 188, 20, 27, 146, 202, 173, 230, 200, 177, 156, 28, 249, 78, 93, 228, 159, 11, 48, 28, 5, 161, 147, 11, 240, 208, 154, 43, 193, 205], s_bytes: [100, 63, 208, 29, 158, 8, 25, 152, 173, 56, 235, 126, 232, 168, 245, 201, 253, 169, 37, 69, 151, 236, 80, 123, 79, 8, 72, 6, 105, 163, 128, 6], 
_marker: PhantomData 
   } 
}

I found this failing test case with a test (see branch below) that goes through all our test vector blocks, filters out the Sapling transactions, computes the sighash and binding verification key and tries to verify the binding signature, just like in the transaction verifier service:

https://github.com/ZcashFoundation/zebra/compare/1939

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-bug Category: This is a bug I-consensus Zebra breaks a Zcash consensus rule S-needs-investigation Status: Needs further investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants