Wrong gas estimation

[pstuermlinger]

Expected Behavior

The gas estimation should be accurate.

Current Behavior

Gas estimation is off under some circumstances. Probably when Storage gets freed.
It's working when MetaMask is talking with Geth, but not when Ganache is running.
By following the steps below, the 2nd transaction fails because it runs out of gas.

Steps to Reproduce (for bugs)

I've created an example contract.
Connect Remix to your local Ganache and deploy MyContract.

SafeMath16.sol:

pragma solidity ^0.4.23;

library SafeMath16 {
  function mul(uint16 a, uint16 b) internal pure returns (uint16) {
    if (a == 0) {
      return 0;
    }
    uint16 c = a * b;
    assert(c / a == b);
    return c;
  }
  function div(uint16 a, uint16 b) internal pure returns (uint16) {
    // assert(b > 0); // Solidity automatically throws when dividing by 0
    uint16 c = a / b;
    // assert(a == b * c + a % b); // There is no case in which this doesn’t hold
    return c;
  }
  function sub(uint16 a, uint16 b) internal pure returns (uint16) {
    assert(b <= a);
    return a - b;
  }
  function add(uint16 a, uint16 b) internal pure returns (uint16) {
    uint16 c = a + b;
    assert(c >= a);
    return c;
  }
}

MyContract.sol:

pragma solidity ^0.4.24;

import "./SafeMath16.sol";

contract MyContract {
  using SafeMath16 for uint16;
    
  bool[80][125] grid;

  struct House {
    address owner;
    uint16 x;
    uint16 y;
    uint16 width;
    uint16 height;
  }

  House[] houses;

  // The following holds a list of currently existing houses (without holes between the indexes)
  uint256[] houseIds;

  // Holds the mapping from houseID to its index in the above houseIds array.
  // If a house gets demolished, we know which index to delete and being filled
  // with the last element instead.
  mapping (uint256 => uint256) houseIdToIndex;
  
  /*********************************************************
   *                                                       *
   *                       Events                          *
   *                                                       *
   *********************************************************/

  event Build(uint256 indexed id, address indexed owner, uint16 x, uint16 y, uint16 width, uint16 height);
  event Demolish(uint256 indexed id, address indexed owner);
  
  /*********************************************************
   *                                                       *
   *                      Modifiers                        *
   *                                                       *
   *********************************************************/

  modifier onlyHouseOwner(uint256 _id) {
    require(houses[_id].owner == msg.sender, "Access denied.");

    _;
  }
  
  modifier houseExists(uint256 _id) {
    uint256 index = houseIdToIndex[_id];
    require(houseIds[index] == _id, "House does not exist.");

    _;
  }
  
  /*********************************************************
   *                                                       *
   *                       Logic                           *
   *                                                       *
   *********************************************************/
  
  function build(uint16 _x, uint16 _y, uint16 _width, uint16 _height)
    public returns (uint)
  {
    
    for (uint16 i = 0; i < _width; i++) {
      for (uint16 j = 0; j < _height; j++) {
        uint16 x = _x.add(i);
        uint16 y = _y.add(j);

        if (grid[x][y]) {
          revert("Already claimed");
        }

        grid[x][y] = true;
      }
    }

    // Create house.
    uint id = createHouse(_x, _y, _width, _height);
    
    emit Build(id, msg.sender, _x, _y, _width, _height);
    return id;
  }
  
  function createHouse(uint16 _x, uint16 _y, uint16 _width, uint16 _height) internal returns (uint id) {
    House memory house = House(msg.sender, _x, _y, _width, _height);
    id = houses.push(house) - 1;
    uint256 key = houseIds.push(id) - 1;
    houseIdToIndex[id] = key;
    return id;
  }
  
  function demolish(uint256 _id) houseExists(_id) onlyHouseOwner(_id) public {
    for (uint16 i = 0; i < houses[_id].width; i++) {
      for (uint16 j = 0; j < houses[_id].height; j++) {
        uint16 x = houses[_id].x.add(i);
        uint16 y = houses[_id].y.add(j);

        // Mark block as free.
        grid[x][y] = false;
      }
    }

    // Delete house
    delete houses[_id];
    // Reorganize index array and map
    uint256 key = houseIdToIndex[_id];
    // Fill gap with last element of houseIds
    houseIds[key] = houseIds[houseIds.length - 1];
    // Update houseIdToIndex
    uint256 movedHouseId = houseIds[key];
    houseIdToIndex[movedHouseId] = key;
    // Decrease length of houseIds array by 1
    houseIds.length--;

    emit Demolish(_id, msg.sender);
  }
}

1. Call method build with parameters: 0, 0, 1, 1
2. Call method demolish with parameter: 0

Context

I'm trying to delete structs from an array while maintaining index- and ID-mapping arrays.

Your Environment

• Version used: Ganache 1.2.1

[mikeseese]

Thanks for the write up @Haggins! We really appreciate it!! Issue #26 already is covering this. I'm going to close this in favor of #26, but CC'ing @davidmurdoch so he can read this and make sure he's not missing anything in the implementation of #26 or #147.

[davidmurdoch]

@Haggins OOG errors due to transactions with refunds (from setting storage to 0 or a selfdestruct) should be fixed by #141 and will be available in the next release.

[pstuermlinger]

Thank you both for the super quick replies!