Adding MCOPY Instruction #2711
Adding MCOPY Instruction #2711
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rskj-core/src/test/resources/dsl/opcode/mcopy/mCopyTestCase1.txt
Outdated
Show resolved
Hide resolved
nagarev
force-pushed
the
add-mcopy
branch
2 times, most recently
from
e640b71 to
a47d5c9
Compare
There was a problem hiding this comment.
Well done @nagarev. These tests are pretty complete and shows a hard work in fulfil the scenarios of test from the EIP.
I just have a few comments to improve it even more the readability. But they are not blocking and doesn't prevent the approval.
You need to fix some tests though, it seems that it's broke for java-17 and java-21.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
| @@ -767,6 +767,16 @@ private long computeDataCopyGas() { | |||
| return calcMemGas(oldMemSize, newMemSize, copySize); | |||
| } | |||
|
|
|||
| private long computeMemoryCopyGas() { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@donequis , do you mean the constant MCopy cost (value of g_verylow from the formula) ?
There was a problem hiding this comment.
No. There's a price pay for word copied. 3 * words_copied
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, you're right. My bad.
There was a problem hiding this comment.
no worries. It's good to validate assumptions and align on that
|
| 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) { |
There was a problem hiding this comment.
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);
}
| @MethodSource("provideParametersForMCOPYTestCases") | ||
| void testMCOPY_OnEachTestCase_ExecutesAsExpected(String dslFile) throws FileNotFoundException, DslProcessorException { | ||
|
|
||
| DslParser parser = DslParser.fromResource("dsl/opcode/mcopy/" + dslFile); |
There was a problem hiding this comment.
praise:
Congrats to have simplified the tests with the input arguments. well done!
|
|
||
| @ParameterizedTest | ||
| @MethodSource("provideParametersForMCOPYTestCases") | ||
| void testMCOPY_OnEachTestCase_ExecutesAsExpected(String dslFile) throws FileNotFoundException, DslProcessorException { |
There was a problem hiding this comment.
praise:
I liked of the simplification with the name of each scenario file. Saved a lot of lines of testing.
Description
Besides memory copying being a basic operation, implementing it on the VM comes with overhead, as described in the EIP-5656
Currently memory copying can be achieved by the usage of
CALL,MLOADandMSTORE(among others opcodes) depending on the scenarios where the memory copy is being done.The
MCOPYinstruction will be introduced at0x5E.Motivation and Context
Increase compatibility
How Has This Been Tested?
Types of changes
Checklist:
N/A