Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve performance of mock getOriginAddress, getCallerAddress #2286

Closed
wants to merge 1 commit into from
Closed

Improve performance of mock getOriginAddress, getCallerAddress #2286

wants to merge 1 commit into from

Conversation

guidovranken
Copy link
Contributor

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

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.

@jurajpiar jurajpiar closed this Apr 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants