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

Improve performance of mock getOriginAddress, getCallerAddress #2287

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

jurajpiar
Copy link
Member

Description

Motivation and Context

The overloaded methods getOriginAddress and getCallerAddress in ProgramInvokeMockImpl perform hashing and secp256k1 base point multiplication each time ORIGIN or CALLER are executed.

/* ORIGIN op */
@Override
public DataWord getOriginAddress() {
byte[] cowPrivKey = HashUtil.keccak256("horse".getBytes(StandardCharsets.UTF_8));
byte[] addr = ECKey.fromPrivate(cowPrivKey).getAddress();
return DataWord.valueOf(addr);
}
/* CALLER op */
@Override
public DataWord getCallerAddress() {
byte[] cowPrivKey = HashUtil.keccak256("monkey".getBytes(StandardCharsets.UTF_8));
byte[] addr = ECKey.fromPrivate(cowPrivKey).getAddress();
return DataWord.valueOf(addr);
}

This makes these opcodes unnecessary slow and it impedes my effort to detect genuine performance issues with the EVM itself.

How Has This Been Tested?

With this test harness (rskj-core/src/test/java/co/rsk/vm/Poc.java):

package co.rsk.vm;

import org.ethereum.config.blockchain.upgrades.ActivationConfig;
import co.rsk.config.TestSystemProperties;
import co.rsk.config.VmConfig;
import org.ethereum.vm.*;
import org.ethereum.core.BlockFactory;
import org.ethereum.core.BlockTxSignatureCache;
import org.ethereum.core.ReceivedTxSignatureCache;
import org.ethereum.vm.program.invoke.ProgramInvokeMockImpl;
import java.util.HashSet;
import org.ethereum.vm.program.Program;
import org.ethereum.config.blockchain.upgrades.ActivationConfigsForTest;
import javax.xml.bind.DatatypeConverter;
import org.junit.jupiter.api.Test;

public class Poc {
    @Test
    void testPoc() {
        TestSystemProperties config = new TestSystemProperties();
        PrecompiledContracts precompiledContracts = new PrecompiledContracts(config, null, new BlockTxSignatureCache(new ReceivedTxSignatureCache()));
        BlockFactory blockFactory = new BlockFactory(config.getActivationConfig());
        VmConfig vmConfig = config.getVmConfig();
        ProgramInvokeMockImpl invoke = new ProgramInvokeMockImpl();
        ActivationConfig.ForBlock activations = ActivationConfigsForTest.arrowhead600().forBlock(0);

        byte[] code = DatatypeConverter.parseHexBinary("5b3333331a1a56");

        invoke.setGas(6800000); /* block limit */
        VM vm = new VM(vmConfig, precompiledContracts);
        Program program = new Program(vmConfig, precompiledContracts, blockFactory, activations, code, invoke,null, new HashSet<>(), new BlockTxSignatureCache(new ReceivedTxSignatureCache()));

        try {
            while (!program.isStopped())
                vm.step(program);
        } catch (RuntimeException e) {
            program.setRuntimeFailure(e);
        }
    }
}

Runtime before this fix: 10 minutes, 17 seconds
Runtime after this fix: 6 seconds

Using:

time ./gradlew test  --tests co.rsk.vm.Poc.testPoc

On Linux x64, Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)

@jurajpiar
Copy link
Member Author

Thank you very much @guidovranken for your contribution. We have some security-related limitations of our processes that mean we cannot merge outside PRs. We are working on resolving these, but meanwhile we cherry-picked your commit and created a new PR, closing yours.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jurajpiar
Copy link
Member Author

pipeline:run

@Vovchyk Vovchyk merged commit 9405ac5 into master Apr 12, 2024
10 checks passed
@Vovchyk Vovchyk deleted the programinvokemockimpl-performance-fix branch April 12, 2024 14:09
@jurajpiar jurajpiar restored the programinvokemockimpl-performance-fix branch May 27, 2024 09:27
@jurajpiar jurajpiar deleted the programinvokemockimpl-performance-fix branch May 27, 2024 09:48
@aeidelman aeidelman added this to the Arrowhead 6.2.0 milestone May 27, 2024
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.

5 participants