zebra: transaction::tests::v4_with_modified_joinsplit_is_rejected fails with unexpected verifier error

Here’s my summary:

We modify the proof here https://github.com/zcashfoundation/zebra/blob/58bd898f5be362d7c67f8ba36eeac172482f4072/zebra-consensus/src/transaction/tests.rs#L1931. As already pointed out, this altered data is then hashed here https://github.com/zcashfoundation/zebra/blob/58bd898f5be362d7c67f8ba36eeac172482f4072/zebra-consensus/src/transaction.rs#L629, which makes the resulting sighash invalid when Zebra checks the tx signatures against it. The proof and signatures are checked concurrently, and the bug shows up when the signature check finishes first. It seems there are also checks on Ed25519 against the invalid sighash, which should also fail, and it’s interesting these have never shown up in the logs. It’s only been RedJubJub sigs. I guess Ed25519 might be slower.

We could alternatively modify the signature when modifying the proof so only the proof verification check would fail if modifying the proof makes the signature invalid?

Since the tx comes from real blocks, we don’t have the original keys. I’m now briefly checking if we could use made-up keys and regenerate the tx sigs for the test. If not, I’ll try to run the test in a loop with a constant threshold until it passes and see if a loop would make the test much slower. If yes, I guess we’ll just have to accept a wrong signature as the correct outcome of the test.

This bug is still not fixed, unfortunately:

---- transaction::tests::v4_with_modified_joinsplit_is_rejected stdout ---- The application panicked (crashed). Message: assertion failed: (left == right) left: Err(InternalDowncastError("downcast to known transaction error type failed, original error: InvalidSignature")), right: Err(Groth16("proof verification failed")) Location: zebra-consensus/src/transaction/tests.rs:2163

https://github.com/ZcashFoundation/zebra/actions/runs/5225593214/jobs/9435269099?pr=6878#step:10:1321

We could increase the number of test cases, or accept either error as correct.

How often is this happening? Can this wait until next sprint?

If it is only happening once every few weeks, then I think this ticket could be closed as “not planned”. Testing mining is a much higher priority right now.

@arya2 what do you think?

At first I thought the test could be changing the same transaction

The async_checks are unordered and the verify_sapling_shielded_data check may occasionally be faster.

I don’t see it filtering out v5 transactions for the test so I wonder if it may be calling verify_orchard_shielded_data as well.

Accept either error?

We could alternatively modify the signature when modifying the proof so only the proof verification check would fail if modifying the proof makes the signature invalid?

Yep, either way would work, let’s do whatever is faster?

At first I thought the test could be changing the same transaction

The async_checks are unordered and the verify_sapling_shielded_data check may occasionally be faster.

I don’t see it filtering out v5 transactions for the test so I wonder if it may be calling verify_orchard_shielded_data as well.

Accept either error?

We could alternatively modify the signature when modifying the proof so only the proof verification check would fail if modifying the proof makes the signature invalid?

This is really baffling, I can’t come up with a scenario where that happens. At first I thought the test could be changing the same transaction, but that’s not it, since test_transactions deserializes blocks and thus returns fresh references each time. The verifier is also freshly created each time.