Undocumented breaking change in update to 2.2.1

[wbt]

Prompted by the interface, I recently updated Ganache from v1.2.1 to v1.2.2 "Bug Fixes" which I thought would be a minor update fixing some bugs that I may not have even yet encountered.

Expected Behavior

I expected that my code would continue working as it did in v1.2.1. I expected the only notable changes to be those described in the release notes (maybe some performance improvement, new "clear logs" button, better UI in small windows) and even clicking through to the ganache-core 2.2.1 release notes I did not see anything that seemed likely to interfere with the function of my code.

Current Behavior

After upgrade, a transaction that sends most of an account's value reports Error: sender doesn't have enough funds to send tx. Since I had a script with a transaction like this, it breaks the script.

Steps to Reproduce (for bugs)

1. Restart Ganache to have 10 accounts each with 100.00 ETH.
2. Send all but 100000 wei from one account to another.
   FORMER/EXPECTED RESULT: Transfer succeeds.
   ACTUAL RESULT:
   Error: sender doesn't have enough funds to send tx. The upfront cost is: 100000179999999900000 and the sender's account only has: 100000000000000000000

Possible Solution

1. Change documentation to reflect that this update introduces a change that can break operation of any action that sends most of the value of one account to another. Note "The gas cost of a simple transfer has increased to at least 18000 gas * your gasPrice for a simple transfer, which may have an impact if you have a step transferring most of an account's value."
2. Fix the gas estimation for simple transfers.

If the transfer should not have worked before, the documentation change is more appropriate.
Generally changes that break code and require adaptation in order to integrate should be documented so that people have a better idea what to expect when upgrading. Such changes should maybe get a more significant version number updated instead of the patch field at the end.

Your Environment

• Version used: Ganache 1.2.2, which incorporates ganache-core 2.2.1
• Operating System and version: Win10 Pro
• Running with: Truffle v4.1.13
• truffle.js gasPrice: 10000000000 (10 Gwei)

Misc

I previously thought this was related to #156 ("Add tests for gas estimate for simple value transfer between accounts") and reported it there. After some discussion I see this as a separately tracked Issue. Adding tests still seems like a good idea, including tests to prevent recurrence of this Issue.

[mikeseese]

@wbt Thanks for reporting this! Unfortunately, I don't believe the breaking change. I'm not quite sure how your steps worked in a prior version per the following math:

                  21000 // the standard gas for simple transactions
*           10000000000 // your gas price of 10 gwei
=       210000000000000 // the amount of wei you'll need for the transaction

  100000000000000000000 // account balance
-       210000000000000 // the amount of wei you'll need for the transaction using price of 10 gwei
=  99999790000000000000 // the remaining balance you can send

and you want to send
  100000000000000000000 // account balance
-                100000 // the amount you don't want to send
=  99999999999999900000 // the amount you want to send

and
  99999999999999900000 
> 99999790000000000000

So given the reproduction steps you've provided, you do not have enough funds for that transaction. Am I missing something?

Further, you say the upfront cost would be 100000179999999900000, but I believe it should be even more: 100000209999999900000

[benjamincburns]

In semver, changes are breaking if the public interface is structurally changed in such a way as to render programs which connect to said interface inoperable. Changes which impact the output of a given method for a given input but which don't impact the structure of the method are generally considered nonbreaking.

In this case we improved the accuracy of eth_estimateGas but its interface is unchanged from the previous version. As a result, this change warranted a minor version number increment.

Semver also accounts for changes which are semantically breaking, which is when a method's name and structure remain the same but the meaning is changed to the point where it is now conveying a completely different idea. When this happens a major version bump is warranted. However in this case we've adjusted eth_estimateGas to be more in line with the specification in the Ethereum foundation's RPC wiki, and it's our opinion that this doesn't alter the semantics of the method enough to call it a breaking change.

It's impossible for us to guarantee that logic changes won't break somebody's code, as we can't begin to account for what assumptions everyone has encoded into their own logic. If we tried doing that, literally every non-additive change would need to be a major version, and that's just not very reasonable.

I'm closing this issue because there's nothing to be done. However if we do slip up in the future I do encourage you to raise issues for the sort of breaking changes I describe above.

[wbt]

I do think an update to the release notes/documentation would be appropriate, describing the condition and how a user may need to fix their code so it won't break with the update.

I don't think guarantees are necessary but including release notes about what has changed in the software and how users need to accommodate those changes are generally appropriate. When something is missing in those notes and this is pointed out, modifying the release notes seems appropriate.

[wbt]

@seesemichaelj "the amount you don't want to send" is useful for paying gas for sending the rest. I'm also not 100% sure how the math worked out that it worked before (nor did it matter a lot as it's not production code), but if you don't believe me you can download v1.2.1, run it fresh, and include these lines in a migration file:

module.exports = function(deployer) {
    web3.eth.getAccounts(function(error,accounts) {
        var amount = web3.eth.getBalance(accounts[3])-100000; //for Ganache <= 1.2.1
        web3.eth.sendTransaction({from:accounts[3], to:accounts[0], value: (amount)});
    }
}

For Truffle.js you can use something like:

module.exports = {
  networks: {
    development: {
      gasPrice: 10000000000, 
      host: "127.0.0.1",
      port: 7545,
      network_id: "5777" 
    }
  }
};

That's not the core issue here, though, which is documentation. I would still like to see the issue reopened until the documentation has been updated to let people know that this can be an issue requiring code changes to run with the updated Ganache.

[davidmurdoch]

@wbt. Thanks for the test case. I've tracked down where this issue is stemming from.

Just to clarify, the gasPrice specified in your truffle.js file is only used for contract deployments; it does not set a default gasPrice in web3 calls or change the gasPrice value of the node you're connecting to.

In web3@0.20.*, which is currently the version of web3 available in truffle migrations, sendTransaction without an explicit gasPrice defaults to undefined. In web3@1.0.0 gasPrice defaults to the result of a call to eth_gasPrice, which, in the case of a default Ganache instance, is 20000000000. There was a bug in the version of ganache-core used by Ganache v1.2.1 that caused calls that specify a gasPrice of undefined, which web3@0.20.* was sending, to default to a gasPrice of 0x1 instead of the value specified in the Ganache instance. In Ganache v1.2.2 this bug was fixed; we now correctly set the gasPrice to Ganache's "Gas Price" value when a transaction's gasPrice is undefined or null.

Sorry for the headache this may have caused you. I'll go ahead and update the Ganache and ganache-core release notes to be more specific regarding the gasPrice bug fix. Thank you for your persistence on this issue.

[wbt]

This is an issue again with the next update. Allowing 180000000000000 wei was enough to cover the cost of a balance transfer after upgrading to Ganache 1.2.2, but in Ganache 1.2.3, that same code produces Error: sender doesn't have enough funds to send tx. The upfront cost is: 100001620000000000000 and the sender's account only has: 100000000000000000000
This is the same code coming up against the same undocumented breaking change in the very next patch release, titled "Core Updates and Small Bug Fixes" with no documentation about the breaking change.

Can I propose a new rule that any changes affecting gas cost calculation, up or down, be documented as a breaking change, in notes and semver?

[davidmurdoch]

Adjustments to gas costs are bug fixes, and our current position is that bug fixes are not breaking changes. Do you call eth_estimateGas before each transaction? We are open to further discussion on this topic if you have a strong position on it. Let me know if you'd like to schedule some time to discuss this further.

With that said, the above error you are describing sounds like a bug unrelated to gas estimation; do you mind opening a new issue with some details about the transaction itself as well as any transaction made prior to the failing transaction?

[wbt]

We are open to further discussion on this topic if you have a strong position on it. Let me know if you'd like to schedule some time to discuss this further.

Yes, I do have a strong position on it and would like to discuss further. A breaking change is when something that used to work, no longer works. If it is possible to get it working again, doing so requires some change to other application code that uses the same base technology. If I run code fully successfully with a dependency at 1.2.2, and update the dependency to 1.2.3 with no other changes, I expect the code to continue to run successfully as it did before. When it doesn't, I think there's been an error in semantic versioning.

When the release notes for the new version contain no documentation saying that something changed which I might be able to guess could cause that outcome from updating, but the update does in fact change behavior not mentioned anywhere in the release notes that causes something previously working to no longer work, I think both the release notes documentation AND the semantic versioning are broken. I also generally stop updating as frequently as I otherwise might because I can no longer trust the release notes to inform me if what used to work is going to stop working.

In the cases where this is observed, I am starting with a freshly restarted Ganache, and use truffle migrate --reset to run the standard initial migration 1_initial_migration.js:

var Migrations = artifacts.require("./Migrations.sol");

module.exports = function(deployer) {
    deployer.deploy(Migrations);
};

In the very next transaction, I attempt to transfer balance from accounts[2] to accounts[0], where balance = web3.eth.getBalance(accounts[2])-x and x is a constant that apparently has to change with every recent patch release of ganache-core. This is the first and only transaction from accounts[2]; nothing that comes before it should have any impact on if that operation succeeds.

[wbt]

Adjustments to gas costs are bug fixes, and our current position is that bug fixes are not breaking changes.

From the original point 5 of this post, I think its author would agree. Two weeks ago, I thought that made sense, and based on experience in this Issue I'd have thought only increases in gas costs could be breaking changes. However, following the experience with Constantinople around a gas cost decrease, which turned out to be a very serious issue, I think it would be a fair new rule to label any gas cost adjustments as breaking changes.