Dynamic return data decoder should not discard components

[LefterisJP]

Problem Description

The scenario is as follows. Let's imagine a contract A with a public struct that also contains variable sized data types not all of which are placed at the end of the struct:

contract A {

...

    struct Proposal {
        address recipient;
        uint amount;
        string description;
        uint votingDeadline;
        bool open;
        bool proposalPassed;
        bytes32 proposalHash;
        uint proposalDeposit;
        bool newCurator;
        SplitData[] splitData;
        uint yea;
        uint nay;
        mapping (address => bool) votedYes;
        mapping (address => bool) votedNo;
        address creator;
    }

    struct SplitData {
        uint splitBalance;
        uint totalSupply;
        uint rewardToken;
        DAO newDAO;
    }
    Proposal[] public proposals;

...

}

Now imagine a contract B trying to interface with contract A:

contract B {
A public other;

function foo(uint _proposalID) {
        if (_proposalID > other.numberOfProposals()) {
            return false;
        }
        var (,,,,proposalPassed, proposalHash,,, yea, nay,) = other.proposals(_proposalID);
}

}

Expected Results

At this call:

var (,,,,proposalPassed, proposalHash,,, yea, nay,) = other.proposals(_proposalID);

We are decoding the struct by extracting all the non-variable sized members. We would expect to see the actual values stored in the solidity variables.

Actual Results

The variables in the above call contain data different than what is expected. The data seems to be moved around, indicating some problem with the way solidity decodes them.

More specifically proposalHash appears to be 0x0 while it's not and in yea we get the value of nay and vice versa.

[chriseth]

Suggested fix: Introduce a new type that is used in the return type of an external function call, replaces dynamic types, takes up 32 bytes and is useless otherwise.

[chriseth]

It turns out that the situation is different for accessor functions than it is for general externally-visible functions: for accessors, the dynamic types are not even part of their public interface.

[chriseth]

Going in reverse again: It actually is fixed by that PR because strings and bytes are still present for accessors.

[LefterisJP]

EDIT: The comment below is mistaken but am leaving this here for sake of posterity and in order to not break the discussion with Christian.

Hmm I tried a solc compiled with this PR included: #624
and unfortunately the issue seems to still be there. It does not seem to totally solve the problem.

This time all returned and decoded data were zero. I tried to use a simple contract that includes the DAO contract and wanted to see if the voting deadline and yay and nay would be read properly:

var (,,,votingDeadline,,,,,,yea,nay,) = client.proposals(proposalID);

So when doing the above all 3 variables were zero, while they should have actual values.

[chriseth]

Perhaps it is an issue about the wildcards? Can you try something like
var (a,b,c,d,votingDeadline,...?

[LefterisJP]

No sorry False alarm. It works. My test had a mistake. I can confirm that the above works perfectly fine with this PR in and does not work without this PR.

👍

[chriseth]

Fixed with #624

[JasonCoombs]

I don't see the problem here, and think a mistake has been made. This PR should be reverted and should not ever go into production. The original bug report was a bug in the mind of the programmer who expected there to be place-holders for dynamic structure elements, when everyone already knew about the lack of support for those elements from the automatic public accessor function for public variables within contracts. The "fix" that was appropriate for this "showstopper" was for the programmer to stop expecting something different from what reality has provided to calling code since the automatic public accessor function was first created.

Consider what PR 598 is going to do if it is deployed to production... Nobody will ever know which version of the solidity compiler was used to compile a particular contract, so the result from a call to the public accessor function will be unpredictable in live blockchains. There's no mechanism for preventing pre-598 contracts from being deployed to a blockchain, so it will happen even though it doesn't make any sense to compile with old compilers, and all this confusion is being created for why, exactly? It seems to me the confusion is being created simply because the original "bug report" was not categorized as a "feature request" for a new alternative to the automatic public accessor function. There is going to need to be an alternative versionable public accessor function in the future, so put it on the list of features and we'll build it, but leave the original functionality please to avoid causing avoidable problems in the real world.

I strongly recommend AGAINST merging PR 598 into the master branch.

[LefterisJP]

@JasonCoombs This is neither the first nor the last solidity breaking change. There is a reason why whenever you deploy a contract live on the blockchain you also need to provide the solidity version.

[redsquirrel]

@LefterisJP

whenever you deploy a contract live on the blockchain you also need to provide the solidity version.

I haven't heard this before. Are you saying that it's a best practice to store the solidity version in the contract itself so that it's self-documenting?

[chriseth]

@JasonCoombs I think you might have misunderstood what this PR does. It does not change the behaviour of accessor functions. Accessor functions have always dropped some array elements and included some other array elements. Specifically, those with packed encodings (bytes, string) are included and padded ones (int8[], uint[], byte[]) are ignored.

This PR is about making it possible to correctly decode return values from accessor functions from within Solidity. Solidity wrongfully removed all array types from the return type, although string and bytes return types are indeed present in the return value. Until EIP-5/8 is accepted, there is no nice way to decode them, so I added placeholders that will be replaced by their proper type once the EVM is upgraded.

To be clear, Solidity tries very very hard not to introduce any breaking change, and even harder not to introduce them at the point where new contracts interact with existing contracts.

[chriseth]

@redsquirrel there is certainly no need to provide the compiler version as a means to check compatibility, but such a feature is certainly planned for source code verification: #611

[JasonCoombs]

@chriseth what this PR does is make it impossible for client code which calls an accessor function to know what to expect unless it has hard-coded knowledge of the solidity compiler version used to produce the bytecode of the accessor function. This means forget about iterating across the blockchain and calling accessor functions, and existing code won't work for newly-compiled contracts -- perhaps you have misunderstood the practical impact of "fixing" what is clearly not a bug but is just an arbitrary design decision which there is no good rationale to alter... You're going to cause "contracts" to be "un-enforceable" on the blockchain until and unless the contracts are replaced with new contracts. This one PR, if implemented, demonstrates a fundamental flaw in Ethereum that will deter its wider adoption.

Suppose Contract A promises to deliver Eth to a recipient B from every Contract C that is deployed to the blockchain. Suppose Contract A is written in such a way as to attempt to query Contract C using an accessor function to keep a record of something, such as the source of the Eth that is going to be transferred, and suppose Contract C is designed with public padded array elements. Contract A must be able to read Contract C regardless of the solidity compiler used to deploy Contract C, but with what you're doing here Contract A is going to be unable to do so, period. End of Contract A.

There will be so many instances in the real world in which people presume that "if they like their smart contract, they can keep their smart contract" that intentionally causing contracts to break in the way that is being proposed here is the real show-stopper. Doing this basically puts an end to Ethereum. You're turning it into a novelty rather than a reliable platform, and why? Just because you don't want to create a different versionable public accessor function v2 -- again, I do understand what this PR is trying to accomplish and I am saying it is very, very wrong and should not be implemented. You are violating the most important covenant that everyone who deploys a contract or who relies on a deployed contract presumes will never be violated! This is not a bug in Contract A it is an arbitrary change of interface that alters the way public accessor functions return values to the caller and it serves no purpose whatsoever.

[destenson]

@JasonCoombs, this PR sounds like it breaks the ABI. If the ABI is broken the system is fundamentally broken. Imagine we wake up tomorrow & all of our computers, our phones, simply couldn't run anything anymore without upgrading them? That's fine on occasion for voluntary major upgrades, or switch to a different platform, but not for this. I have to agree with you Jason, this does not sound good at all. Most people in the software world know, backwards compatibility is very important and should only be violated for good reason.

[VoR0220]

@JasonCoombs @destenson it does neither of that. What this does do is say that all future contracts will not work in the way a contract prior to this PR that had been deployed will work. That's all it does. Your previous contracts work just as they did prior.

[VoR0220]

I should also add that as @chriseth has stated, the functionality will be returned in an upgraded format once we are able to do this in a pleasant way so that it works across all types. Lest there be confusion here.

[chriseth]

@JasonCoombs @destenson as I have explained in an earlier comment, this change does not in any way break ABI compatibility. This means it does not change the way contracts accept input or return output values.

I absolutely agree with the fact that ABI compatibility should never ever be broken. This PR does NOT break ABI compatibility.