Fix duplicate hash logic (#868)

lib/collector.js

@@ -28,6 +28,8 @@ class DataCollector {
 
     this.lastHash = null;
     this.viaIR = viaIR;
+    this.pcZeroCounter = 0;
+    this.lastPcZeroCount = 0;
   }
 
   /**
@@ -36,11 +38,13 @@ class DataCollector {
    * @param  {Object} info  vm step info
    */
   step(info){
+    if (info.pc === 0) this.pcZeroCounter++;
+
     try {
       if (this.validOpcodes[info.opcode.name] && info.stack.length > 0){
         const idx = info.stack.length - 1;
         let hash = '0x' +  info.stack[idx].toString(16);
-        this._registerHash(hash)
+        this._registerHash(hash);
       }
     } catch (err) { /*Ignore*/ };
   }
@@ -66,8 +70,10 @@ class DataCollector {
 
     if(this.instrumentationData[hash]){
       // abi.encode (used to circumvent viaIR) sometimes puts the hash on the stack twice
-      if (this.lastHash !== hash) {
+      // We should only skip duplicate hashes *within* a transaction (see issue #863)
+      if (this.lastHash !== hash || this.lastPcZeroCount !== this.pcZeroCounter) {
         this.lastHash = hash;
+        this.lastPcZeroCount = this.pcZeroCounter;
         this.instrumentationData[hash].hits++
       }
       return;

test/integration/standard.js

@@ -424,6 +424,11 @@ describe('Hardhat Plugin: standard use cases', function() {
         file: mock.pathToContract(hardhatConfig, 'ModifiersC.sol'),
         pct: 25
       },
+      {
+        file: mock.pathToContract(hardhatConfig, 'ModifiersD.sol'),
+        pct: 100
+      },
+
     ];
 
     verify.branchCoverage(expected);

test/sources/projects/modifiers/contracts/ModifiersD.sol

@@ -0,0 +1,12 @@
+pragma solidity >=0.8.0 <0.9.0;
+
+import "./../external/Ownable.sol";
+
+contract ModifiersD is Ownable {
+    constructor() Ownable(msg.sender) {}
+
+    function a() public onlyOwner {
+    }
+}
+
+

test/sources/projects/modifiers/external/Context.sol

@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: MIT
+// OpenZeppelin Contracts (last updated v5.0.1) (utils/Context.sol)
+
+pragma solidity >=0.8.0 <0.9.0;
+
+/**
+ * @dev Provides information about the current execution context, including the
+ * sender of the transaction and its data. While these are generally available
+ * via msg.sender and msg.data, they should not be accessed in such a direct
+ * manner, since when dealing with meta-transactions the account sending and
+ * paying for execution may not be the actual sender (as far as an application
+ * is concerned).
+ *
+ * This contract is only required for intermediate, library-like contracts.
+ */
+abstract contract Context {
+    function _msgSender() internal view virtual returns (address) {
+        return msg.sender;
+    }
+
+    function _msgData() internal view virtual returns (bytes calldata) {
+        return msg.data;
+    }
+
+    function _contextSuffixLength() internal view virtual returns (uint256) {
+        return 0;
+    }
+}

test/sources/projects/modifiers/external/Ownable.sol

@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: MIT
+// OpenZeppelin Contracts (last updated v5.0.0) (access/Ownable.sol)
+
+pragma solidity >=0.8.0 <0.9.0;
+
+import {Context} from "./Context.sol";
+
+/**
+ * @dev Contract module which provides a basic access control mechanism, where
+ * there is an account (an owner) that can be granted exclusive access to
+ * specific functions.
+ *
+ * The initial owner is set to the address provided by the deployer. This can
+ * later be changed with {transferOwnership}.
+ *
+ * This module is used through inheritance. It will make available the modifier
+ * `onlyOwner`, which can be applied to your functions to restrict their use to
+ * the owner.
+ */
+contract Ownable is Context {
+    address private _owner;
+
+    /**
+     * @dev The caller account is not authorized to perform an operation.
+     */
+    error OwnableUnauthorizedAccount(address account);
+
+    /**
+     * @dev The owner is not a valid owner account. (eg. `address(0)`)
+     */
+    error OwnableInvalidOwner(address owner);
+
+    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
+
+    /**
+     * @dev Initializes the contract setting the address provided by the deployer as the initial owner.
+     */
+    constructor(address initialOwner) {
+        if (initialOwner == address(0)) {
+            revert OwnableInvalidOwner(address(0));
+        }
+        _transferOwnership(initialOwner);
+    }
+
+    /**
+     * @dev Throws if called by any account other than the owner.
+     */
+    modifier onlyOwner() {
+        _checkOwner();
+        _;
+    }
+
+    /**
+     * @dev Returns the address of the current owner.
+     */
+    function owner() public view returns (address) {
+        return _owner;
+    }
+
+    /**
+     * @dev Throws if the sender is not the owner.
+     */
+    function _checkOwner() internal view {
+        if (owner() != _msgSender()) {
+            revert OwnableUnauthorizedAccount(_msgSender());
+        }
+    }
+
+    /**
+     * @dev Leaves the contract without owner. It will not be possible to call
+     * `onlyOwner` functions. Can only be called by the current owner.
+     *
+     * NOTE: Renouncing ownership will leave the contract without an owner,
+     * thereby disabling any functionality that is only available to the owner.
+     */
+    function renounceOwnership() public onlyOwner {
+        _transferOwnership(address(0));
+    }
+
+    /**
+     * @dev Transfers ownership of the contract to a new account (`newOwner`).
+     * Can only be called by the current owner.
+     */
+    function transferOwnership(address newOwner) public onlyOwner {
+        if (newOwner == address(0)) {
+            revert OwnableInvalidOwner(address(0));
+        }
+        _transferOwnership(newOwner);
+    }
+
+    /**
+     * @dev Transfers ownership of the contract to a new account (`newOwner`).
+     * Internal function without access restriction.
+     */
+    function _transferOwnership(address newOwner) internal {
+        address oldOwner = _owner;
+        _owner = newOwner;
+        emit OwnershipTransferred(oldOwner, newOwner);
+    }
+}

test/sources/projects/modifiers/test/modifiers.js

@@ -1,5 +1,6 @@
 const ModifiersA = artifacts.require("ModifiersA");
 const ModifiersC = artifacts.require("ModifiersC");
+const ModifiersD = artifacts.require("ModifiersD");
 
 contract("Modifiers", function(accounts) {
   let instance;
@@ -7,6 +8,7 @@ contract("Modifiers", function(accounts) {
   before(async () => {
     A = await ModifiersA.new();
     C = await ModifiersC.new();
+    D = await ModifiersD.new();
   })
 
   it('simpleSet (overridden method)', async function(){
@@ -37,4 +39,14 @@ contract("Modifiers", function(accounts) {
       /* ignore */
     }
   });
+
+  it('when false & true branches are hit in immediate succesion by EOA (issue #863)', async function(){
+    try {
+      await D.a({from: accounts[1]});
+    } catch (e) {
+      /* ignore */
+    }
+
+    await D.a({from: accounts[0]});
+  })
 });