Include space for small function parameters/local variables in scratch space. - [merged]

[kwannoel]

In GitLab by @isd on May 9, 2021, 05:04

Merges scratch-space -> master

This partially addresses #106, though this still doesn't reserve space for global variables. I'm not totally sure what that should look like, since we don't have any right now. I'm curious to hear from @AlexKnauth how storing the deposit amounts for different assets is going to work, since that's going to be our first usage of it.

For now, this is a fairly trivial change, as we were already computing this as params-end and just needed to actually stick it in the binary. It includes some normative comments -- I think the control flow here is not great.

I don't have automated tests currently, though I did some fiddling with various glow programs to make sure adding & removing variables has the right effect on the size of the scratch space, and it does. Automating this might take some non-trivial refactoring -- I'll look into it a bit more tomrrow or Monday.

[kwannoel]

In GitLab by @isd on May 9, 2021, 05:10

added 6 commits

• f82cdc6b...88bf555b - 5 commits from branch master
• 2945c726 - Incorporate small functions into brk-start.

Compare with previous version

[kwannoel]

In GitLab by @isd on May 11, 2021, 04:49

added 2 commits

• 37189185 - Add tests for scratch-space.
• 6f03f19b - Comments in consensus-code-generator.

Compare with previous version

[kwannoel]

In GitLab by @isd on May 11, 2021, 04:51

added 4 commits

• c821f63 - 1 commit from branch master
• 09e703d - Incorporate small functions into brk-start.
• f37f9db - Add tests for scratch-space.
• 822e71e - Comments in consensus-code-generator.

Compare with previous version

[kwannoel]

In GitLab by @isd on May 11, 2021, 04:54

Ok, I added some tests. Per discussion on discord, the assets2 branch just uses define-consecutive-address to allocate the (fixed number of) globals for assets. I'm of the opinion that handling globals in the scratch space calculation should be deferred until that's merged, since otherwise nothing would use it. Then as a step towards removing the fixed bound on assets, we could update the compiler to allocate these in the scratch space.

I think we can merge this as is, though there may be follow-on tasks.

[kwannoel]

In GitLab by @isd on May 11, 2021, 10:16

The source of the failure appears to be that the new test needs a running test net, but we don't currently set one up for the unit tests.

In principle, it seems like the new test shouldn't need one, since it's not actually deploying or running the contract, but without the (ensure-ethereum-connection "pet"), we get an error "No configured ethereum node connection.", so I assume somewhere we're querying ethereum for configuration.

So options:

1. Mark this as an integration test, so it only runs in the section where we do have a running network.
2. Start the test net sooner in CI, so that it's available.

My inclination is (1). Thoughts?

[kwannoel]

In GitLab by @isd on May 12, 2021, 07:31

added 1 commit

• c857730 - Make the consensus-code-generator test an integration test.

Compare with previous version

[kwannoel]

In GitLab by @isd on May 12, 2021, 07:32

Ok, went ahead with (1) -- hopefully that will fix the CI issue.

[kwannoel]

In GitLab by @isd on May 19, 2021, 05:06

mentioned in commit a85c539