[davxy]

Fixes paritytech/eng_findings#43.

BatchVerifier derived the KZG randomizer of each proof from that proof alone, so a prover who controls the whole batch could pick invalid proofs whose randomized errors cancel (Wagner k-sum, see paritytech/eng_findsings#43).

Now the batch transcript absorbs the entropy of every pushed proof and all randomizers are drawn from the final state at verify, so each depends on the whole batch.

Bumps both crates to 0.0.11.

[burdges]

It pushed two randomizers before but now only pushes one? What else changed in the logic?

[burdges]

Was r2 the weak randomizer for the batch before?

[drskalman]

no both r1 and r2 were weak. It was just that accumulator was dealing with evalutating at zeta and zeta * omega separately (so we had r1 as random coefficient of the evalution at zeta and r2 for zeta * omega. Now @davxy has written generic code to deal with just as a generic point, so the function is called twice.

[burdges]

Alright the core bug fix in w3f-ring-proof/src/multi_ring_batch_verifier.rs‎ looks okay to me. self.transcript is the transcript of the batch verifier, independent of the transcripts for each individual proof. It looks fine to me, although I've not yet pulled up the Openings::new logic side by side with the original, so I'll get back to this after lunch.

[drskalman]

Mostly opiniated nits but I think the namings are genuinely confusing.

A less intrusive fix would have been that each r depends on all r submitted before, would have been a two line change, I know @burdges thinks it is not secure but vanilla Wagner doesn't apply to it I think at least.

[drskalman]

nit: name is misleading, because this is an Opening, not a Claim: ClaimOpening, PointOpening?

[drskalman]

Nit: This is hard code two point openings, I think the name should indicate that (like the similar object we have for generic Pcs).

[swasilyev]

We've similar structs at

ring-proof/w3f-plonk-common/src/verifier.rs

Line 20 in 6939e71

pub struct PcsOpeningAt2Points<F: PrimeField, C: Commitment<F>> {

(or also

ring-proof/w3f-plonk-common/src/prover.rs

Line 24 in 6939e71

pub struct PcsOpeningAt2Points<F: PrimeField> {

), but that can be refactored later.

[drskalman]

Isn't it better that user force to hand in the transcript/fs object to the accumulator so they don't mess up with the randmoness per proof somehow (I think we had that bug before that we submited that r didn't depened on the proof). Or if you don't want to do that, maybe leave a comment here on what r is acceptable so poeple won't abuse it in future again.

[drskalman]

same nit here maybe accoumulate_opening_point ?

[davxy]

Thanks for the reviews. I've merged it as-is since it's needed ASAP, good enough for now, and I'm actually OOO sick 🤒. We can always refactor and improve it later.

[burdges]

A less intrusive fix would have been that each r depends on all r submitted before, would have been a two line change, I know @burdges thinks it is not secure but vanilla Wagner doesn't apply to it I think at least.

It impacts how big you choose r. We typically have only 128 bit r when doing this, because that's a bit faster in the MSM. Here, the r have 256 bits, so you could get away with more. We do not afaik pay any significant price for those 256 bit r here, since we've stacked on a few layers of randomizers, but that seems kinda unique to plonk or similar RO snarks.

I'd probably have done this using some more generic code, which would've used 128 bit r because that'd make a difference in other protocols. In principle, you could accept an external Fiat-Shamir seed, like the block hash. We do not expose that for the runtime however, so that'd be a much bigger change. Another thing you can do is accumulate the hash of all the tx, place that into the finished batched type that you serialize and put into the block, and then verify it when checking the block. This risks some complexity when folks want to do bad stuff to blocks, like remove tx later or whatever.