Fallback function reverts without an apparent reason

[barakman]

I am trying to execute a transaction in which a contract function transfers ether to another contract, thus invoking the other contract's fallback function:

function() external payable {
    require(someInternalViewFuncWhichReturnsTrue());
}

When running truffle test, the transaction completes successfully.
When running solidity-coverage, the transaction reverts at the fallback function.

As you can imagine, it took me a very long time to formulate this conclusion.

And how did I formulate that conclusion?

First, I changed the fallback function to this:

function() external payable {
    revert("7777777");
}

Where the transaction has reverted with a revert 7777777 message in both Truffle and SC.

Then, I changed it to this:

function() external payable {
    require(someInternalViewFuncWhichReturnsTrue(), "7777777");
}

Where the transaction has reverted with a revert message only in SC.

Finally, I changed it to this:

function() external payable {
}

Where the transaction has completed successfully in both Truffle and SC.

My guess is that it exceeds the gas stipend (2300) because of the event-emitting that SC injects to it:

    function() external payable {
        emit __FunctionCoverageMyContract('C:/Users/.../MyContract.sol',7);
        emit __CoverageMyContract('C:/Users/.../MyContract.sol',168);
        emit __AssertPreCoverageMyContract('C:/Users/.../MyContract.sol',8);
        emit __StatementCoverageMyContract('C:/Users/.../MyContract.sol',12);
        require(someInternalViewFuncWhichReturnsTrue(), "7777777");
        emit __AssertPostCoverageMyContract('C:/Users/.../MyContract.sol',8);
    }

I am using:

• Windows 10
• node v10.16.0
• npm v6.9.0
• solidity-coverage v0.6.7
• truffle v4.1.16 (relying on solc 0.4.26)

Any thoughts or ideas how to workaround this?

Thanks

[barakman]

Here is one workaround that I just found:
Put the contents of someInternalViewFuncWhichReturnsTrue directly inside the require statement, which probably saves enough gas for allowing the transaction to complete successfully.
I'm not a great fan of this solution.
It might be a good point to mention that I'm using compiler optimization-runs=200.

Update - I have confirmed the above by running:

    event GGGGG(uint256 indexed gasssss);
    function() external payable {
        require(someInternalViewFuncWhichReturnsTrue());
        emit GGGGG(gasleft());
    }

And then:

    event GGGGG(uint256 indexed gasssss);
    function() external payable {
        require(<the contents of someInternalViewFuncWhichReturnsTrue>);
        emit GGGGG(gasleft());
    }

The first one gives 1973, while the second one gives 2000, which means that the first one has consumed 27 gas units more than the second one. I am guessing that the transaction was just on the brinks of exceeding the gas stipend provided to the fallback, and with SC's additional events, it has exceeded that point.
I tested the above via Truffle of course, because the first one reverts under SC due to the aforementioned reasons.
Since the gas-usage is subjected to the underlying EVM used, it might be a good point to mention that I'm using ganache-cli@6.7.0 for Truffle and ethereumjs-testrpc-sc@6.5.1-sc.1 for SC.
I believe that both emulate Byzantium, but I'm not 100% sure of that...

[cgewecke]

Thanks for reporting and for describing the work-around.

testrpc-sc sets an emitFreeLogs flag for the vm which should make events free...something's not right there.

[barakman]

@cgewecke :
Perhaps it's not about the gas consumption, but my workaround above implies otherwise IMO:

1. The transaction reverts with require(func()) but completes successfully with require(map[address(0)].x), where:
   • mapping(address => Struct) public map;
   • struct Struct {uint256 a; uint32 b; bool c; bool d; bool x;}
   • function func() view returns(bool) {return map[address(0)].x;}
2. Calling gasleft() immediately after the require statement in the reverting transaction shows a value lower than calling gasleft() immediately after the require statement in the successful transaction

[cgewecke]

Perhaps it's not about the gas consumption, but my workaround above implies otherwise IMO:

Oh yes, agree. Something's not working as expected in that version of testrpc-sc.

[barakman]

Any chance it could be fixed on 0.6.x, or am I the only one left using it?
(I have 'Upgrade SC to 0.7.x' on my Trello but it's going to take a long while for me to get there).
Thanks.

[cgewecke]

The first one gives 1973, while the second one gives 2000, which means that the first one has consumed 27 gas units more than the second one.

Actually, @barakman on second glance - I'm not sure you'd be better off with 0.7.x or that much can be done with testrpc-sc. You're very close to the wire there - basically 327 gas?

I think you're getting free events but might be incurring opcode charges for setting them up or something. A single LOG is 375 plus topics at 375 each and SC is injecting 5 in that function which should take you way over 2300.

0.7.x would likely still fail here - its instrumentation statements cost ~114 gas each.

Very sorry about this. There are things about the way SC works which are fundamentally not great, esp in tight gas conditions.

[barakman]

@cgewecke : Thanks.

As I wrote above - "I am guessing that the transaction was just on the brinks of exceeding the gas stipend provided to the fallback, and with SC's additional events, it has exceeded that point".

That said, I think your interpretation of me being "very close to the wire there - basically 327 gas" actually implies that I'm not that close to the wire, since I spend only 327 out of 2300 available.

In addition to that, you're also suggesting that the 5 events injected by SC consume an additional amount larger than 1973, leading the function to exceed the stipend. However, 5 events times 375 per event equals 1900, so that doesn't quite explain it (since 327 + 1900 < 2300).

I believe that the actual formula for the gas consumption of an event is:

375 + 375 * numberOfIndexedParameters + numberOfUnindexedBits

In which case, the function should have reverted even after applying my workaround.

So while your analysis fails to explain why the original transaction reverts, my analysis fails to explain why the workaround allows the transaction to complete successfully.

I guess I might be missing something here (or perhaps the formula above is incorrect).

In any case, to my understand, there is not much that you can do about it... is that correct?

[cgewecke]

@barakman I've misunderstood what's in the original fallback. I thought you had an event in there and the code below costs 1973 on ganache-cli, so you'd only have a few hundred gas left.

    function() external payable {
        require(someInternalViewFuncWhichReturnsTrue());
        emit GGGGG(gasleft());
    }

Let me investigate what it would take to get an 0.7.x script working on your repo sometime in the next few days. I think that's probably the best option for the long term since you will be able to use any ganache-cli with it and be synced with updates etc.

Is the repo/branch you're working on https://github.com/bancorprotocol/contracts at master?

[barakman]

@cgewecke : It's actually in this ongoing branch, so there could be some changes ahead, but most likely not around the related area.

Running SC on it may take about an hour if not more, so you may want to delete all tests except for the one relevant to this issue (as well as the two folders under the same path).

Moreover, this test scans through many use-cases before finally reaching the one which would cause a problem without my workaround.

So you may want to "strip them out" by changing this line to:

const combinations = [[initWithEtherConnector, 23, false]];

Then, in order to actually make the transaction revert, you'll need to undo my workaround, by replacing the current require statement with the one which is commented-out on the same line.

I recommend that you add a message in the (new) require statement, in order to get an absolute proof that the transaction does not revert as a result of the condition inside that statement evaluating to false.

Thank you for helping out :)