Merge pull request #419 from status-im/fix-399/same-contract-deployed

fix #399: track each internal transaction and associated nonce

src/clj/commiteth/bounties.clj

@@ -25,7 +25,8 @@
     (log/errorf "issue %s: Unable to deploy bounty contract because repo owner has no Ethereum addres" issue-id)
     (try
       (log/infof "issue %s: Deploying contract to %s" issue-id owner-address)
-      (if-let [transaction-hash (multisig/deploy-multisig owner-address)]
+      (if-let [transaction-hash (multisig/deploy-multisig {:owner owner-address
+                                                           :internal-tx-id (str "contract-github-issue-" issue-id)})]
         (do
           (log/infof "issue %s: Contract deployed, transaction-hash: %s" issue-id transaction-hash)
           (let [resp (github/post-deploying-comment owner

src/clj/commiteth/eth/core.clj

@@ -28,7 +28,9 @@
 (defn auto-gas-price? [] (env :auto-gas-price false))
 (defn offline-signing? [] (env :offline-signing true))
 
-(def web3j-obj (atom nil))
+(def web3j-obj
+  (delay (Web3j/build (HttpService. (eth-rpc-url)))))
+
 (def creds-obj (atom nil))
 
 (defn wallet-file-path []
@@ -37,10 +39,6 @@
 (defn wallet-password []
   (env :eth-password))
 
-(defn create-web3j []
- (or @web3j-obj
-      (swap! web3j-obj (constantly (Web3j/build (HttpService. (eth-rpc-url)))))))
-
 (defn creds []
   (or @creds-obj
       (let [password  (wallet-password)
@@ -53,38 +51,59 @@
           (throw (ex-info "Make sure you provided proper credentials in appropriate resources/config.edn"
                           {:password password :file-path file-path}))))))
 
-(defn get-nonce []
+(defn get-web3j-nonce [web3j-instance]
-  (let [current-nonce (atom nil)]
+  (.. (.ethGetTransactionCount web3j-instance (env :eth-account) DefaultBlockParameterName/LATEST)
-    (fn []
+      sendAsync
-      (let [nonce (.. (.ethGetTransactionCount (create-web3j) 
+      get
-                                               (env :eth-account) 
+      getTransactionCount))
-                                               DefaultBlockParameterName/LATEST)
-                      sendAsync
-                      get
-                      getTransactionCount)]
-        (if (= nonce @current-nonce)
-          (throw (Exception. (str "Attempting to create transaction with the same nonce: " nonce)))
-          (swap! current-nonce (constantly nonce)))
-        nonce))))
-(def get-nonce-fn (get-nonce))
 
-(defn get-signed-tx [gas-price gas-limit to data]
+(defprotocol INonceTracker
-  "Create a sign a raw transaction.
+  "The reason we need this is that we might send mutliple identical
-   'From' argument is not needed as it's already
+  transactions (e.g. bounty contract deploy with same owner) shortly
-   encoded in credentials.
+  after another. In this case web3j's TX counting only increases once
+  a transaction has been confirmed and so we send multiple identical
+  transactions with the same nonce.
+
+  Web3j also provides a TransactionManager that increases nonces but it
+  does not track nonces related to transactions and so as far as I understand
+  it might cause transactions to be executed twice if they are retried.
+
+  https://github.com/web3j/web3j/blob/d19855475aa6620a7e93523bd9ede26ca50ed042/core/src/main/java/org/web3j/tx/RawTransactionManager.java"
+  (get-nonce [this internal-tx-id]
+    "Return the to be used nonce for an OpenBounty Ethereum
+    transaction identified by `internal-tx-id`. As these IDs are stable
+    we can use them to use consistent nonces for the same transaction."))
+
+(defrecord NonceTracker [state]
+  INonceTracker
+  (get-nonce [this internal-tx-id]
+    (let [prev-nonce  (get @state internal-tx-id)
+          web3j-nonce (get-web3j-nonce @web3j-obj)
+          nonces      (set (vals @state))
+          nonce       (if (seq nonces)
+                        (inc (apply max nonces))
+                        web3j-nonce)]
+      (when prev-nonce
+        (log/warnf "%s: tx will be retried (prev-nonce: %s, new-nonce: %s, web3j-nonce: %s)"
+                   internal-tx-id prev-nonce nonce web3j-nonce))
+      ;; TODO this is a memory leak since tracking state is never pruned
+      ;; Since we're not doing 1000s of transactions every day yet we can
+      ;; probably defer worrying about this until a bit later
+      (swap! state assoc internal-tx-id nonce)
+      nonce)))
+
+(def nonce-tracker
+  (->NonceTracker (atom {})))
+
+(defn get-signed-tx [{:keys [gas-price gas-limit to data internal-tx-id]}]
+  "Create a sign a raw transaction. 'From' argument is not needed as it's already encoded in credentials.
    See https://web3j.readthedocs.io/en/latest/transactions.html#offline-transaction-signing"
-  (let [nonce (get-nonce-fn)
+  (let [nonce (get-nonce nonce-tracker internal-tx-id)]
-        tx (RawTransaction/createTransaction
+    (log/infof "%s: Signing nonce: %s, gas-price: %s, gas-limit: %s"
-             nonce
+               internal-tx-id nonce gas-price gas-limit)
-             gas-price
+    (-> (RawTransaction/createTransaction (biginteger nonce) gas-price gas-limit to data)
-             gas-limit
+        (TransactionEncoder/signMessage (creds))
-             to
+        (Numeric/toHexString))))
-             data)
-        signed (TransactionEncoder/signMessage tx (creds))
-        hex-string (Numeric/toHexString signed)]
-    (log/infof "Signing TX: nonce: %s, gas-price: %s, gas-limit: %s, data: %s"
-               nonce gas-price gas-limit data)
-    hex-string))
 
 (defn eth-gasstation-gas-price
   "Get max of average and average_calc from gas station and use that
@@ -139,7 +158,8 @@
   (atom 0))
 
 (defn eth-rpc
-  [method params]
+  [{:keys [method params internal-tx-id]}]
+  {:pre [(string? method) (some? params)]}
   (let [request-id (swap! req-id-tracker inc)
         body       {:jsonrpc "2.0"
                     :method  method
@@ -149,9 +169,10 @@
                   :body (json/write-str body)}
         response  @(post (eth-rpc-url) options)
         result   (safe-read-str (:body response))]
-    (log/infof "eth-rpc req(%s) body: %s\neth-rpc req(%s) result: %s"
+    (when internal-tx-id
-               request-id body request-id result)
+      (log/infof "%s: eth-rpc %s" internal-tx-id method))
-
+    (log/debugf "%s: eth-rpc req(%s) body: %s" internal-tx-id request-id body)
+    (log/debugf "%s: eth-rpc req(%s) result: %s" internal-tx-id request-id result)
     (cond
       ;; Ignore any responses that have mismatching request ID
       (not= (:id result) request-id)
@@ -159,7 +180,10 @@
 
       ;; If request ID matches but contains error, throw
       (:error result)
-      (throw (ex-info "Error submitting transaction via eth-rpc" (:error result)))
+      (throw
+       (ex-info (format "%s: Error submitting transaction via eth-rpc %s"
+                        (or internal-tx-id "(no-tx-id)") (:error result))
+                (:error result)))
 
       :else
       (:result result))))
@@ -196,9 +220,10 @@
 (defn estimate-gas
   [from to value & [params]]
   (let [geth-estimate (eth-rpc
-                       "eth_estimateGas" [(merge params {:from  from
+                       {:method "eth_estimateGas"
-                                                         :to    to
+                        :params [(merge params {:from  from
-                                                         :value value})])
+                                                :to    to
+                                                :value value})]})
         adjusted-gas (adjust-gas-estimate geth-estimate)]
 
     (log/debug "estimated gas (geth):" geth-estimate)
@@ -221,7 +246,8 @@
 
 (defn get-balance-hex
   [account]
-  (eth-rpc "eth_getBalance" [account "latest"]))
+  (eth-rpc {:method "eth_getBalance"
+            :params [account "latest"]}))
 
 (defn get-balance-wei
   [account]
@@ -240,7 +266,8 @@
 
 (defn get-transaction-receipt
   [hash]
-  (eth-rpc "eth_getTransactionReceipt" [hash]))
+  (eth-rpc {:method "eth_getTransactionReceipt"
+            :params [hash]}))
 
 (defn format-call-params
   [method-id & params]
@@ -250,10 +277,12 @@
 (defn call
   [contract method-id & params]
   (let [data (apply format-call-params method-id params)]
-    (eth-rpc "eth_call" [{:to contract :data data} "latest"])))
+    (eth-rpc {:method "eth_call"
+              :params [{:to contract :data data} "latest"]})))
 
 (defn execute
-  [from contract method-id gas-limit & params]
+  [{:keys [from contract method-id gas-limit params internal-tx-id]}]
+  {:pre [(string? method-id)]}
   (let [data (apply format-call-params method-id params)
         gas-price (gas-price)
         value (format "0x%x" 0)
@@ -264,21 +293,23 @@
                  (merge {:gasPrice (integer->hex gas-price)})
                  contract
                  (merge {:to contract}))
-        gas (if gas-limit gas-limit 
+        gas    (or gas-limit (estimate-gas from contract value params))
-              (estimate-gas from contract value params))
         params (if (offline-signing?)
-                 (get-signed-tx (biginteger gas-price)
+                 (get-signed-tx {:internal-tx-id internal-tx-id
-                                      (hex->big-integer gas)
+                                 :gas-price (biginteger gas-price)
-                                      contract
+                                 :gas-limit (hex->big-integer gas)
-                                      data) 
+                                 :to        contract
+                                 :data      data})
                  (assoc params :gas gas))]
     (if (offline-signing?)
       (eth-rpc
-        "eth_sendRawTransaction"
+       {:method "eth_sendRawTransaction"
-        [params])
+        :params [params]
+        :internal-tx-id internal-tx-id})
       (eth-rpc
-        "personal_sendTransaction"
+       {:method "personal_sendTransaction"
-        [params (eth-password)]))))
+        :params [params (eth-password)]
+        :internal-tx-id internal-tx-id}))))
 
 (defn hex-ch->num
   [ch]
@@ -339,7 +370,6 @@
   eth-core
   :start
   (do
-    (swap! web3j-obj (constantly nil))
     (swap! creds-obj (constantly nil))
     (log/info "eth/core started"))
   :stop

src/clj/commiteth/eth/multisig_wallet.clj

@@ -1,6 +1,5 @@
 (ns commiteth.eth.multisig-wallet
-  (:require [commiteth.eth.core :as eth
+  (:require [commiteth.eth.core :as eth]
-             :refer [create-web3j creds]]
             [commiteth.config :refer [env]]
             [clojure.tools.logging :as log]
             [taoensso.tufte :as tufte :refer (defnp p profiled profile)]
@@ -36,24 +35,19 @@
   []
   (env :tokenreg-base-format :status))
 
-(defn create-new
-  [owner1 owner2 required]
-  (eth/execute (eth/eth-account)
-               (factory-contract-addr)
-               (:create method-ids)
-               (eth/integer->hex 865000) ;; gas-limit
-               0x40
-               0x2
-               required
-               owner1
-               owner2))
-
-
 (defn deploy-multisig
   "Deploy a new multisig contract to the blockchain with commiteth bot
-  and given owner as owners."
+  and given owner as owners.
-  [owner]
+  `internal-tx-id` is used to identify what issue this multisig is deployed
-  (create-new (eth/eth-account) owner 2))
+  for and manage nonces at a later point in time."
+  [{:keys [owner internal-tx-id]}]
+  {:pre [(string? owner) (string? internal-tx-id)]}
+  (eth/execute {:internal-tx-id internal-tx-id
+                :from      (eth/eth-account)
+                :contract  (factory-contract-addr)
+                :method-id (:create method-ids)
+                :gas-limit (eth/integer->hex 865000)
+                :params    [0x40 0x2 2 (eth/eth-account) owner]}))
 
 (defn find-event-in-tx-receipt [tx-receipt topic-id]
   (let [logs (:logs tx-receipt)
@@ -92,20 +86,18 @@
 
 
 (defn send-all
-  [contract to]
+  [{:keys [contract payout-address internal-tx-id]}]
-  (log/debug "multisig/send-all " contract to)
+  {:pre [(string? contract) (string? payout-address) (string? internal-tx-id)]}
+  (log/debug "multisig/send-all " contract payout-address internal-tx-id)
   (let [params (eth/format-call-params
                 (:withdraw-everything method-ids)
-                to)]
+                payout-address)]
-    (eth/execute (eth/eth-account)
+    (eth/execute {:internal-tx-id internal-tx-id
-                 contract
+                  :from      (eth/eth-account)
-                 (:submit-transaction method-ids)
+                  :contract  contract
-                 nil
+                  :method-id (:submit-transaction method-ids)
-                 contract
+                  :gas-limit nil
-                 0
+                  :params    [contract 0 "0x60" "0x24" params]})))
-                 "0x60" ;; TODO: document these
-                 "0x24" ;;  or refactor out
-                 params)))
 
 
 (defn get-token-address [token]
@@ -118,17 +110,17 @@
   (log/debug "multisig/watch-token" bounty-addr token)
   (let [token-address (get-token-address token)]
     (assert token-address)
-    (eth/execute (eth/eth-account)
+    (eth/execute {:internal-tx-id (str "watch-token-" (System/currentTimeMillis) "-" bounty-addr)
-                 bounty-addr
+                  :from      (eth/eth-account)
-                 (:watch method-ids)
+                  :contract  bounty-addr
-                 nil
+                  :method-id (:watch method-ids)
-                 token-address)))
+                  :gas-limit nil
-
+                  :params    [token-address]})))
 
 (defn load-bounty-contract [addr]
   (MultiSigTokenWallet/load addr
-    (create-web3j)
+    @eth/web3j-obj
-    (creds)
+    (eth/creds)
     (eth/gas-price)
     (BigInteger/valueOf 500000)))
 

src/clj/commiteth/eth/token_registry.clj

@@ -1,6 +1,5 @@
 (ns commiteth.eth.token-registry
-  (:require [commiteth.eth.core :as eth
+  (:require [commiteth.eth.core :as eth]
-             :refer [create-web3j creds]]
             [commiteth.config :refer [env]]
             [clojure.tools.logging :as log])
   (:import [org.web3j
@@ -23,8 +22,8 @@
 
 (defn- load-tokenreg-contract [addr]
   (TokenReg/load addr
-                 (create-web3j)
+                 @eth/web3j-obj
-                 (creds)
+                 (eth/creds)
                  (eth/gas-price)
                  (BigInteger/valueOf 21000)))
 
@@ -59,9 +58,8 @@
 (defn deploy-parity-tokenreg
   "Deploy an instance of parity token-registry to current network"
   []
-  (let [web3j (create-web3j)]
+  (TokenReg/deploy @eth/web3j-obj
-    (TokenReg/deploy web3j
+                   (eth/creds)
-                     (creds)
+                   (eth/gas-price)
-                     (eth/gas-price)
+                   (BigInteger/valueOf 4000000) ;; gas limit
-                     (BigInteger/valueOf 4000000) ;; gas limit
+                   BigInteger/ZERO))
-                     BigInteger/ZERO)))

src/clj/commiteth/scheduler.clj

@@ -126,7 +126,9 @@
                                                    tokens
                                                    winner-login
                                                    true))
-             (let [execute-hash (multisig/send-all contract-address payout-address)]
+             (let [execute-hash (multisig/send-all {:contract contract-address
+                                                    :payout-address payout-address
+                                                    :internal-tx-id (str "payout-github-issue-" issue-id)})]
                (log/infof "issue %s: Payout self-signed, called sign-all(%s) tx: %s" issue-id contract-address payout-address execute-hash)
                (db-bounties/update-execute-hash issue-id execute-hash)
                (db-bounties/update-winner-login issue-id winner-login)