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

Adding MCOPY Instruction #2711

Merged
merged 27 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ public enum ConsensusRule {
RSKIP428("rskip428"),
RSKIP434("rskip434"),
RSKIP438("rskip438"),
RSKIP445("rskip445"), // From EIP-5656 MCOPY instruction
RSKIP454("rskip454"),
;

Expand Down
5 changes: 4 additions & 1 deletion rskj-core/src/main/java/org/ethereum/vm/OpCode.java
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,10 @@ public enum OpCode {
* (0x5b)
*/
JUMPDEST(0x5b, 0, 0, SPECIAL_TIER),

/**
* (0x5e) Memory copying instruction
*/
MCOPY(0x5e, 3, 0, VERY_LOW_TIER),

/* Push Operations */
/**
Expand Down
4 changes: 4 additions & 0 deletions rskj-core/src/main/java/org/ethereum/vm/OpCodes.java
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,10 @@ private OpCodes() {
* (0x5b)
*/
static final byte OP_JUMPDEST =0x5b ;
/**
* (0x5e)
*/
public static final byte OP_MCOPY = 0x5e;

/* Push Operations */
/**
Expand Down
44 changes: 44 additions & 0 deletions rskj-core/src/main/java/org/ethereum/vm/VM.java
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,25 @@ private long computeDataCopyGas() {
return calcMemGas(oldMemSize, newMemSize, copySize);
}

private long computeMemoryCopyGas() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still only charging for memory expansion, but ignoring the copy cost. Please update following the specs

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-5656.md#gas-costs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donequis , do you mean the constant MCopy cost (value of g_verylow from the formula) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There's a price pay for word copied. 3 * words_copied

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. if so, it's defined in here, and used in here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the formula has 3 components. First is memory expansion which is considered here. Second is the basic cost. Third part is the length copied. This las part is not considered in the cost.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't that part in here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right. My bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we are good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries. It's good to validate assumptions and align on that

DataWord length = stack.get(stack.size() - 3);
DataWord src = stack.get(stack.size() - 2);
DataWord dst = stack.peek();

long copySize = Program.limitToMaxLong(length);
checkSizeArgument(copySize);

DataWord offset = dst;
if (src.value().compareTo(dst.value()) > 0) {
offset = src;
}

long newMemSize = memNeeded(offset, copySize);

// Note: 3 additional units are added outside because of the "Very Low Tier" configuration
return calcMemGas(oldMemSize, newMemSize, copySize);
}

protected void doCODESIZE() {
if (computeGas) {
if (op == OpCode.EXTCODESIZE) {
Expand Down Expand Up @@ -1436,6 +1455,25 @@ protected void doJUMPDEST()
program.step();
}

protected void doMCOPY() {
if (computeGas) {
gasCost = GasCost.add(gasCost, computeMemoryCopyGas());
spendOpCodeGas();
}

// EXECUTION PHASE
DataWord dst = program.stackPop();
DataWord src = program.stackPop();
DataWord length = program.stackPop();

if (isLogEnabled) {
hint = "dst: " + dst + " src: " + src + " length: " + length;
nagarev marked this conversation as resolved.
Show resolved Hide resolved
}

program.memoryCopy(dst.intValueSafe(), src.intValueSafe(), length.intValueSafe());
program.step();
}

protected void doCREATE(){
if (program.isStaticCall() && program.getActivations().isActive(RSKIP91)) {
throw Program.ExceptionHelper.modificationException(program);
Expand Down Expand Up @@ -1992,6 +2030,12 @@ protected void executeOpcode() {
break;
case OpCodes.OP_JUMPDEST: doJUMPDEST();
break;
case OpCodes.OP_MCOPY:
if (!activations.isActive(RSKIP445)) {
throw Program.ExceptionHelper.invalidOpCode(program);
}
doMCOPY();
break;
case OpCodes.OP_CREATE: doCREATE();
break;
case OpCodes.OP_CREATE2:
Expand Down
4 changes: 4 additions & 0 deletions rskj-core/src/main/java/org/ethereum/vm/program/Program.java
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,10 @@ public byte[] memoryChunk(int offset, int size) {
return memory.read(offset, size);
}

public void memoryCopy(int dst, int src, int length) {
memorySave(dst, memoryChunk(src, length));
}

/**
* Allocates extra memory in the program for
* a specified size, calculated from a given offset
Expand Down
3 changes: 2 additions & 1 deletion rskj-core/src/main/resources/expected.conf
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,9 @@ blockchain = {
rskip428 = <hardforkName>
rskip434 = <hardforkName>
rskip438 = <hardforkName>
rskip445 = <hardforkName>
rskip454 = <hardforkName>
}
}
}
gc = {
enabled = <enabled>
Expand Down
1 change: 1 addition & 0 deletions rskj-core/src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ blockchain = {
rskip428 = lovell700
rskip434 = arrowhead631
rskip438 = lovell700
rskip445 = lovell700
rskip454 = lovell700
}
}
Expand Down
46 changes: 46 additions & 0 deletions rskj-core/src/test/java/co/rsk/vm/VMExecutionTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,10 @@ private Program executeCodeWithActivationConfig(String code, int nsteps, Activat
return executeCodeWithActivationConfig(compiler.compile(code), nsteps, activations);
}

private Program executeCodeWithActivationConfig(String code, DataWord[] stack, int nsteps, ActivationConfig.ForBlock activations) {
return executeCodeWithActivationConfig(compiler.compile(code), stack, nsteps, activations);
}

private Program executeCodeWithActivationConfig(byte[] code, int nsteps, ActivationConfig.ForBlock activations) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

This method could be simplified as well to something like this:

    private Program executeCodeWithActivationConfig(byte[] code, int nsteps, ActivationConfig.ForBlock activations) {
        return executeCodeWithActivationConfig(code, new DataWord[0], nsteps, activations);
    }

VM vm = new VM(vmConfig, precompiledContracts);
Program program = new Program(vmConfig, precompiledContracts, blockFactory, activations, code, invoke,null, new HashSet<>(), new BlockTxSignatureCache(new ReceivedTxSignatureCache()));
Expand All @@ -881,6 +885,21 @@ private Program executeCodeWithActivationConfig(byte[] code, int nsteps, Activat
return program;
}

private Program executeCodeWithActivationConfig(byte[] code, DataWord[] stack, int nsteps, ActivationConfig.ForBlock activations) {
VM vm = new VM(vmConfig, precompiledContracts);
Program program = new Program(vmConfig, precompiledContracts, blockFactory, activations, code, invoke,null, new HashSet<>(), new BlockTxSignatureCache(new ReceivedTxSignatureCache()));

for (DataWord element : stack) {
program.stackPush(element);
}

for (int k = 0; k < nsteps; k++) {
vm.step(program);
}

return program;
}

private Program playCodeWithActivationConfig(String code, ActivationConfig.ForBlock activations) {
return playCodeWithActivationConfig(compiler.compile(code), activations);
}
Expand Down Expand Up @@ -947,4 +966,31 @@ private void executeBASEFEE(ActivationConfig.ForBlock activations) {
// See ProgramInvokeMockImpl.getMinimumGasPrice()
Assertions.assertEquals(DataWord.valueFromHex("000000000000000000000000000000000000000000000000000003104e60a000"), stack.peek());
}

@Test
void testMCOPY_WhenActive_ExecutesAsExpected() {
ActivationConfig.ForBlock activations = mock(ActivationConfig.ForBlock.class);
when(activations.isActive(RSKIP445)).thenReturn(true);

executeMCOPY(activations);
}

@Test
void testMCOPY_WhenInactive_ExecutesAsExpected() {
ActivationConfig.ForBlock activations = mock(ActivationConfig.ForBlock.class);
when(activations.isActive(RSKIP445)).thenReturn(false);

Assertions.assertThrows(Program.IllegalOperationException.class, () -> {
executeMCOPY(activations);
});
}

private void executeMCOPY(ActivationConfig.ForBlock activations) {
DataWord[] stackValues = {DataWord.ZERO, DataWord.ZERO, DataWord.ZERO};
Program program = executeCodeWithActivationConfig("MCOPY", stackValues, 1, activations);
Stack stack = program.getStack();

Assertions.assertEquals(0, stack.size());
}

}
184 changes: 184 additions & 0 deletions rskj-core/src/test/java/co/rsk/vm/opcode/MCopyDslTest.java
nagarev marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
package co.rsk.vm.opcode;

import co.rsk.config.TestSystemProperties;
import co.rsk.test.World;
import co.rsk.test.dsl.DslParser;
import co.rsk.test.dsl.DslProcessorException;
import co.rsk.test.dsl.WorldDslProcessor;
import com.typesafe.config.ConfigValueFactory;
import org.ethereum.core.Block;
import org.ethereum.core.Transaction;
import org.ethereum.core.TransactionReceipt;
import org.ethereum.core.util.TransactionReceiptUtil;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.io.FileNotFoundException;
import java.util.stream.Stream;

class MCopyDslTest {

@Test
void testMCOPY_whenNotActivated_behavesAsExpected() throws FileNotFoundException, DslProcessorException {

// Test Config Setup

TestSystemProperties configWithRskip445Disabled = new TestSystemProperties(rawConfig ->
rawConfig.withValue("blockchain.config.hardforkActivationHeights.lovell700", ConfigValueFactory.fromAnyRef(-1))
);

// Test Setup

DslParser parser = DslParser.fromResource("dsl/opcode/mcopy/testRSKIPNotActivatedTest.txt");
World world = new World(configWithRskip445Disabled);
WorldDslProcessor processor = new WorldDslProcessor(world);
processor.processCommands(parser);

// Assertions

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b01", 1);
assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopy", true);

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b02", 1);
assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopyNotActivated", false);

}

// Advanced Overwrite Test Cases
// https://github.com/ethereum/execution-spec-tests/blob/c0065176a79f89d93f4c326186fc257ec5b8d5f1/tests/cancun/eip5656_mcopy/test_mcopy.py)

@Test
void testMCOPY_overwriteCases_behaveAsExpected() throws FileNotFoundException, DslProcessorException {
nagarev marked this conversation as resolved.
Show resolved Hide resolved

DslParser parser = DslParser.fromResource("dsl/opcode/mcopy/testOverwriteCases.txt");
World world = new World();
WorldDslProcessor processor = new WorldDslProcessor(world);
processor.processCommands(parser);

// Assertions

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b01", 1);
assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopy", true);

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b02", 1);
TransactionReceipt txTestMCopyOKCallReceipt = assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopyOKCall", true);

// Event Assertions

Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "ZERO_INPUTS_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "SINGLE_BYTE_REWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "FULL_WORD_REWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "SINGLE_BYTE_FWD_OVERWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "FULL_WORD_FWD_OVERWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "MID_WORD_SINGLE_BYTE_REWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "MID_WORD_SINGLE_WORD_REWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "MID_WORD_MULTY_WORD_REWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "TWO_WORDS_FWD_OVERWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "TWO_WORDS_BWD_OVERWRITE_OK", null));
Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "TWO_WORDS_BWD_OVERWRITE_SINGLE_BYTE_OFFSET_OK", null));

Assertions.assertEquals(0, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "ERROR", null));

}

@ParameterizedTest
@MethodSource("provideParametersForMCOPYTestCases")
void testMCOPY_OnEachTestCase_ExecutesAsExpected(String dslFile) throws FileNotFoundException, DslProcessorException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise:

I liked of the simplification with the name of each scenario file. Saved a lot of lines of testing.


DslParser parser = DslParser.fromResource("dsl/opcode/mcopy/" + dslFile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise:

Congrats to have simplified the tests with the input arguments. well done!

World world = new World();
WorldDslProcessor processor = new WorldDslProcessor(world);
processor.processCommands(parser);

// Assertions

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b01", 1);
assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopy", true);

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b02", 1);
TransactionReceipt txTestMCopyOKCallReceipt = assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopyOKCall", true);

// Event Assertions

Assertions.assertEquals(1, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "OK", null));
Assertions.assertEquals(0, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "ERROR", null));

}

@Test
void testMCOPY_zeroLengthOutOfBoundsDestination_runsOutOfGasAsExpected() throws FileNotFoundException, DslProcessorException {

DslParser parser = DslParser.fromResource("dsl/opcode/mcopy/testZeroLengthOutOfBoundsDestination.txt");
World world = new World();
WorldDslProcessor processor = new WorldDslProcessor(world);
processor.processCommands(parser);

// Assertions

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b01", 1);
assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopy", true);

assertBlockExistsAndContainsExpectedNumberOfTxs(world, "b02", 1);
TransactionReceipt txTestMCopyOKCallReceipt = assertTxExistsWithExpectedReceiptStatus(world, "txTestMCopyOKCall", false);

// Event Assertions

Assertions.assertEquals(0, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "OK", null));
Assertions.assertEquals(0, TransactionReceiptUtil.getEventCount(txTestMCopyOKCallReceipt, "ERROR", null));

}

private static Stream<Arguments> provideParametersForMCOPYTestCases() {
return Stream.of(

// Basic Test Cases from 1 to 4 on EIP
Arguments.of("testCopying32BytesFromOffset32toOffset0.txt"),
Arguments.of("testCopying32BytesFromOffset0toOffset0.txt"),
Arguments.of("testCopying8BytesFromOffset1toOffset0.txt"),
Arguments.of("testCopying8BytesFromOffset0toOffset1.txt"),

// Full Memory Copy/Rewrite/Clean Tests
Arguments.of("testFullMemoryClean.txt"),
Arguments.of("testFullMemoryCopy.txt"),
Arguments.of("testFullMemoryCopyOffset.txt"),
Arguments.of("testFullMemoryRewrite.txt"),

// Memory Extension Tests
Arguments.of("testOutOfBoundsMemoryExtension.txt"),
Arguments.of("testSingleByteMemoryExtension.txt"),
Arguments.of("testSingleWordMemoryExtension.txt"),
Arguments.of("testSingleWordMinusOneByteMemoryExtension.txt"),
Arguments.of("testSingleWordPlusOneByteMemoryExtension.txt")

);
}

private static void assertBlockExistsAndContainsExpectedNumberOfTxs(World world, String blockName, int expectedNumberOfTxs) {
Block block = world.getBlockByName(blockName);
Assertions.assertNotNull(block);
Assertions.assertEquals(expectedNumberOfTxs, block.getTransactionsList().size());
}

private static TransactionReceipt assertTxExistsWithExpectedReceiptStatus(World world, String txName, boolean mustBeSuccessful) {
Transaction tx = world.getTransactionByName(txName);
Assertions.assertNotNull(tx);

TransactionReceipt txReceipt = world.getTransactionReceiptByName(txName);
Assertions.assertNotNull(txReceipt);
byte[] creationStatus = txReceipt.getStatus();
Assertions.assertNotNull(creationStatus);

if (mustBeSuccessful) {
Assertions.assertEquals(1, creationStatus.length);
Assertions.assertEquals(1, creationStatus[0]);
} else {
Assertions.assertEquals(0, creationStatus.length);
}

return txReceipt;
}

}
Loading
Loading