-
Notifications
You must be signed in to change notification settings - Fork 707
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
Bandersnatch tweaks after backend update #1482
Bandersnatch tweaks after backend update #1482
Conversation
return false | ||
}; | ||
let preouts: [bandersnatch_vrfs::VrfPreOut; N] = | ||
core::array::from_fn(|i| self.outputs[i].0.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How big is the thing being cloned here? I tried to follow the types and manually see how big it is, but there are so many layers of abstraction between what's used here and the types which actually store the raw bytes (and some broken docs.rs
builds in between :P) that I just gave up. (As far as I can see this has some inline big ints inside, so I'd like to know how big exactly.) Can you core::mem::size_of
in a throwaway #[test]
and paste me the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each element is a Bandersnatch curve group element (aka curve point).
struct Affine {
x: U256, // 32 bytes
y: U256, // 32 bytes
infinity: bool,
}
The in memory size for a 64 bit machine is 72 bytes (8 bytes are given to the infinity
bool for alignment).
The constructed array size is bounded by the max number we support at the moment (MAX_VRF_IOS = 3
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@burdges can we implement Copy
for VrfInput
and VrfPreOut
(as the inner types are Copy)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright sure. Arkworks & dalek both have copy types of this size.
In memory, yeah affine curve points are two base field elements plus a flag. This one has a 32 byte base field, and serializes in 33 bytes.
Yes, arkworks has layers itself. And I made it worse for other reasons.
.expect("ring-proof serialization can't fail"); | ||
.proof | ||
.serialize_compressed(signature.signature.as_mut_slice()) | ||
.expect("ring-signature serialization can't fail"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the old rule of "proof or remove" ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more assertions in the "backend sanity checks" test.
This test basically checks our assumptions about the serialized lengths of some objects.
These serialized lengths are constant and any variation of the sizes is early catched by the test
|
||
thin_signature | ||
.proof | ||
.serialize_compressed(signature.signature.0.as_mut_slice()) | ||
.expect("serialization can't fail"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArkScale requires you "audit" arkworks types for this, but doing so is simply a grep command:
https://github.com/w3f/ark-scale/blob/master/src/lib.rs#L126
https://github.com/w3f/ark-scale/blob/master/README.md
Also VrfSignature became a scale type:
https://github.com/w3f/ring-vrf/blob/master/dleq_vrf/src/traits.rs#L97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you mean the length, yeah a test for this makes sense.
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
* sync Westmint to Millau * "Westend parachains at Millau" dashboard
As per title