Fix external storage range

[ethanfrey]

Closes #508
Builds on test case reported by #509 and develops a solution.

All discussion was in #509, but then wanted to change the branch name properly.

[maurolacy]

Just out of curiosity, I wonder why these fail in the vm context.

[ethanfrey]

They return something, but likely a pointer to stack, which is overwritten, not a pointer to heap.

[maurolacy]

I see. The db_scan is out of scope!

[reuvenpo]

what do you mean by out of scope?
Looking at this diff it's not clear to me what the difference is between the two versions.

[ethanfrey]

Me either. But the compiler does something so they work very different.

Check out the WIP: lots of debug println commit and run it, and then switch out what is commented out. This is odd behavior, but we are grabbing a result that was created in a closure and dereferenced inside an Option, and I am assuming something happens a bit odd, so we get a stack pointer not a heap pointer.

[maurolacy]

I guess there are no safe (in the sense of compiler warnings and such) ways to do these mappings between languages / frameworks.

[maurolacy]

OK, I was looking into this, and the alternative way to do it, if you cannot avoid using a raw pointer, is to use Box::into_raw. The advantage is that the returned pointer is to the original value on the heap. So, it avoids copying of the value to the stack, and the scoping issues.

The code also ends up being somewhat clearer:

        let start_ptr = match start {
            Some(reg) => Box::into_raw(reg) as u32,
            None => 0,
        };
        let end = end.map(|e| build_region(e));
        let end_ptr = match end {
            Some(reg) => Box::into_raw(reg) as u32,
            None => 0,
        };

Now, after into_raw you're dealing with a raw (therefore unmanaged) pointer. Being on the heap, it's your responsibility to release it. It has to be manually released with something like:

        if start_ptr != 0 {
            unsafe { drop(Box::from_raw(start_ptr as *mut Region)); }
        }
        if end_ptr != 0 {
            unsafe { drop(Box::from_raw(end_ptr as *mut Region)); }
        }

This can still probably be improved a bit by avoiding the cast tu u32in the match (that is, casting in the db_scan parameters), in order to avoid recasting to *Region.

[maurolacy]

Something like:

        let start = start.map(|s| build_region(s));
        let start_ptr = match start {
            Some(reg) => Box::into_raw(reg),
            None => null_mut(),
        };
        let end = end.map(|e| build_region(e));
        let end_ptr = match end {
            Some(reg) => Box::into_raw(reg),
            None => null_mut(),
        };
        let iterator_id = unsafe { db_scan(start_ptr as u32, end_ptr as u32, order as i32) };
        let iter = ExternalIterator { iterator_id };
        if start_ptr != null_mut() {
            unsafe { Box::from_raw(start_ptr); }
        }
        if end_ptr != null_mut() {
            unsafe { Box::from_raw(end_ptr); }
        }
        Box::new(iter)

[ethanfrey]

Nice suggestion. Do you want to make a branch with those changes and see if it passes tests?

I actually kind of like the current way as it is more explicit and fewer if statments. But the Box::into_raw is a nice addition as well.

You can make a PR proposing this as a cleaner logic and we can iterate on it and see if anyone has better ideas. I really wanted to get the thing fixed first of all, so iterators work in the contracts, but happy for a refined version in the future. Whatever is easiest to read really.

[maurolacy]

Sure. I tested it, and it works. I'll make a PR.

[ethanfrey]

Given Simon is on vacation and this is an important bugfix that doesn't affect other sections, I will merge this.

[webmaster128]

Nice one

[webmaster128]

Why is there an additional a byte in this limit?

[ethanfrey]

End is exclusive I believe and I wanted to ensure we included it.
(I know start is inclusive)

[webmaster128]

Ah, I see. Any reason for having the contract's API flipped compared to the storage access?

Storage: start included; end excluded
API: start excluded (in late); end included (in early)

If we change the API to

pub struct ListResponse {
    /// List an empty range, both bounded
    pub empty: Vec<u32>,
    /// List all IDs lower than 0x20
    pub early: Vec<u32>,
    /// List all IDs starting from 0x20
    pub late: Vec<u32>,
}

this gets simpler

[ethanfrey]

Sure, sounds proper. Please make an issue or a PR for this. I'll be doing orga for the next day or two and not get to coding very soon.