Change ed19 to work with arbitrary-length messages
#795
Conversation
| @@ -101,6 +100,10 @@ pub fn default_gas_costs() -> GasCostsValues { | |||
| base: 2, | |||
| units_per_gas: 214, | |||
| }, | |||
| ed19: DependentCost::HeavyOperation { | |||
| base: 3000, | |||
| gas_per_unit: 0, | |||
There was a problem hiding this comment.
is it heavy or light? I would've thought its light operation given that it's just doing a sha3-512 over the input and all the other hash opcodes are light.
There was a problem hiding this comment.
I changed it to light and copied gas_per_unit from s256 which should be quite similar.
Dentosal
changed the title
ed19 modificationsed19 to work with arbitrary-length messages
| base: v3.ed19, | ||
| gas_per_unit: 0, | ||
| }, | ||
| GasCostsValues::V4(v4) => v4.ed19, |
There was a problem hiding this comment.
Why was it HeavyOperation before, but LightOperation now?
There was a problem hiding this comment.
Probably just an oversight. Brandon was wonderin which one it should be, and changing it seemed to make sense. #795 (comment)
| @@ -59,7 +59,7 @@ where | |||
| )) | |||
| })?, | |||
| ); | |||
| if pc < self.registers[RegId::IS] || pc >= self.registers[RegId::SSP] { | |||
| if pc < self.registers[RegId::IS] || pc >= self.registers[RegId::SP] { | |||
There was a problem hiding this comment.
Why did this get changed to SP from SSP?
There was a problem hiding this comment.
The PR originially included changes from an old version of the Blob TX PR. Git merged this part incorrectly and I missed it when reviewing the code myself. Removed in 1c56802.
There was a problem hiding this comment.
I re-reviewed myself as well to make sure nothing else was missed.
fuel-vm/src/tests/crypto.rs
Outdated
| #[test_case(31, 0 => (); "Zero defaults to 32")] | ||
| #[test_case(31, 100 => (); "Way over the end")] | ||
| #[test_case(0, 32 => (); "Empty range, goes over it")] | ||
| fn ed25519_verify_c_plus_d_gt_vmaxram(offset: u16, len: u32) { |
There was a problem hiding this comment.
I feel like the name should mention that we should expect it to "overflow". All of these tests have to do with the offset causing the overflow, so maybe the name should reflect that.
The test_case names don't really inform me that much either, but I think that has more to do with the base name not being clear enough.
There was a problem hiding this comment.
The other tests were named like this as well, I was trying to be consistent. But I just fixed them all in eb11817 now.
Closes #793. Spec PR FuelLabs/fuel-specs#600
Checklist
Before requesting review