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

Very high values when eth.estimateGas requested #642

Closed
gurdeepakjoshi opened this issue Sep 18, 2018 · 11 comments
Closed

Very high values when eth.estimateGas requested #642

gurdeepakjoshi opened this issue Sep 18, 2018 · 11 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@gurdeepakjoshi
Copy link

gurdeepakjoshi commented Sep 18, 2018

Which Aion version?: 0.3.1.46f09bac
Which operating system?: Linux
Which distributor and version?: Ubuntu 16.04 LTS 64 bit
How installed?: binaries
Are you fully synchronized?: yes
Did you try to restart the node?: yes

Actual Behaviour

  • Launch aion node
  • cd web3/
  • node console.js
  • aion-127.0.0.1:8545> eth.estimateGas({data: "0x000"})
    13914991129094744000
    It returns a very high estimate which makes the contract deployment infeasible.

Expected Behaviour
estimateGas should return realistic estimate such as 2000000

FYI
If you run same node locally (private) without syncing with external nodes then this issue doesn't occur. This issue is only occuring on fully synced node.

@qoire
Copy link
Contributor

qoire commented Sep 18, 2018

@AionJayT might be important for us to check that we can reproduce this.

From my side:

I was unable to reproduce this issue with a locally synced node (as suggested by Gurdeep). I also was informed by another colleague that it seemed to have to do with his keystore being placed in the keystore folder vs not. I tried this as well and was not able to reproduce the error.

@qoire qoire added the bug Something isn't working label Sep 18, 2018
@qoire
Copy link
Contributor

qoire commented Sep 18, 2018

@gurdeepakjoshi Just to be clear, when you say fully synced, are you referring to mainnet or mastery?

@gurdeepakjoshi
Copy link
Author

gurdeepakjoshi commented Sep 18, 2018

@gurdeepakjoshi Just to be clear, when you say fully synced, are you referring to mainnet or mastery?

@qoire , I meant mastery.

@erik-iglikov
Copy link
Contributor

I was able to reproduce this issue with importing the keystore and config when I apply to a brand new mainnet kernel(aion-v0.3.1.dcaf9e8-2018-08-30)

@arajasek
Copy link
Contributor

I was able to reproduce this on mastery using both v0.3.0.17d0f332, as well as the latest dev. I did not need to sync to completion in order to reproduce the bug.

I stepped through the code, and it looks like the wonky behaviour is happening inside the VM. @AionJayT is following up with the VM team.

@iamyulong
Copy link
Contributor

iamyulong commented Sep 20, 2018

Was debugging this issue, found the root cause is that the from address of the transaction being empty. The Address#toBytes() returns empty byte array instead of 32 bytes of zeros. After the serialization of the the ExecutionContext, the encoded bytes are interpreted with incorrect offsets.

Code:

  1. Where the transaction parameters, with default values, are constructed

    public static ArgTxCall fromJSON(final JSONObject _jsonObj, NrgOracle oracle, long defaultNrgLimit){
    try {
    Address from = Address.wrap(ByteUtil.hexStringToBytes(_jsonObj.optString("from", "")));
    Address to = Address.wrap(ByteUtil.hexStringToBytes(_jsonObj.optString("to", "")));
    byte[] data = ByteUtil.hexStringToBytes(_jsonObj.optString("data", ""));

  2. The AionTransaction constructor is taking from, rather than decoding from signature

    public AionTransaction(byte[] nonce, Address from, Address to, byte[] value, byte[] data, long nrg, long nrgPrice) {
    super(nonce, to, value, data, nrg, nrgPrice);
    this.from = from;
    parsed = true;
    }

  3. Address is designed to be of variable length, zero or 32

    public Address(final String in) {
    if (in == null) {
    throw new IllegalArgumentException();
    }
    byte[] hexByte = ByteUtil.hexStringToBytes(in);
    if (hexByte.length != ADDRESS_LEN && hexByte.length != 0) {
    throw new IllegalArgumentException();
    }
    setupData(hexByte);
    }

  4. Where addresses are serialized, fixed 32 bytes are being expected.

    public byte[] toBytes() {
    ByteBuffer buffer = ByteBuffer.allocate(getEncodingLength());
    buffer.order(ByteOrder.BIG_ENDIAN);
    buffer.put(address.toBytes());
    buffer.put(origin.toBytes());
    buffer.put(sender.toBytes());
    buffer.put(nrgPrice.getData());

@iamyulong
Copy link
Contributor

Quick fix would be updating the AionTransaciton constructor to not set the from variable.

Sine this issue won't affect consensus, put it at low-to-median priority.

@AionJayT
Copy link
Collaborator

some of the contract calls do need to verify the sender, therefore, I think the better solution is putting an address when the call doesn't have the sender address or just return an error in the API layer.

@AionJayT AionJayT added this to the 0.3.2 milestone Sep 20, 2018
@iamyulong
Copy link
Contributor

iamyulong commented Sep 20, 2018

I thought the transaction would be signed by a random address. If we don't set the from variable, the getFrom() method will decode the address from the signature. Correct me if I'm wrong.

@AionJayT
Copy link
Collaborator

that's correct, the problem is if the contract function call requires the sender's address (let assume modifier), but this API doesn't require the sender's signature for the constant call. Then the getfrom() won't work.

@arajasek
Copy link
Contributor

PR #646 adds the Zero Address if no address is provided, resolves the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants