Conversation
rachel-bousfield
changed the base branch from
master
to
arbitrator-return-optimization
rachel-bousfield
changed the base branch from
arbitrator-return-optimization
to
master
Codecov Report
@@ Coverage Diff @@
## redo-devnet-reinit-1 #586 +/- ##
========================================================
- Coverage 51.92% 50.82% -1.10%
========================================================
Files 223 223
Lines 26043 26610 +567
Branches 497 486 -11
========================================================
+ Hits 13522 13525 +3
- Misses 11142 11700 +558
- Partials 1379 1385 +6 |
tsahee
left a comment
tsahee
left a comment
There was a problem hiding this comment.
some naming comments/thoughts. Feel free to ignore.
still reviewing: testsuit itself
arbitrator/prover/src/machine.rs
Outdated
| let main_module = &modules[main_module_idx]; | ||
| // Rust support | ||
| if let Some(&f) = main_module.exports.get("main") { | ||
| if let Some(&f) = main_module.exports.get("main").filter(|_| language_support) { |
There was a problem hiding this comment.
that filter is a little backwards..
maybe just put lines 1041..1109 under if(language_support?)
There was a problem hiding this comment.
I wish if let Some() = expr && conditional was stable. For now let's keep this since otherwise there's a comment with a URL that'll be too wide
arbitrator/prover/src/machine.rs
Outdated
| pub fn from_binaries( | ||
| libraries: &[WasmBinary<'_>], | ||
| bin: WasmBinary<'_>, | ||
| language_support: bool, |
There was a problem hiding this comment.
runtime_support?
runtime_emulation?
There was a problem hiding this comment.
Renamed to runtime_support :)
| int32_t wavm__i32_trunc_sat_f32_s(uint32_t v) { | ||
| // signed truncation is defined over (i32::min - 1, i32::max + 1) | ||
| float32_t max = {0x4f000000}; // i32::max + 1 = 0x4F000000 | ||
| float32_t min = {0xcf000001}; // i32::min - 1 = 0xCF000000 (adjusted due to rounding) |
There was a problem hiding this comment.
value and comments don't match
There was a problem hiding this comment.
0xCF000000 is the min, but our comparator is inclusive so it's up by 1. The max rounds differently
| return (1u << 31) - 1; | ||
| // signed truncation is defined over (i32::min - 1, i32::max + 1) | ||
| float32_t max = {0x4f000000}; // i32::max + 1 = 0x4F000000 | ||
| float32_t min = {0xcf000001}; // i32::min - 1 = 0xCF000000 (adjusted due to rounding) |
There was a problem hiding this comment.
value and comment don't match
There was a problem hiding this comment.
0xCF000000 is the min, but our comparator is inclusive so it's up by 1. The max rounds differently
| @@ -198,39 +202,113 @@ uint8_t wavm__f32_ge(uint32_t va, uint32_t vb) { | |||
| } | |||
|
|
|||
| int32_t wavm__i32_trunc_f32_s(uint32_t v) { | |||
There was a problem hiding this comment.
is it possible to merge the truncated and non-truncated versions?
There was a problem hiding this comment.
The go and rust compilers currently do not generate truncating floating point ops, but they might one day and it's probably better to keep things general
| if (f32_le(max, f)) { | ||
| return (1u << 31) - 1; | ||
| // signed truncation is defined over (i32::min - 1, i32::max + 1) | ||
| float32_t max = {0x4f000000}; // i32::max + 1 = 0x4F000000 |
There was a problem hiding this comment.
I think the best way to create these would be with funcs like i32_to_f32 (will cost CPU cycles, but probably worth the readability).
If not.. maybe at least have constants like I32MAX_F32?
There was a problem hiding this comment.
I hesitate to do that because in each case it's not exactly the min or max integers as floating points, it's the first integer that is out of bounds. It's very particular hex for the funcs they're in so I think that, although not ideal, we should have the hex inline
| return 0; | ||
| } | ||
| if (f32_le(max, val)) { | ||
| return 2147483647; |
There was a problem hiding this comment.
these would be more readable in hex
There was a problem hiding this comment.
Agreed but due to C parsing that'd cause type warnings in the negative case
|
tests pass for me after rebuilding |
Remove unused WAVM opcodes
Remove wavm blocks
PlasmaPower
left a comment
PlasmaPower
left a comment
There was a problem hiding this comment.
LGTM except a couple minor code cleanup comments
use remote types
rachel-bousfield
changed the base branch from
devnet-reinit-1
to
redo-devnet-reinit-1
Co-authored-by: Goran Vladika <goran.vladika@gmail.com> Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
3 participants
Introduces the official wasm testsuite to ensure compliance with the standard and fixes current deviations including
Additionally, this PR
language_supportflag for enabling rust & go supportstart- andstop-merkle_cachingfor faster proof generationone-step-proof.tscausing tests to sometimes not run