Repository: ofek/bit

Issue:
Bug with send(combine=False)
I think there's some weird behavior here.

https://blockchain.info/tx/6519c1977bf0863aed1b70caeaeb521c32fab43e5fd663c310f09cdee63d0ad6?show_adv=true

The fee was super high but I think that was a sideeffect of the main bug.

Discussion:
teran-mckinney: Thank you for reviewing!

Could you consider making a new version and pushing it to pip if this looks good to you now?

Patch:
diff --git a/.gitignore b/.gitignore
index 9367500..ba62003 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,6 @@
 *.log
 *.pyc
+*.orig
 /.cache
 /.idea
 /.coverage
diff --git a/bit/network/fees.py b/bit/network/fees.py
index 58a783a..4204400 100644
--- a/bit/network/fees.py
+++ b/bit/network/fees.py
@@ -1,3 +1,4 @@
+import logging
 from functools import wraps
 from time import time
 
@@ -54,7 +55,12 @@ def get_fee_local_cache(f):
                     cached_fee_fast = request.json()['fastestFee']
                     fast_last_update = now
                 except (ConnectionError, HTTPError, Timeout):  # pragma: no cover
-                    return cached_fee_fast or DEFAULT_FEE_FAST
+                    if cached_fee_fast is None:
+                        logging.warning('Connection to fee API failed, returning default fee (fast) of {}'.format(DEFAULT_FEE_FAST))
+                        return DEFAULT_FEE_FAST
+                    else:
+                        logging.warning('Connection to fee API failed, returning cached fee (fast).')
+                        return cached_fee_fast
 
             return cached_fee_fast
 
@@ -71,7 +77,12 @@ def get_fee_local_cache(f):
                     cached_fee_hour = request.json()['hourFee']
                     hour_last_update = now
                 except (ConnectionError, HTTPError, Timeout):  # pragma: no cover
-                    return cached_fee_hour or DEFAULT_FEE_HOUR
+                    if cached_fee_hour is None:
+                        logging.warning('Connection to fee API failed, returning default fee (hour) of {}'.format(DEFAULT_FEE_HOUR))
+                        return DEFAULT_FEE_HOUR
+                    else:
+                        logging.warning('Connection to fee API failed, returning cached fee (hour).')
+                        return cached_fee_hour
 
             return cached_fee_hour
 
diff --git a/bit/transaction.py b/bit/transaction.py
index 7ef723c..4f38bdd 100644
--- a/bit/transaction.py
+++ b/bit/transaction.py
@@ -1,3 +1,4 @@
+import logging
 from collections import namedtuple
 from itertools import islice
 
@@ -70,10 +71,19 @@ def estimate_tx_fee(n_in, n_out, satoshis, compressed):
         + 8
     )
 
-    return estimated_size * satoshis
+    estimated_fee = estimated_size * satoshis
+
+    logging.debug('Estimated fee: {} satoshis for {} bytes'.format(estimated_fee, estimated_size))
+
+    return estimated_fee
 
 
 def sanitize_tx_data(unspents, outputs, fee, leftover, combine=True, message=None, compressed=True):
+    """
+    sanitize_tx_data()
+
+    fee is in satoshis per byte.
+    """
 
     outputs = outputs.copy()
 
@@ -94,12 +104,15 @@ def sanitize_tx_data(unspents, outputs, fee, leftover, combine=True, message=Non
             messages.append((message, 0))
 
     # Include return address in fee estimate.
-    fee = estimate_tx_fee(len(unspents), len(outputs) + len(messages) + 1, fee, compressed)
-    total_out = sum(out[1] for out in outputs) + fee
 
     total_in = 0
+    num_outputs = len(outputs) + len(messages) + 1
+    sum_outputs = sum(out[1] for out in outputs)
 
     if combine:
+        # calculated_fee is in total satoshis.
+        calculated_fee = estimate_tx_fee(len(unspents), num_outputs, fee, compressed)
+        total_out = sum_outputs + calculated_fee
         unspents = unspents.copy()
         total_in += sum(unspent.amount for unspent in unspents)
 
@@ -110,6 +123,8 @@ def sanitize_tx_data(unspents, outputs, fee, leftover, combine=True, message=Non
 
         for index, unspent in enumerate(unspents):
             total_in += unspent.amount
+            calculated_fee = estimate_tx_fee(len(unspents[:index + 1]), num_outputs, fee, compressed)
+            total_out = sum_outputs + calculated_fee
 
             if total_in >= total_out:
                 break


Test patch:
diff --git a/tests/test_transaction.py b/tests/test_transaction.py
index 9f792ec..fc1a881 100644
--- a/tests/test_transaction.py
+++ b/tests/test_transaction.py
@@ -172,6 +172,48 @@ class TestSanitizeTxData:
         assert outputs[1][0] == RETURN_ADDRESS
         assert outputs[1][1] == 1000
 
+    def test_no_combine_remaining_small_inputs(self):
+        unspents_original = [Unspent(1500, 0, '', '', 0),
+                             Unspent(1600, 0, '', '', 0),
+                             Unspent(1700, 0, '', '', 0)]
+        outputs_original = [(RETURN_ADDRESS, 2000, 'satoshi')]
+
+        unspents, outputs = sanitize_tx_data(
+            unspents_original, outputs_original, fee=0, leftover=RETURN_ADDRESS,
+            combine=False, message=None
+        )
+        assert unspents == [Unspent(1500, 0, '', '', 0), Unspent(1600, 0, '', '', 0)]
+        assert len(outputs) == 2
+        assert outputs[1][0] == RETURN_ADDRESS
+        assert outputs[1][1] == 1100
+
+    def test_no_combine_with_fee(self):
+        """
+        Verify that unused unspents do not increase fee.
+        """
+        unspents_single = [Unspent(5000, 0, '', '', 0)]
+        unspents_original = [Unspent(5000, 0, '', '', 0),
+                             Unspent(5000, 0, '', '', 0)]
+        outputs_original = [(RETURN_ADDRESS, 1000, 'satoshi')]
+
+        unspents, outputs = sanitize_tx_data(
+            unspents_original, outputs_original, fee=1, leftover=RETURN_ADDRESS,
+            combine=False, message=None
+        )
+
+        unspents_single, outputs_single = sanitize_tx_data(
+            unspents_single, outputs_original, fee=1, leftover=RETURN_ADDRESS,
+            combine=False, message=None
+        )
+
+        assert unspents == [Unspent(5000, 0, '', '', 0)]
+        assert unspents_single == [Unspent(5000, 0, '', '', 0)]
+        assert len(outputs) == 2
+        assert len(outputs_single) == 2
+        assert outputs[1][0] == RETURN_ADDRESS
+        assert outputs_single[1][0] == RETURN_ADDRESS
+        assert outputs[1][1] == outputs_single[1][1]
+
     def test_no_combine_insufficient_funds(self):
         unspents_original = [Unspent(1000, 0, '', '', 0),
                              Unspent(1000, 0, '', '', 0)]