-
Notifications
You must be signed in to change notification settings - Fork 622
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
BLS12-381 aggregate signature verification support #8184
Conversation
* Minor code improvments * Improve blst code estimation * Apply cargo fmt
CC: @jakmeier; @Ekleog-NEAR; @nagisa. |
Adding host functions of this sort has required a NEP in the past (for example NEP-364 was necessary to add a |
Good point. @karim-en: please see https://github.com/near/NEPs/blob/master/neps/nep-0001.md for more information on the NEP process and of course the example above. |
#[test] | ||
fn test_bls12_381_verify_valid() { | ||
let hex_convert_error_msg = "Error during converting hex string to bytes"; | ||
let signature: Vec<u8> = <Vec<u8>>::from_hex("912c3615f69575407db9392eb21fee18fff797eeb2fbe1816366ca2a08ae574d8824dbfafb4c9eaa1cf61b63c6f9b69911f269b664c42947dd1b53ef1081926c1e82bb2a465f927124b08391a5249036146d6f3f1e17ff5f162f779746d830d1").expect(hex_convert_error_msg); |
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.
would be nice to have a reference, or a way to reproduce these {signatures, pubkeys, ...}. In Aptos I've seen that they attach a python script.
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.
fixed: 8a083a2
let message: Vec<u8> = | ||
<Vec<u8>>::from_hex("abababababababababababababababababababababababababababababababab") | ||
.expect(hex_convert_error_msg); | ||
let pubkeys_raw: Vec<Vec<u8>> = vec![ |
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.
[nit]
aren't types inferred by the compiler?
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.
fixed: aded51a
runtime/near-vm-logic/src/logic.rs
Outdated
/// Verify aggregate BLS12-381 signature for given message and | ||
/// public keys list (or one aggregate public key) | ||
/// | ||
/// The length of each public key(include aggregate one) should be 48 bytes and |
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.
/// The length of each public key(include aggregate one) should be 48 bytes and | |
/// The length of each public key (including the aggregated one) should be 48 bytes and |
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.
fixed: 89051f9
runtime/near-vm-logic/src/logic.rs
Outdated
self.get_vec_from_memory_or_register(aggregate_signature_ptr, aggregate_signature_len)?; | ||
let message = self.get_vec_from_memory_or_register(msg_ptr, msg_len)?; | ||
let pubkeys_raw = self.get_vec_from_memory_or_register(pubkeys_ptr, pubkeys_len)?; | ||
let num_pubkeys = (pubkeys_raw.len() as u64) / PUBKEY_LEN; |
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.
is there a need to add a check in case pubkeys raw length isn't a multiple of PUBKEY_LEN
?
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.
fixed: 6355c63
@blasrodri thank you for your comments and for finding time for reviewing this PR :) |
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.
overall PR looks good.
one 'meta' thing - is to add the compiler flag & re-sync to the current HEAD.
Next step - please prepare the NEP (and mention it in this PR)
@@ -62,6 +62,15 @@ extern "C" { | |||
pub_key_len: u64, | |||
pub_key_ptr: u64, | |||
) -> u64; | |||
|
|||
fn bls12_381_aggregate_verify( |
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.
new features have to be 'cfg' protected (look at how the protocol_feature_ed25519_verify looks like)
self.gas_counter.pay_base(bls12381_verify_base)?; | ||
const PUBKEY_LEN: usize = 48; | ||
|
||
let aggregate_signature = |
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.
shouldn't you check that aggregate_signature.len() is equal to 96 here?
self.gas_counter.pay_per(bls12381_verify_byte, message.len() as u64)?; | ||
self.gas_counter.pay_per(bls12381_verify_elements, num_pubkeys as u64)?; | ||
|
||
let aggregate_sig = match blst::min_pk::Signature::sig_validate(&aggregate_signature, false) |
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.
add comment explaining why you set sig_infcheck to false
Vec::with_capacity(num_pubkeys.try_into().unwrap()); | ||
|
||
for i in 0..num_pubkeys { | ||
pubkeys.push( |
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.
nit: a little bit more rusty approach would be:
let pubkeys = pubkeys_raw.chunks(PUBKEY_LEN).map(|key| {...}).collect();
let res = aggregate_sig.fast_aggregate_verify( | ||
true, | ||
&message, | ||
b"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_", |
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.
please add more explanation on where this magic string comes from.
} | ||
|
||
let res = aggregate_sig.fast_aggregate_verify( | ||
true, |
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.
mention what does the first variable do - and why do we set it to true.
return Err(VMLogicError::from(Bls1238VerifyError(Bls12381Error::IncorrectPubKeysLen))); | ||
} | ||
|
||
let num_pubkeys = pubkeys_raw.len() / PUBKEY_LEN; |
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.
can you also add a test what happens when pubkeys_raw.len is 0?
|
||
let aggregate_signature = | ||
self.get_vec_from_memory_or_register(aggregate_signature_ptr, aggregate_signature_len)?; | ||
let message = self.get_vec_from_memory_or_register(msg_ptr, msg_len)?; |
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.
what happens if message is empty? (can you add a test?)
I think this is not being actively worked on. |
BLS12-381 aggregate signature verification support
Motivation
For proper work of the Rainbow Bridge, the ETH2 Light Client should be implemented as a NEAR contract. Inside the ETH2 Light Client, it is necessary to verify BLS12-381 signatures. At the moment, there are no practical implementations of BLS signatures that (1) have been audited, (2) could be compiled into a wasm file, and (3) would fit into the gas limit.
Because of this, at the moment, verification of BLS signatures in the Rainbow Bridge is performed out-of-chain, which makes it not trustless, as we would like. This pull request is dedicated to adding the ability to verify the correctness of the aggregated BLS12-381 signature to the NEAR level, which will allow us to further verify ETH2 data completely on NEAR and make Rainbow Bridge trustless.
Main changes
bls12_381_aggregate_verify
function intoruntime/near-vm-logic/src/logic.rs
. This function verifies the aggregate bls12-381 signature for a given message and public keys list (or one aggregate public key).Library
For implementation, we use the blst library(https://github.com/supranational/blst). This library was audited and shows good performance, but can't be used directly in the smart contracts.
Gas estimation
We have two changeable parameters: message len and public keys count. For verification, we first aggregate keys and only after verifying the signature, so the message length and public keys number influence gas consumption independently.
For gas estimation we have 3 parameters:
bls12381_verify_base
-- the basic gas consumptionbls12381_verify_byte
-- the extra gas consumption for each extra message bytebls12381_verify_elements
-- the extra gas consumption for each extra public keyThe resulting formula for gas estimation:
For the gas estimation we have 3 functions:
bls12_381_verify_basic_100
-- for calculationbls12381_verify_base
verify signature for short message signed by one key.bls12_381_verify_bytes_10k_100
-- for calculationbls12381_verify_byte
verify signature for a long message with one keybls12_381_verify_elements_1000_10
-- for calculationbls12381_verify_elements
verify the signature for a short message with a lot of public keys.bls12381_verify_base = 2_478_509_560_740
bls12381_verify_byte = 253_238_393
bls12381_verify_elements = 111_561_061_580
In our case, we have a 32-bytes message with 512 public keys and the gas consumption is approximately =
60 * 10**12
(with the safe constant~180 * 10**12
). The MAX_GAS =300 * 10**12