enable batching multiple sign calls in one batch transaction #909
Conversation
ppca
force-pushed
the
xiangyi/fix_same_receipt_id_sign_calls
branch
from
d565657 to
0e87e27
Compare
ppca
force-pushed
the
xiangyi/fix_same_receipt_id_sign_calls
branch
from
0e87e27 to
477c860
Compare
There was a problem hiding this comment.
Looks good overall
This PR can close https://github.com/near/mpc/issues/763 too. If so, let's add a separate test.
| @@ -166,6 +172,7 @@ impl VersionedMpcContract { | |||
| "sign: predecessor={predecessor}, payload={payload:?}, path={path:?}, key_version={key_version}", | |||
| ); | |||
| env::log_str(&serde_json::to_string(&near_sdk::env::random_seed_array()).unwrap()); | |||
| self.mark_request_received(&request); | |||
There was a problem hiding this comment.
I assume a duplicate request with the same payload, epsilon, and crypto hash will not pass the check on line 170 and throw an error on 184. Let's have a separate test for that (batch transaction with 2+ requests with the same payload and path). And it should not break the counter.
BTW, the error should not be "PayloadCollision" anymore. Should be SignRequestCollision.
There was a problem hiding this comment.
Done. Added test_batch_duplicate_signature where 2 identical requests are sent. The test checks that tx fails with RequestCollision.
There was a problem hiding this comment.
hmm, I'm against changing the name, because it's going to be a breaking change now if somebody was directly matching against the name of the error
| pub receipt_id: CryptoHash, | ||
| pub receipt_id: [u8; 32], |
There was a problem hiding this comment.
why change these to non-CryptoHash type?
There was a problem hiding this comment.
ok, so talked with @volovyks about this one and it seems like we're moving to a more generic naming for these because other chains don't have this. We can change it to some other type later
There was a problem hiding this comment.
Have you tried using the newly introduced SignRequestIdentifier everywhere?
| #[derive(Debug, Clone, Eq, Hash, PartialEq)] | ||
| pub struct SignRequestIdentifier { | ||
| pub receipt_id: [u8; 32], | ||
| pub epsilon: Vec<u8>, | ||
| pub payload: Vec<u8>, | ||
| } | ||
|
|
||
| impl SignRequestIdentifier { | ||
| pub fn new(receipt_id: [u8; 32], epsilon: Scalar, payload: Scalar) -> Self { | ||
| Self { | ||
| receipt_id, | ||
| epsilon: borsh::to_vec(&SerializableScalar { scalar: epsilon }).unwrap(), | ||
| payload: borsh::to_vec(&SerializableScalar { scalar: payload }).unwrap(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why are we making epsilon and the payload vectors? Is it for hashing? If so, just implement hashing for SerializableScalar
There was a problem hiding this comment.
This will come in a refactoring PR
| @@ -356,13 +357,13 @@ impl SignatureManager { | |||
| #[allow(clippy::result_large_err)] | |||
| fn retry_failed_generation( | |||
| &mut self, | |||
| receipt_id: ReceiptId, | |||
| sign_request_identifier: SignRequestIdentifier, | |||
There was a problem hiding this comment.
this is quite long now as a variable name. Let's just simplify all of them to just request_id: SignRequestIdentifier
There was a problem hiding this comment.
I'll do all the rafactoring in an upcoming PR. Want to prioritize making batch multiple sign possible on dev first.
|
Talked with @volovyks this morning, since many people have been waiting for batch tx with multiple signs to work on dev, I want to prioritize merging this fix, then in the next PR refactor the code. @ChaoticTempest |
changes:
mark_request_received(request)with <request, None> in sign(), so that same payloads in the same batch tx will not be able to go thru. Only one should go thru;SignRequestIdentifier {receipt_id, epsilon, payload}to index sign requests instead ofreceipt_idin backend code. When sending multiple sign() in one batch tx, the receipt_id and entropy of all of them are exactly same. What should differentiate is epsilon (which reflects path + predecessor) or payload;