Worker selection guard + Additional Unit Tests #121
Conversation
Merging new version 0.1.3 into master
…ainst a sealed marker
# Conflicts: # enigma-principal/app/Cargo.lock # enigma-principal/app/src/epoch_u/epoch_provider.rs # enigma-principal/app/src/epoch_u/epoch_types.rs
…essing exception scenarios.
…gma.specs.js`. TODOs: 1) Create a better test for the exception scenario of a failed Eth tx; 2) Unit test `get_state_keys` with mock EpochProvider that passes the worker selection.
… test runs before warming up the storage dirs. The fix is temporary. I'm currently working on improving the unit test organization. I will create a PR with a more elegant permanent fix. I did not detect this locally because the storage dirs existed. I will account for this in the future.
… local build and a failed build on the CI
…hereum-types` broke some things in our code.
# Conflicts: # enigma-principal/app/Cargo.lock
# Conflicts: # enigma-core/app/Cargo.lock # enigma-principal/app/Cargo.lock # enigma-principal/app/Cargo.toml # enigma-tools-u/Cargo.toml
fredfortier
changed the title
lacabra
changed the title
lacabra
changed the title
# Conflicts: # enigma-principal/enclave/Cargo.lock
| Token::Array(self.worker_params.stakes.iter().map(|s| Token::Uint(*s)).collect()), | ||
| ]; | ||
| encode(&tokens) | ||
| let raw_seed = H256::from(self.seed).0.to_vec(); |
There was a problem hiding this comment.
Can you please explain what are you doing here exactly? why don't use use the regular hash_multiple? Implementing the same thing multiple times is a bad idea.
(especially something as sensitive as this)
There was a problem hiding this comment.
Yes, that's fine. I wasn't aware of the hash_multiple then but I have no problem using it. That said, I don't believe that hash_multiple behave as expected so the signature won't recover. I just need to better understand how hash_multiple represents array lengths.
There was a problem hiding this comment.
@elichai @fredfortier
Definitely KM node should use a general core implementation of hashing for multiple values, and not use a special function for specific values. The problem is that today in core there is no correct implementation of hashing of multiple values that may include nested arrays. Also the @fredfortier's implementation of raw_encode is not good as it may lead to the same output for different inputs. I and @elichai found an encoding for nested arrays, for which I did a formal proof that for any two different inputs, the outputs will be always different: https://github.com/enigmampc/protocol-discovery/blob/master/docs/hash_mul_nested.pdf.
So it seems to me that:
- The correct implementation should be added to core
- KM node should use this implementation
- The implementation of hashing in the contract for signature verification should be changed accordingly.
Meanwhile before 1 is done, I am OK to leave raw_encode as is, because signature verification works now, and we can progress with integration tests and other stuff. Once 1 is done, we will do 2 and 3 ASAP.
There was a problem hiding this comment.
Also the @fredfortier's implementation of raw_encode is not good as it may lead to the same output for different inputs.
@moriaab Do you mind providing an example? This isn't my implementation, I implemented exactly what I was told the algorithm was in order to match the contract.
# Conflicts: # enigma-principal/app/src/epoch_u/epoch_provider.rs
| Token::Array(self.worker_params.stakes.iter().map(|s| Token::Uint(*s)).collect()), | ||
| ]; | ||
| encode(&tokens) | ||
| let raw_seed = H256::from(self.seed).0.to_vec(); |
There was a problem hiding this comment.
Yes, that's fine. I wasn't aware of the hash_multiple then but I have no problem using it. That said, I don't believe that hash_multiple behave as expected so the signature won't recover. I just need to better understand how hash_multiple represents array lengths.
| pub fn setup_epoch_storage() { | ||
| let mut path = storage_dir(ENCLAVE_DIR).unwrap(); | ||
| path.push(EPOCH_DIR); | ||
| fs::remove_dir_all(path).unwrap_or_else(|_| println!("Epoch dir already empty")); |
There was a problem hiding this comment.
FYI you have expect(msg) for this
There was a problem hiding this comment.
Doesn't expect panic just like unwrap? I want it do continue of the directory is already removed (possibly by another test running concurrently).
# Conflicts: # enigma-core/app/Cargo.lock # enigma-core/enclave/Cargo.lock # enigma-principal/app/Cargo.lock # enigma-principal/enclave/Cargo.lock
There was a problem hiding this comment.
Approved on the basis of @elichai's previous approval. Fred and I noticed the addition of 3 very old commits, and we were trying to remove them by changing the base branch and change it back to develop. That did not change those commits, but reset the approval.
Key features added: