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.

Sign up for GitHub

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

Jump to bottom

String reverts not working #173

Closed
xcantera opened this issue Aug 17, 2022 · 48 comments
Closed

String reverts not working #173

xcantera opened this issue Aug 17, 2022 · 48 comments

Comments

@xcantera
Copy link

We found a critical error on the network that makes a lot of code useless and can be a huge security problem.

The main network not working with "String reverts", for example when you have a revert on the smart contract. The network doesn't indicate what revert happened. (This is mandatory to fix it or most of the applications will not work if a revert happens)

This is a code example.

`function stake(uint256 tokenId, uint256 amountToStake) public nonReentrant {

require(_exists(tokenId), "Invalid tokenId");       
require(amountToStake <= _levels[_nfts[tokenId].level].dailymax, "Daily limit reached");
require(_nfts[tokenId].laststake == 0 || _nfts[tokenId].laststake + 1 days < block.timestamp, "Daily limit reached");
require(prnt.allowance(msg.sender, address(this)) >= amountToStake,"Insuficient Allowance");
require(prnt.transferFrom(msg.sender,address(this),amountToStake),"transfer Failed");

if (_nfts[tokenId].prnt==0){
    stakeValues.multiplierTotal +=  _nfts[tokenId].nftmultiplier;
}else{
    stakeValues.totalweight -= _nfts[tokenId].weight;
}

stakeValues.totalPRNT += amountToStake;
_nfts[tokenId].laststake = block.timestamp;
_nfts[tokenId].stakeinlevel+=amountToStake;
_nfts[tokenId].prnt+=amountToStake;
if ( _nfts[tokenId].stakeinlevel >= _levels[_nfts[tokenId].level].tolevelup && _nfts[tokenId].level < 20){
    _nfts[tokenId].level += 1;
    _nfts[tokenId].stakeinlevel = 0;
    stakeValues.multiplierTotal +=  _levels[_nfts[tokenId].level].levelmultiplier;
    if (_nfts[tokenId].level > 1){
        stakeValues.multiplierTotal -=  _levels[_nfts[tokenId].level-1].levelmultiplier;
    }
}
weightCal(tokenId);

}`

We can see the next example with more details:

Im creating a jackpot smartcontract on the XDC network.

https://xdc.blocksscan.io/address/xdc4de3fb29288452474948d16296128048e5dbc700 - > Smartcontract on XDC

https://polygonscan.com/address/0x22568f35016a8bc0c61b21b5360f7340e46ead1e - > Smartcontract on Polygon

As you can see, you cant write the smartcontract with Blockscan(You cant write also with remix). Im doing the play buttons(functions) with requires.

function buttonClick() public payable {
        require(msg.value == 1 ether, "1 XDC required to bet!");
        if (activeGame)
            require(msg.sender != getLast(), "You are already winning!");
        currentTimeWin = now + 5 minutes;
        addressArr.push(msg.sender);
        activeGame = true;
    }

We know XDC network have a problem and require().... is not working in the network.

On polygon works perfect, on xdc network is not working.

You can get the smartcontract code from the explorers, its verified!
@gzliudan
Copy link
Collaborator

gzliudan commented Aug 20, 2022 •
edited
Loading

@xcantera Do you mean the contract never reverts? Or the contract reverts but not report any reason string? Not found transaction from contract xdc4de3fb29288452474948d16296128048e5dbc700 .

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 •
edited
Loading

@gzliudan if you check what I sent you will be able to see it.

First thing: The reverts dont send any error message.

Second thing: xdc4de3fb29288452474948d16296128048e5dbc700
This is an example in what the network is useless if you put reverts, this revert is used to create a button.(send a tx to the smartcontract). You will never be able to interact with this smartcontract until you fix this issue.

If you check it on polygon, its working perfect.

As I said, if you check my examples you can easy see what its happening

@xcantera xcantera reopened this Aug 20, 2022
@gzliudan
Copy link
Collaborator

gzliudan commented Aug 20, 2022 •
edited
Loading

Would you please deploy the same contracts to test chain apothem ? Yes, ploygon chain works very well. There are some problems on xinfin chain. I will check if it works on test chain first.

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@gzliudan
Copy link
Collaborator

gzliudan commented Aug 20, 2022 •
edited
Loading

I deployed it on testnet just now: https://explorer.apothem.network/address/xdcb08ba7324663840621dd587e0a70a0eafff37e87#readContract and https://explorer.apothem.network/address/xdc7f3de014bfd22d06ec1852b0b179da1edda139d5#readContract with different solidity compiler version. The problem is same.

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@gzliudan
Copy link
Collaborator

The stake function you mentioned belongs to which contract? Would you please post the address and code?

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@gzliudan
Copy link
Collaborator

gzliudan commented Aug 20, 2022 •
edited
Loading

I checked the failed transations before 3-6 days ago, the fail reason messages are below:

missing trie Node 7f2218d7f466aa5718812126428017753d2b68e24365197fce244a6eb7ebb3c1 (path )
missing trie Node f869c6e34823b8a14f8c33f0fdeb7fb2c42936fb6fe9fc9a2d036baee5a5a325 (path )
missing trie Node 5ba03ac903a1c8eb1d5d42c9ca55225708e18dd73ab2ab9615c76a40614cf9ea (path )
missing trie Node b4b069586810a654cb73a04bacfa5d4cf5c10d0c9fe53c2a70fc8cf6df5cc4a7 (path )
missing trie Node 3d9ffa7c545c145880d1e7ab5e9a7ad9fb39498d2ec7fc5513b8918949daf446 (path )
missing trie Node b7ee0ce15752c5385e3320a39da93eef8949ef11fe435fb5deac4d640f8366a1 (path )
missing trie Node 0417cc06299708cba750a6519f120b977ee586fcd2b6ff56a15ef8b7e8cebb13 (path )
missing trie Node 2d57b886fb1f2b65ad1219e4b9c5b219c8e3eac0fae392354db8cd70db7e0900 (path )

Maybe these error messages are caused by the bug of RPC node. It seems the require commands in contract are executed right.

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@gzliudan
Copy link
Collaborator

gzliudan commented Aug 20, 2022 •
edited
Loading

We found 2 problems now:

  • Can't call write function on explorer web site.
  • The failed transation report error message: missing trie Node.

The second issue maybe has something to do with RPC node.

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@gzliudan
Copy link
Collaborator

I'll forword the information to XinFin blockchain development team.

@xcantera
Copy link
Author

xcantera commented Aug 20, 2022 via email

@gzliudan
Copy link
Collaborator

I found you are using ABDK Libraries. I opened a issue just now: abdk-consulting/abdk-libraries-solidity#23.

https://xdc.blocksscan.io/tokens/xdcf87f7dd4e47dd5bcac902c381ea0d2730db5c6ad#tokenTransfers

@BlocksScanIO
Copy link

Hello @xcantera & @gzliudan ,

You can find exact error reason on BlocksScan Explorer now , Kindly review and let us know.

Thank you

@gzliudan
Copy link
Collaborator

@BlocksScanIO Yes, the error message can be showed on blockscan exploer now. But the write functions can't be showed and called on blockscan exploer still now!

@BlocksScanIO
Copy link

Thank you for confirmation @gzliudan , Does write function for this contract work on remix ?

@gzliudan
Copy link
Collaborator

@BlocksScanIO I'm not using Remix. Please check:
https://xdc.blocksscan.io/address/xdc4de3fb29288452474948d16296128048e5dbc700
https://explorer.apothem.network/address/xdcb08ba7324663840621dd587e0a70a0eafff37e87#readContract https://explorer.apothem.network/address/xdc7f3de014bfd22d06ec1852b0b179da1edda139d5#readContract

@BlocksScanIO
Copy link

Write functions work for other verified contracts, We would analyse the given contract and get back to you with the required solution.

Tested Write function:- https://xdc.blocksscan.io/address/xdc1f37bbbfa244f6a39f8447e342dfcd3c651ce73e#readContract

@xcantera
Copy link
Author

Cool! I see our smartcontract now show the error msg on Blocksccan. I will let you if it works with our interface

@gzliudan
Copy link
Collaborator

gzliudan commented Aug 23, 2022 •
edited
Loading

The page of Write Contract should show function buttonClick() and claimPrize() in the above example contracts.

@xcantera
Copy link
Author

Yes @gzliudan, it should show function buttonClick() and claimPrize() in the above example contracts.

@xcantera
Copy link
Author

Hey, guys @gzliudan the error is still the same.

Now you need to force a failing tx to see the error on Blocksscan.

We don't want to make people force the transaction when it's going to fail 100%.

I did test here: https://explorer.xinfin.network/address/xdc4b4336ec39321ab030459b51f256e2f00b960405#transactions
(If you force the tx you can see the error message from the revert)

As you can see the revert is not working if you don't force the tx.

@gzliudan
Copy link
Collaborator

gzliudan commented Aug 31, 2022 •
edited
Loading

Which transaction is not work as you expected? What's your error now?

@xcantera
Copy link
Author

xcantera commented Sep 6, 2022

@gzliudan you need to force all the tx to get a revert. The revert should show the information before force the tx

@xcantera
Copy link
Author

xcantera commented Sep 6, 2022

@gzliudan just try on your own. Use remix and use a smart contract with reverts. You will see that you don't see any information before forcing the tx.

@xcantera
Copy link
Author

Please check the VIDEO:

2022-09-15.18-47-20.mp4

@xcantera
Copy link
Author

@gzliudan

@gzliudan
Copy link
Collaborator

gzliudan commented Sep 15, 2022 •
edited
Loading

I caught what you mean. If you execute a transaction which should be failed:

  • If use xdc blockchain: MetaMask will not alert at all
  • If use other blockchains.: MetaMask will alert when send transaction

@xcantera
Copy link
Author

xcantera commented Sep 16, 2022 via email

@gzliudan
Copy link
Collaborator

I did some tests today. When I send a fail transaction with remix, the below results occurred:

  • Metamask does not alert user sometimes when use metamask + mumbai
  • xdcpay alert when use xdcpay + apothem

I think both of metamask and xdcpay maybe alter or not alter user when send a transaction which will revert. And metamask alter me that a transaction will fail but it is executed successfully in fact last year. Both of them are not absolutely right at any time.

@xcantera
Copy link
Author

xcantera commented Sep 16, 2022 via email

@xcantera
Copy link
Author

xcantera commented Sep 16, 2022 via email

@gzliudan
Copy link
Collaborator

gzliudan commented Sep 16, 2022 •
edited
Loading

Yes, I use remix to do tests. What I checked is metamask and xdcpay whether alert user before send transaction. eg:

image

In the above example, this transaction should revert, but metamask didn't warn me.

@xcantera
Copy link
Author

xcantera commented Sep 16, 2022 via email

@gzliudan
Copy link
Collaborator

gzliudan commented Sep 16, 2022 •
edited
Loading

We will investigate this problem in detail and work out a better solution. It seems that RPC nodes need to be optimized. I deployed a test contract to mumbai and apothem chains:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract TestRevert {
    uint256 public value = 0;

    function setValue(uint256 newValue) external {
        require(newValue < 10, "newValue >= 10");

        value = newValue;
    }
}

test with mumbai RPC

code: https://mumbai.polygonscan.com/address/0x135cd8b7002c345b3da3e7712b77cae17d58b38e#code

Request

curl -X POST -H "Content-Type: application/json" https://rpc.ankr.com/polygon_mumbai -d '
{
  "jsonrpc":"2.0",
  "method":"eth_estimateGas",
  "params":[
    {
      "from":"0xD4CE02705041F04135f1949Bc835c1Fe0885513c",
      "to":"0x135cd8b7002c345b3da3e7712b77cae17d58b38e",
      "data": "0x55241077000000000000000000000000000000000000000000000000000000000000000a",
      "value":"0x0"
    }
  ],
  "id":1
}'

Response

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 3,
    "message": "execution reverted: newValue >= 10",
    "data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000e6e657756616c7565203e3d203130000000000000000000000000000000000000"
  }
}

test with apothem RPC

code: https://explorer.apothem.network/address/xdc41cfa4c7c764c39237a765788eb0596c4b7a05ba#readContract

Request:

curl -X POST -H "Content-Type: application/json" https://rpc.apothem.network -d '
{
  "jsonrpc":"2.0",
  "method":"eth_estimateGas",
  "params":[
    {
      "from":"0xD4CE02705041F04135f1949Bc835c1Fe0885513c",
      "to":"0x41cfa4c7c764c39237a765788eb0596c4b7a05ba",
      "data": "0x55241077000000000000000000000000000000000000000000000000000000000000000a",
      "value":"0x0"
    }
  ],
  "id":1
}'

Response

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": -32000,
    "message": "gas required exceeds allowance or always failing transaction"
  }
}

conclusion

Fail reason:

  • ploygon: "execution reverted: newValue >= 10"
  • xinfin: "gas required exceeds allowance or always failing transaction"

Yes, ploygon blockchain outputs better reason, xinfin blockchain needs some improvement.

@xcantera
Copy link
Author

xcantera commented Oct 11, 2022 via email

gzliudan added a commit to Carry-So/XDPoSChain that referenced this issue Oct 18, 2022
@gzliudan
internal/ethapi: EstimateGas and Call handle revert error(XinFinOrg#173)
92c10e7
@gzliudan gzliudan mentioned this issue Oct 18, 2022
Merged
wgr523 pushed a commit that referenced this issue Oct 27, 2022
@gzliudan
internal/ethapi: EstimateGas and Call handle revert error(#173) (#200)
42379f5 
hot fix for EstimateGas and Call handle revert error #173
@AnilChinchawale AnilChinchawale changed the title Critical error on the network. String reverts not working Dec 19, 2022
@AnilChinchawale
Copy link
Member

AnilChinchawale commented Jan 18, 2023 •
edited
Loading

Hi @xcantera ,

I would suggest you test Apothem RPC where we have applied the latest v1.4.6 to fix the reported issue.

Apothem TestNet RPC :-

https://rpc.apothem.network
https://erpc.apothem.network
https://apothem.xdcrpc.com

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"execution reverted: newValue \u003e= 10"}}

Screenshot 2023-01-18 at 12 58 11 PM

@nitishpatel
Copy link

Hi, the error messages didn't seem to pop up on the remix for xinfin.
I tried deploying the code snippet you added, however, it didn't seem to throw any error and goes in an endless pending state.


Here is the deployment: https://apothem.blocksscan.io/address/xdc7E2d03BEfeD07913F88770f22105EE222bb72632

You can check the video attachment for the same when tried with XDCPay.
2023-02-27.22-20-33.mp4
However, it does pop up an error while using Metamask
2023-02-27.22-24-33.mp4

@gzliudan
Copy link
Collaborator

gzliudan commented Feb 28, 2023 •
edited
Loading

@nitishpatel Do you mean the following result ?

Remix Wallet Prompt revert
remix.ethereum.org MetaMask Yes
remix.xinfin.network MetaMask Yes
remix.ethereum.org XdcPay No
remix.xinfin.network XdcPay No

@nitishpatel
Copy link

@nitishpatel Do you mean the following result ?

Remix Wallet Prompt revert
remix.ethereum.org MetaMask Yes
remix.xinfin.network MetaMask Yes
remix.ethereum.org XdcPay No
remix.xinfin.network XdcPay No

@gzliudan Yes, Also It behaves similarly on dapp, XDCPay does return an error but with no string.
I wrote this a month back when I wasn't aware of this GitHub issue, it compares how each of the wallets is behaving differently for the same error, which is what my most concern is because XDCPay does returns an error but the reason is not part of it, so there is no way to find out why it's being reverted.

Context this error comes because I have a check before any transaction is done to see if it's likely to fail.

const result = await web3.eth.call(getCallOptionsFromCallObj(methodCallObj), blockNumber);

One can also check if its failing by just doing a methodCallObj.estimateGas();
https://www.xdc.dev/ncode/error-strings-support-in-xdc-4bec

@AnilChinchawale
Copy link
Member

Hello, @nitishpatel Does XDCPay 2.0 Work for you with remix.xinfin.network ?

@nitishpatel
Copy link

Hello, @nitishpatel Does XDCPay 2.0 Work for you with remix.xinfin.network ?
@AnilChinchawale Yes XDCPay 2.0 works and returns the error in a similar way as metamask does.

@wjrjerome
Copy link
Collaborator

Closing the issues as it was resolved

wanwiset25 pushed a commit to XinFinOrg/XDC-Subnet that referenced this issue Apr 1, 2024
@gzliudan @wanwiset25
internal/ethapi: EstimateGas and Call handle revert error(#173) (#200)
34fbba1 
hot fix for EstimateGas and Call handle revert error XinFinOrg/XDPoSChain#173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@gzliudan @wjrjerome @AnilChinchawale @xcantera @nitishpatel @BlocksScanIO