mimblewimble / rust-secp256k1-zkp

ZKP fork for rust-secp256k1, adds wrappers for range proofs, pedersen commitments, etc
Creative Commons Zero v1.0 Universal
57 stars 51 forks source link

commitments generated with unexpected first byte (0x08)? #3

Closed antiochp closed 7 years ago

antiochp commented 7 years ago

Grin currently generates commits that look like this - Commitment(0251fb65e6cd4ae54749b59e139935dd4cfa10f0694aa8a2645c3950fe8785b198)

But rust-secp256k1-zkp generates commits that look like this - Commitment(08032fc3685013a35124c0275457e97a7ff0046ba763fa45409b95e8b3eae53425)

ffi::secp256k1_ec_pubkey_parse calls into secp256k1_eckey_pubkey_parse which checks that a 33 byte public key starts with 0x02 or 0x03

Q) Why is rust-secp256k1-zkp creating commitments that appear to be invalid?

antiochp commented 7 years ago

The following test can reproduce this (see #2 for the failing test) -

fn test_to_pubkey() {
    let secp = Secp256k1::with_caps(ContextFlag::Commit);
    let blinding = SecretKey::new(&secp, &mut OsRng::new().unwrap());
    let commit = secp.commit(5, blinding).unwrap();
    println!("{:?}", commit);
    commit.to_pubkey(&secp).unwrap();
}

And generates the following -

thread 'pedersen::tests::test_to_pubkey' panicked at 'called `Result::unwrap()` on an `Err` value: InvalidPublicKey', src/libcore/result.rs:860:4
antiochp commented 7 years ago

Wondering if this is related to the generator that now gets passed into ffi::secp256k1_pedersen_commit?

apoelstra commented 7 years ago

Commitments are not pubkeys. They aren't interchangeable and shouldn't parse as each other. This does differ from the old behaviour; in the secp included with Elements Alpha everything was more-or-less "untyped" and treated haphazardly. We considered this a dangerous API and changed it.

antiochp commented 7 years ago

So guessing this will need to be rewritten somehow - https://github.com/ignopeverell/grin/blob/64934689593ce2331a4d3488d5ae95de90e8b70a/core/src/core/transaction.rs#L211

    pub fn verify_sig(&self, secp: &Secp256k1) -> Result<TxKernel, secp::Error> {
        let rsum = self.sum_commitments(secp)?;

        // pretend the sum is a public key (which it is, being of the form r.G) and
        // verify the transaction sig with it
        let pubk = rsum.to_pubkey(secp)?;
        ...

Not even sure right now how we would go about getting the public key from the txn.

antiochp commented 7 years ago

PR now includes a to_two_pubkeys() workaround (one of these is a valid public key).