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

feat: new getMemory cheatcode #4664

Closed
wants to merge 2 commits into from
Closed

Conversation

teddav
Copy link
Contributor

@teddav teddav commented Mar 29, 2023

getMemory cheatcode

Addition of 3 new cheatcodes:

struct FormattedMemory {
        string header;
        string[] words;
}

function getMemory(uint256,uint256) external view returns (bytes memory);

// Gets the current memory and returns it ready to be console.logged
function getMemoryFormattedAsString(uint256,uint256) external view returns (string memory);

function getMemoryFormatted(uint256,uint256) external view returns (FormattedMemory memory);

Motivation

In Solidity, the memory can only be accessed in blocks of 32 bytes. It can be more convenient to inspect the memory as a whole and log the chunks of memory we're interested in.
The debugger is a great tool but takes too much time to go through when you just need to log parts of the memory at a specific moment.
The expectSafeMemory cheat by @clabby was a great addition to keep memory safe. This getMemory cheatcode comes in addition to it, to ease debugging and visualize the memory while writing tests.

Solution

I added 3 different cheatcodes to make it as easy as possible for the user.

The formatted memory looks like this:

   0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
  41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41  0x00 (0)
  ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba ba  0x20 (32)
  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 44  0x40 (64)
  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x60 (96)
  00                                                                                               0x80 (128)

Issues

  • This cheatcode returns the memory as a string or bytes so it modifies the memory, which is obviously not ideal for a cheatcode that's supposed to help inspect the current memory.
    I think that the only way to avoid that is to just log the memory straight from the cheatcode, and avoid returning it.
  • A big issue: we do not have access to the current memory (via the Interpreter) in the apply_cheatcode() function. So I had to clone the memory in every step() which obviously seems unnecessary. How can I copy the memory only before the cheatcode is applied?

@mattsse
Copy link
Member

mattsse commented Mar 29, 2023

thank you for this

before we proceed with this I'd get @mds1 and @clabby opinion on this.

an issue is that console.log and cheatcodes are a separate step, so the easiest impl would be

  1. get memory slice via cheatcode
  2. console.log

A big issue: we do not have access to the current memory (via the Interpreter) in the apply_cheatcode() function. So I had to clone the memory in every step() which obviously seems unnecessary.

this isn't actually a big deal because memory is of type Bytes which is cheap to clone

@prestwich
Copy link
Contributor

This cheatcode returns the memory as a string or bytes so it modifies the memory, which is obviously not ideal for a cheatcode that's supposed to help inspect the current memory. I think that the only way to avoid that is to just log the memory straight from the cheatcode, and avoid returning it.

It also modifies memory when it stores the function arguments for the call to the cheatcode contract, no?

@teddav
Copy link
Contributor Author

teddav commented Apr 4, 2023

It also modifies memory when it stores the function arguments for the call to the cheatcode contract, no?

Yes you're right. Really not the best... In my head, this cheatcode was meant as a quick (and dirty) way to access the memory for now.
But you're right, you have to know what to expect (that it itself modifies the memory). I definitely want to rewrite it soon and make it cleaner. Do you think it would be good enough for now? I think it could be merged like this as long as I explain in the doc that the memory will be modified? @prestwich
My main issue was the Bytes cloning in step() but as @mattsse said, it should be good.

@Philogy wrote here https://t.me/foundry_rs/27582 we should write something on top of console.log

@prestwich
Copy link
Contributor

prestwich commented Apr 4, 2023

you could have your cheatcode implementation strip its own call and modify the 0x40 freemem pointer 🤔 basically remove its impact on the memory

is there a reason that you return the memory and write it into memory? if you just want to inspect local memory, then a pointer lib would be better for that?

if the puprose is to log it, better to make a lib on top of console.log? 🤔

@teddav
Copy link
Contributor Author

teddav commented May 4, 2023

Just updated my work on this new cheatcode. Based on @prestwich 's advice, I decided to move it to the Console instead.
I added a new console.logMemory() function which formats the memory as a string and adds to the Vector of Logs to be printed at the end.
The impact is minimal and shouldn't affect any project using logs, since it's just a String.

Unfortunately, the memory is still modified, due to an external call being made. I couldn't figure out how to remove the modified memory from the call simply. It involved too many changes, and I dont think it's worth it for this feature.

cc @clabby

@mattsse
Copy link
Member

mattsse commented May 4, 2023

what to do with this @mds1 ?

imo we can live with this

Unfortunately, the memory is still modified, due to an external call being made.

@teddav
Copy link
Contributor Author

teddav commented May 4, 2023

Just to add some details for the reviewers:

Regarding the change to console.json, it's hard to see the diff. This is what I added

{
  "inputs": [
    { "internalType": "uint256", "name": "start", "type": "uint256" },
    { "internalType": "uint256", "name": "end", "type": "uint256" },
    { "internalType": "bool", "name": "prettyPrint", "type": "bool" }
  ],
  "outputs": [],
  "stateMutability": "view",
  "type": "function",
  "name": "logMemory"
}

This is what the logs will look like:
For this test

function testLog() public {
        console.log("AA");
        console.logMemory(50, 71, true);
        console2.logMemory(4, 7, true);
        console2.logMemory(10, 100, false);
    }
Logs:
  AA
  LogMemory:
             0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
[0x20:0x40] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[0x40:0x60] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 c8

  LogMemory:
             0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
[0x00:0x20] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  LogMemory:
[0x00:0x20] 0000000000000000000000000000000000000000000000000000000000000000
[0x20:0x40] 0000000000000000000000000000000000000000000000000000000000000000
[0x40:0x60] 00000000000000000000000000000000000000000000000000000000000002d0
[0x60:0x80] 0000000000000000000000000000000000000000000000000000000000000000

Same for Scripts

function run() public {
        console.log("ok");
        console.logMemory(1, 2, true);
    }
== Logs ==
  ok
  LogMemory:
             0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
[0x00:0x20] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

@mds1
Copy link
Collaborator

mds1 commented May 8, 2023

So one thought is that we might soon have 3 ways to get memory:

  1. The getMemory cheats introduced in this PR, which IIUC from above still affects memory simply by using it
  2. Use the debugger (you can now set breakpoints with vm.breakpoint so it's easier/faster to view memory this way.
  3. The safeconsole lib PR'd by @Philogy in feat: add memory safe logging forge-std#375. I haven't taken a look at this yet, but the purpose of this is to avoid the downside of affecting memory when logging it

I haven't compared (1) and (3) but it feels redundant to have both? But I can see how it's useful to have at least one of them as an alternative to the deubgger. TBH I don't often have the need to inspect memory so I'm probably not the best person to decide here and would defer to some optimzoooors like @teddav @Philogy @clabby @brockelmore to figure out the best path forward

@teddav
Copy link
Contributor Author

teddav commented May 9, 2023

@mds1 I agree it might be redundant with the debugger, but not with safeconsole.
vm.breakpoint is really useful and I love to use the debugger, but sometimes i feel lazy and I want to inspect quickly my memory as I write my tests. That was the idea behind this cheatcode: avoid opening the debugger.

Regarding safeconsole, I think what @Philogy did is great and could definitely be used to improve this PR, but they are 2 different things. safeconsole is a console lib that restores the memory after the log, here we're just logging the memory.
So if we decide to implement safeconsole, we could add it to this PR.
One negative point regarding safeconsole: I do think that the code is a bit messy, but it makes sense because of what it is doing (I dont know how we could write the lib differently). If we want to have the same result in a cleaner way, we should probably erase the memory directly in Foundry.

I think the cheatcode could be helpful but if others think this cheatcode is useless I'm ok to close the PR. I won't fight for it 😁

@Philogy
Copy link

Philogy commented May 10, 2023

So one thought is that we might soon have 3 ways to get memory:

  1. The getMemory cheats introduced in this PR, which IIUC from above still affects memory simply by using it
  2. Use the debugger (you can now set breakpoints with vm.breakpoint so it's easier/faster to view memory this way.
  3. The safeconsole lib PR'd by @Philogy in feat: add memory safe logging forge-std#375. I haven't taken a look at this yet, but the purpose of this is to avoid the downside of affecting memory when logging it

I haven't compared (1) and (3) but it feels redundant to have both? But I can see how it's useful to have at least one of them as an alternative to the deubgger. TBH I don't often have the need to inspect memory so I'm probably not the best person to decide here and would defer to some optimzoooors like @teddav @Philogy @clabby @brockelmore to figure out the best path forward

A bit biased here since I opened the PR to add the memory-preserving logging library, but if the goal is to be able to log slices of memory I think it's better to avoid adding complexity to the core of foundry if the same effect can be achieved with external and/or the standard library.

Another problem with the cheatcode-based approach is that Solidity treats them like normal calls meaning it'll in and of itself change memory for the call to the magic cheatcode address. It will only change "free" memory but this may be relevant in certain more optimized contracts (e.g. Seaport relies on the integrity of free memory for its transfer accumulator meaning you can't use cheatcodes or the normal logging libs without breaking actual logic in certain code sections).

PoC:

    function testCheatcode() public {
        address user = makeAddr("user");

        uint256 fp1;
        uint256 d1;
        assembly {
            fp1 := mload(0x40)
            d1 := mload(fp1)
        }

        vm.prank(user);

        uint256 fp2;
        uint256 d2;
        assembly {
            fp2 := mload(0x40)
            d2 := mload(fp2)
        }

        // Shows that calling a cheat code alone affects the free memory
        assertTrue(d1 != d2);
        // Free memory pointer stays intact tho.
        assertEq(fp1, fp2);
    }

It'd be pretty straightforward to write a memory slice logging library function assuming you had a formatting flag that'd allow you to get a string display in hex. Alternatively you could leverage the logBytes(bytes) signature of the magic console address but then it'd be harder to have the nice formatting. Would just have to save and restore a few words before the memory section or do "memcopy" with the help of the identity precompile, saving and restoring the last bytes if you're trying to log a section that's too close to the start of memory.

@Philogy
Copy link

Philogy commented May 10, 2023

Created a memory preserving logMemory helper function leveraging console's logBytes(bytes) function: foundry-rs/forge-std@4e5903a (original: https://github.com/Philogy/forge-safe-log/blob/a35748139df09952acc5d87a9b051d37f03ed3cc/src/safelog.sol#L26-L88)

@clabby
Copy link
Contributor

clabby commented May 15, 2023

I tend to agree with @Philogy here - given that this can be done in Solidity with relative ease + without modifying existing memory, it makes more sense to add this to forge-std. The cheatcode modifying memory could make testing certain libraries that utilize creative memory management more difficult as well (or rather, you just couldn't use it where it may be the most important).

Alternatively, if the cheatcode can take a snapshot of the memory before the call and restore it prior to logging, that would work as well - it does feel a bit messier than the logMemory helper, though.

@mds1
Copy link
Collaborator

mds1 commented May 15, 2023

Thanks for the thoughts @clabby @Philogy! So I'm thinking we:

  • Review/merge the safeconsole PR
  • Close this PR
  • Pretty logging of memory could potentially be implemented with a console.table functionality: Feature request: console.table forge-std#91
  • Eventually as part of feat: move cheatcodes from forge-std to forge #3782 we may want to try upstreaming cheats to forge from solidity. So at that time if we can find a way to implement the cheatcode without affecting memory then we can replace safeconsole with a native cheat, otherwise we can leave it

@teddav lmk if you have any objections here, appreciate the PR though!

@teddav
Copy link
Contributor Author

teddav commented May 15, 2023

All good. I agree with the comments above.
We can always re-open it later, if we feel it could be useful.
As you said, it would be nice to have some "pretty logging" eventually.

@mds1
Copy link
Collaborator

mds1 commented May 17, 2023

Per above convo, I'm going to close this PR and later today review/merge the safeconsole one in forge-std. Sorry for the wasted work here @teddav, thanks for your understanding!

@mds1 mds1 closed this May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants