Ownership issue in `RingVerifier`

[davxy]

I'm trying to integrate your recent updates into Substrate.

Everything fits nicely and is more ergonomic except for one thing (yeah there should always be one 😃).

You introduced RingVerifier(ring::RingVerifier) newtype. Unfortunately, given that in my verify function (in Substrate) I have an instance of ring::RingVerifier and I should use it to verify multiple items (tickets).

The ring_vrf_verify wrapper in Substrate is defined like this (and this is the function which is called N times to verify N tickets):

https://github.com/paritytech/polkadot-sdk/blob/6b3aa7d23e75ef110790d013f58caf2b01a18b77/substrate/primitives/core/src/bandersnatch.rs#L739-L743

We take an immutable reference to ring::RingVerifier as parameter.

It would be nice if we can easily define the RingVerifier newtime like RingVerifier<'a>(&'a ring::RingVerifier). Similarly to what has been done for the RingProver definition (i.e. to take references). And indeed I tried to do that... but...

Unfortunately this leaks the lifetime requirement to the VrfSignature type (here) and this is even more annoying. I don't thing we want to change the VrfSignature to be generic over a lifetimes.

As VrfSignature uses the generic Verifier only to extract the VrfProof associated type, and H maybe we can make VrfSignature generic over these two? In order to don't leak the RingVerifier lifetime.

A brutal (unsafe) approach is in our code to reinterpret cast the ring:RingVerifier we have in the parameter to a RingVerifier. But ... no, this is not really the approach I want to use and should be the very last resort.

What your suggest?

[burdges]

Yeah RingVerifier(ring::RingVerifier) only exists because of the orphan rules. Also similarly, tuples not being #[fundemental] is why `RingPover<'a> exists. We should do whatever's most convenient.

I called it like https://github.com/burdges/verifiable/blob/try2/src/ring_vrf_impl.rs#L141 in the crate for Gav, but yeah we can make it more like RingProver. Would this make sense?

pub struct RingVerifier<'a>(&'a pub ring::RingVerifier);

impl<'a> From<&'a ring::RingVerifier> for RingVerifier { .. }

It gives me an idea..

We'll always invoke KZG::kzg().init_ring_verifier(verifier_key: ring::VerifierKey). We should ask @swasilyev how long that takes, but if it takes a while then we could cache the VerifierKey inside the runtime like this:

struct CachedVerifier {
    key: ring::VerifierKey,
    run: ring::RingVerifier,
}

static VERIFIERS: Mutex<LinkedList<Verifier>> = Mutex::new(LinkedList::new());

pub fn ring_verifier(key: &ring::VerifierKey) -> RingVerifier<'static> {
    fn unsafety(run: &ring::RingVerifier) -> RingVerifier<'static> {
            // We only ever append so 'static remains safe here.
            RingVerifier(unsafe { run as *ring::VerifierKey as &'static ring::RingVerifier })
    }

    let mut verifiers = VERIFIERS.lock();
    for v in *verifiers {
        if v.key == key {
            return unsafety(v.run);
        }
    }
    let run = KZG::kzg().init_ring_verifier(key);
    let r = unsafety(run);
    verifiers.append(CachedVerifier { key: key.clone(), run, });
    r
}

It'd be a nasty memory leak in the host, but runtimes die after one block even this simple caching strategy should work in runtimes. I'd expect the memepool runs init_ring_verifier for every tx, but then the block itself exploits the static mutex.