Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

double check the output of TraceThis.obs #434

Open
ivoysey opened this issue Mar 29, 2022 · 1 comment
Open

double check the output of TraceThis.obs #434

ivoysey opened this issue Mar 29, 2022 · 1 comment
Assignees
Labels

Comments

@ivoysey
Copy link
Collaborator

ivoysey commented Mar 29, 2022

the TraceThis test produces -2438. sketched out in Python,

class IntContainer:
    def __init__(self):
        self.x = 0

    def set(self, new):
        self.x = new

    def get(self):
        return self.x


class IntContainerBox:
    def __init__(self):
        self.ic1 = IntContainer()
        self.ic2 = IntContainer()
        self.ic3 = IntContainer()

    def set3(self, new1, new2, new3):
        self.ic1.set(new1)
        self.ic2.set(new2)
        self.ic3.set(new3)

    def getsum(self):
        return self.ic1.get() + self.ic2.get() + self.ic3.get() + 4


class IntContainerBoxBag:
    def __init__(self):
        self.icb1 = IntContainerBox()
        self.icb2 = IntContainerBox()

    def set2(self, arg1, arg2, arg3):
        self.icb1.set3(arg1 * arg1, arg2 * arg2, arg3 * arg3)
        self.icb2.set3(arg1 + arg1, arg2 + arg2, arg3 + arg3)
        print(f"inner: {self.icb1.getsum()}, {self.icb2.getsum()}")

    def get(self):
        return self.icb1.getsum() - self.icb2.getsum()


ic1 = IntContainerBoxBag()
ic1.set2(5, 20, -19)

ic2 = IntContainerBoxBag()
ic2.set2(500, 0, 1219)

print(ic1.get() + ic2.get())

the same test produces 1733297. i am more confident that the python version is correct, so i think this is a sign of a bug. nothing crashes during a run of that test case, just the answer is wrong.

one first step would be to run it on the Fabric framework and see what the answer is there, which i don't know how to do immediately.

@ivoysey
Copy link
Collaborator Author

ivoysey commented Apr 1, 2022

i removed this test case from the JSON listing in #430. prior to that PR, it would run and produce a bad answer (as above). now it runs out of gas. there are a few reasons this could be happening -- it could actually just be running out of gas; i have recently learned that that output can mean mload is getting run on an address out of range, too, which is more likely here.

the description of the test is here:

        {
            "file": "TraceThis.obs",
            "expected": "-2438",
            "shows_that_we_support": "emitting tracers and default constructors for more complicated nestings of objects. note that this emits 30 log messages between deployment and invocation; they look right but haven't been checked carefully"
        }

note that expected ought to be 1733297

ivoysey added a commit that referenced this issue Apr 1, 2022
* first pass at including default constructors

* adding a time string to the benchmarks file so it doesn't get overwritten

* updating the test script to optionally save the optimized and pretty yul from running solc, for local debugging

* generate constructor and arguments

* changing the name of the test transaction to avoid #424

* adding a prefix to constructor argument names so that they are unique

* whitespace, scala style

* writing a helper for unified names of transactions

* updating codegen to use the single helper, adding comments

* convenience script for local debugging

* updating translateTransaction to be able to use the unified helper for transaction names

* fixing whitespace

* changing the test script so that it calls transactions from main with the contract name prefix

* removing inMain boolean argument, it was a symptom of nonuniform transaction names. this addresses #407

* removing some redunant code in translateProgram, pushing helpers into util file

* fixing a silly error, changing how line comments get printed for readability

* first pass at producing the sequence of assignments for the body of the default constructor

* whitespace, scala style

* some comments, changing the signature of the internal function so i can actually use fold

* removing an accidental implementation of seq.reverse

* updating helper script

* update the field where it actually is in memory; adding a helper function because this code alsp appears in the assignment case

* using the helper to declare vars not just assign them

* adding a test specifically for default constructors

* whitespace

* another missing reverse here

* adding comments

* first cut at calling the constructor for main per #426 #427

* comment

* adding memory allocation to the invoke block, calling default constructor and tracer there. first cut

* moving the codecopy after the tracer def and call

* adding a test case

* scraps

* first cut at tracing after pointer write

* adding a couple new helper functions; changing the update field helper to add the storage offset, which I think breaks the SetGetDefaultChecks test.

* using new helper functions. adding tracer call code in pointer writes

* reversing a bad change, adding a comment about why so i dont feel tempted to do it again

* after debugging, changing which identifier goes to the trace call

* whitespace, comments

* Removing trace this test case. see #434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant