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

ringct: MLSAG speedup and zero-hash check #5707

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@SarangNoether
Copy link
Contributor

commented Jun 28, 2019

  • Removes redundant point conversions in MLSAG verification
  • Replaces public key hash-to-point zero checks with round hash-to-scalar zero checks
  • Updates tests
  • Improves error messages
@moneromooo-monero
Copy link
Contributor

left a comment

I don't really like the assert and {} bikeshedding growing the diff, but hey, give and take.

@SarangNoether

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

I can revert those if that's what we really want. Will be adding some additional speedups to this, so don't merge yet.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

I think I'm just easily annoyed at comparing things just to work out that there's no actual difference in code, so it feels like busywork :P Don't mind me, I'll go grumble in a corner.

@SarangNoether

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Added a zero-check to key images per discussion with @b-g-goodell, which replaces existing public key hash checks.

hash_to_p3(pk[i][j], hash8_p3);
ge_p2 R_p2;
ge_double_scalarmult_precomp_vartime(&R_p2, rv.ss[i][j].bytes, &hash8_p3, c_old.bytes, Ip[j].k);
key R;

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 4, 2019

Contributor

This is masking an existing variable R on the stack. The best action looks to be dropping the existing one above.

This comment has been minimized.

Copy link
@SarangNoether

SarangNoether Jul 5, 2019

Author Contributor

Cleaned up in new commit.

@@ -317,6 +328,7 @@ namespace rct {
toHash[ndsRows + 2 * ii + 2] = L;
}
c = hash_to_scalar(toHash);
CHECK_AND_ASSERT_MES(!(c == rct::zero()), false, "Bad signature hash");

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 4, 2019

Contributor

This is a valid hash ... ? It would be (unfathomably) rare, but I'm not sure why this is explicitly banned.

This comment has been minimized.

Copy link
@SarangNoether

SarangNoether Jul 5, 2019

Author Contributor

It could remove the signer's need to know a private key.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 5, 2019

Contributor

But this value is directly from a hash function - the attacker would have to find a pre-image mapping to zero. The check is probably worth it, because this would break the implementation in the next iteration (nothing is being added on line 326).

This comment has been minimized.

Copy link
@SarangNoether

SarangNoether Jul 5, 2019

Author Contributor

Yes, it would require a preimage of zero, making it extremely unlikely. One of the Bulletproofs auditors pointed out a similar check was missing in Schnorr signature verification code, and it seemed wise (and cheap) to add it here as well.

// the signer knows a secret key for each row in that column
// Ver verifies that the MG sig was created correctly
// Hash a key to p3 representation
static void hash_to_p3(const key &k, ge_p3 &hash8_p3) {

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 4, 2019

Contributor

This now aliases a function in src/ringct/rctOps.cpp which itself is an alias for a function in src/crypto/crypto.cpp.

Merge one of these functions somewhow. The existing one in src/ringct/rctOps.cpp can call this function and then immediately convert to bytes (assuming that code somewhere is still using that function).

This can also return-by-value in its current usage, although the benefit seems low since ge_p1p1_to_p3 takes an out variable anyway.

CHECK_AND_ASSERT_MES(!(Hi == rct::identity()), false, "Data hashed to point at infinity");
addKeys3(R, rv.ss[i][j], Hi, c_old, Ip[j].k);

// Compute R directly

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 4, 2019

Contributor

Mainly to ensure I didn't misread this code - the check here was only used as input to the hash function. So if it were the identity point, it couldn't manipulate any of the other calculations ... ?

This comment has been minimized.

Copy link
@SarangNoether

SarangNoether Jul 5, 2019

Author Contributor

A forgery using hashed-to-zero public keys would require that the adversary "trapdoor" both the left and right hash inputs for a given index. We can remove the checks on each hashed public key and replace them with a single check on the key image; same effect, but cheaper.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 5, 2019

Contributor

The response wasn't exactly what I was getting at - if R were used in some math operation the situation might be different. But instead it is only being fed into a hash function.

This comment has been minimized.

Copy link
@SarangNoether

SarangNoether Jul 5, 2019

Author Contributor

Yes, R is only used for its byte representation to be fed into the hash function. I meant that the checks on line 308 have been removed and replaced with a single check on the key image instead; the speedup comes from the portion starting at line 313, where we avoid the hashToPoint byte conversion that's undone by addKeys3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.