add opt-in rbf support for direct sends

bugfix: use enumerate instead of len

reviewed estimation of transaction sizes

estimates are still a bit conservative with room for improvement;
signatures could still save up to one byte each if using low-r
values. python-bitcointx==1.1.2-dev already supports grinding for
low-r values so when it's stable and referenced version is updated,
this should be reviewed again so as to utilize that benefit.

added utility method `estimate_extra_bytes`

the purpose of this method is for the computation
of extra bytes when the coinjoin or direct send output
type is different from that of the wallet

updated tests to reflect new transaction size
computation

p2pkh transactions are now 1 byte larger for
the inputs hence the change amount should be
less 4 * 30 sats.

add private keys for utxos that we may not be
tracking

some transactions (e.g. opt-in rbf) may require signing
with private keys for utxos that we may have stopped
tracking. this commit will search through all inputs
and for those we own and retrieve their private keys
so we can sign with them.

added support for p2wsh output scripts in

refactored the estimation of the transaction size
when outputs of a different type is the target

Previously, a different method was employed which
was kind of kludgy considering the fact that the
`extra_bytes` parameter is really for `OP_RETURN`
outputs. This method modifies the `estimate_tx_size`
method to accept an optional extra parameter called
`outtype` which is used to estimate the correct
transaction size if the target output is different
from that of the wallet.

added missing import

added a note about preserving the order of wallet type constants

Fix bug with timelocked addrs in receive payjoin

Previously there would be a crash if the wallet receiving a payjoin
had a timelocked UTXO.

jmbitcoin/jmbitcoin/secp256k1_transaction.py

@@ -18,6 +18,31 @@ from bitcointx.core.scripteval import (VerifyScript, SCRIPT_VERIFY_WITNESS,
                                        SCRIPT_VERIFY_STRICTENC,
                                        SIGVERSION_WITNESS_V0)
 
+# for each transaction type, different output script pubkeys may result in
+# a difference in the number of bytes accounted for while estimating the
+# transaction size, this variable stores the difference and is factored in
+# when calculating the correct transaction size. For example, for a p2pkh
+# transaction, if one of the outputs is a p2wsh pubkey, then the transaction
+# would need 9 extra bytes to account for the difference in script pubkey
+# sizes
+OUTPUT_EXTRA_BYTES = {
+    'p2pkh': {
+        'p2wpkh': -3,
+        'p2sh-p2wpkh': -2,
+        'p2wsh': 9
+    },
+    'p2wpkh': {
+        'p2pkh': 3,
+        'p2sh-p2wpkh': 1,
+        'p2wsh': 12
+    },
+    'p2sh-p2wpkh': {
+        'p2pkh': 2,
+        'p2wpkh': -1,
+        'p2wsh': 11
+    }
+}
+
 def human_readable_transaction(tx, jsonified=True):
     """ Given a CTransaction object, output a human
     readable json-formatted string (suitable for terminal
@@ -81,7 +106,7 @@ def human_readable_output(txoutput):
         pass # non standard script
     return outdict
 
-def estimate_tx_size(ins, outs, txtype='p2pkh'):
+def estimate_tx_size(ins, outs, txtype='p2pkh', outtype=None):
     '''Estimate transaction size.
     The txtype field as detailed below is used to distinguish
     the type, but there is at least one source of meaningful roughness:
@@ -91,42 +116,55 @@ def estimate_tx_size(ins, outs, txtype='p2pkh'):
     say, 10% inaccuracy here.
 
     Assuming p2pkh:
-    out: 8+1+3+2+20=34, in: 1+32+4+1+1+~73+1+1+33=147,
-    ver:4,seq:4, +2 (len in,out)
-    total ~= 34*len_out + 147*len_in + 10 (sig sizes vary slightly)
+    out: 8+1+3+20+2=34, in: 32+4+1+1+~72+1+33+4=148,
+    ver: 4, locktime:4, +2 (len in,out)
+    total = 34*len_out + 148*len_in + 10 (sig sizes vary slightly)
+
     Assuming p2sh M of N multisig:
     "ins" must contain M, N so ins= (numins, M, N) (crude assuming all same)
-    74*M + 34*N + 45 per input, so total ins ~ len_ins * (45+74M+34N)
-    so total ~ 34*len_out + (45+74M+34N)*len_in + 10
+    73*M + 34*N + 45 per input, so total ins ~ len_ins * (45+73M+34N)
+    so total ~ 32*len_out + (45+73M+34N)*len_in + 10
+
     Assuming p2sh-p2wpkh:
-    witness are roughly 3+~73+33 for each input
+    witness are roughly 1+1+~72+1+33 for each input
     (txid, vin, 4+20 for witness program encoded as scriptsig, 4 for sequence)
     non-witness input fields are roughly 32+4+4+20+4=64, so total becomes
-    n_in * 64 + 4(ver) + 4(locktime) + n_out*34
+    n_in * 64 + 4(ver) + 2(marker, flag) + 2(n_in, n_out) + 4(locktime) + n_out*32
+
     Assuming p2wpkh native:
     witness as previous case
     non-witness loses the 24 witnessprogram, replaced with 1 zero,
     in the scriptSig, so becomes:
-    n_in * 41 + 4(ver) + 4(locktime) +2 (len in, out) + n_out*34
+    4 + 1 + 1 + (n_in) + (vin) + (n_out) + (vout) + (witness) + (locktime)
+    non-witness: 4(ver) +2 (marker, flag) + n_in*41 + 4(locktime) +2 (len in, out) + n_out*31
+    witness: 1 + 1 + 72 + 1 + 33
     '''
     if txtype == 'p2pkh':
-        return 10 + ins * 147 + 34 * outs
+        return 4 + 4 + 2 + ins*148 + 34*outs + (
+            OUTPUT_EXTRA_BYTES[txtype][outtype]
+            if outtype and outtype in OUTPUT_EXTRA_BYTES[txtype] else 0)
     elif txtype == 'p2sh-p2wpkh':
         #return the estimate for the witness and non-witness
         #portions of the transaction, assuming that all the inputs
         #are of segwit type p2sh-p2wpkh
         # Note as of Jan19: this misses 2 bytes (trivial) for len in, out
         # and also overestimates output size by 2 bytes.
-        witness_estimate = ins*109
-        non_witness_estimate = 4 + 4 + outs*34 + ins*64
+        witness_estimate = ins*108
+        non_witness_estimate = 4 + 4 + 4 + outs*32 + ins*64 + (
+            OUTPUT_EXTRA_BYTES[txtype][outtype]
+            if outtype and outtype in OUTPUT_EXTRA_BYTES[txtype] else 0)
         return (witness_estimate, non_witness_estimate)
     elif txtype == 'p2wpkh':
-        witness_estimate = ins*109
-        non_witness_estimate = 4 + 4 + 2 + outs*31 + ins*41
+        witness_estimate = ins*108
+        non_witness_estimate = 4 + 4 + 4 + outs*31 + ins*41 + (
+            OUTPUT_EXTRA_BYTES[txtype][outtype]
+            if outtype and outtype in OUTPUT_EXTRA_BYTES[txtype] else 0)
         return (witness_estimate, non_witness_estimate)
     elif txtype == 'p2shMofN':
         ins, M, N = ins
-        return 10 + (45 + 74*M + 34*N) * ins + 34 * outs
+        return 4 + 4 + 2 + (45 + 73*M + 34*N)*ins + outs*32 + (
+            OUTPUT_EXTRA_BYTES['p2sh-p2wpkh'][outtype]
+            if outtype and outtype in OUTPUT_EXTRA_BYTES['p2sh-p2wpkh'] else 0)
     else:
         raise NotImplementedError("Transaction size estimation not" +
                                   "yet implemented for type: " + txtype)

jmclient/jmclient/cli_options.py

@@ -520,6 +520,11 @@ def get_sendpayment_parser():
                       'broadcasting the transaction. '
                       'Currently only works with direct '
                       'send (-N 0).')
+    parser.add_option('--rbf',
+                      action='store_true',
+                      dest='rbf',
+                      default=False,
+                      help='enable opt-in rbf')
     parser.add_option('-u',
                       '--custom-change',
                       type="str",

jmclient/jmclient/cryptoengine.py

@@ -10,10 +10,13 @@ from .configure import get_network, jm_single
 #NOTE: before fidelity bonds and watchonly wallet, each of these types corresponded
 # to one wallet type and one engine, not anymore
 #with fidelity bond wallets and watchonly fidelity bond wallet, the wallet class
-# can have two engines, one for single-sig addresses and the other for timelocked addresses
+# can have two engines, one for single-sig addresses and the other for timelocked addresses.
+# It is also necessary to preserve the order of the wallet types when making modifications
+# as they are mapped to a different Engine when using wallets. Failure to do this would
+# make existing wallets unsable.
 TYPE_P2PKH, TYPE_P2SH_P2WPKH, TYPE_P2WPKH, TYPE_P2SH_M_N, TYPE_TIMELOCK_P2WSH, \
     TYPE_SEGWIT_WALLET_FIDELITY_BONDS, TYPE_WATCHONLY_FIDELITY_BONDS, \
-    TYPE_WATCHONLY_TIMELOCK_P2WSH, TYPE_WATCHONLY_P2WPKH = range(9)
+    TYPE_WATCHONLY_TIMELOCK_P2WSH, TYPE_WATCHONLY_P2WPKH, TYPE_P2WSH = range(10)
 NET_MAINNET, NET_TESTNET, NET_SIGNET = range(3)
 NET_MAP = {'mainnet': NET_MAINNET, 'testnet': NET_TESTNET,
     'signet': NET_SIGNET}
@@ -42,6 +45,8 @@ def detect_script_type(script_str):
         return TYPE_P2SH_P2WPKH
     elif script.is_witness_v0_keyhash():
         return TYPE_P2WPKH
+    elif script.is_witness_v0_scripthash():
+        return TYPE_P2WSH
     raise EngineError("Unknown script type for script '{}'"
                       .format(bintohex(script_str)))
 

jmclient/jmclient/taker.py

@@ -328,7 +328,9 @@ class Taker(object):
             #find sufficient utxos extremely rare. Indeed, a doubling of 'normal'
             #txfee indicates undesirable behaviour on maker side anyway.
             self.total_txfee = estimate_tx_fee(3, 2,
-                txtype=self.wallet_service.get_txtype()) * self.n_counterparties
+                txtype=self.wallet_service.get_txtype()) * (self.n_counterparties - 1) + \
+                estimate_tx_fee(3, 2, txtype=self.wallet_service.get_txtype(),
+                                outtype=self.wallet_service.get_outtype(self.coinjoin_address()))
             total_amount = self.cjamount + self.total_cj_fee + self.total_txfee
             jlog.info('total estimated amount spent = ' + btc.amount_to_str(total_amount))
             try:
@@ -351,8 +353,9 @@ class Taker(object):
             jlog.debug("Estimated ins: "+str(est_ins))
             est_outs = 2*self.n_counterparties + 1
             jlog.debug("Estimated outs: "+str(est_outs))
-            self.total_txfee = estimate_tx_fee(est_ins, est_outs,
-                                            txtype=self.wallet_service.get_txtype())
+            self.total_txfee = estimate_tx_fee(
+                est_ins, est_outs, txtype=self.wallet_service.get_txtype(),
+                outtype=self.wallet_service.get_outtype(self.coinjoin_address()))
             jlog.debug("We have a fee estimate: "+str(self.total_txfee))
             total_value = sum([va['value'] for va in self.input_utxos.values()])
             if self.wallet_service.get_txtype() == "p2pkh":
@@ -439,7 +442,8 @@ class Taker(object):
             #Estimate fee per choice of next/3/6 blocks targetting.
             estimated_fee = estimate_tx_fee(
                 len(sum(self.utxos.values(), [])), len(self.outputs) + 2,
-                txtype=self.wallet_service.get_txtype())
+                txtype=self.wallet_service.get_txtype(),
+                outtype=self.wallet_service.get_outtype(self.coinjoin_address()))
             jlog.info("Based on initial guess: " +
                       btc.amount_to_str(self.total_txfee) +
                       ", we estimated a miner fee of: " +
@@ -483,7 +487,8 @@ class Taker(object):
             num_ins = len([u for u in sum(self.utxos.values(), [])])
             num_outs = len(self.outputs) + 1
             new_total_fee = estimate_tx_fee(num_ins, num_outs,
-                                    txtype=self.wallet_service.get_txtype())
+                                    txtype=self.wallet_service.get_txtype(),
+                                    outtype=self.wallet_service.get_outtype(self.coinjoin_address()))
             feeratio = new_total_fee/self.total_txfee
             jlog.debug("Ratio of actual to estimated sweep fee: {}".format(
                 feeratio))

jmclient/jmclient/taker_utils.py

@@ -80,6 +80,7 @@ def direct_send(wallet_service, amount, mixdepth, destination, answeryes=False,
             return
 
     txtype = wallet_service.get_txtype()
+    outtype = wallet_service.get_outtype(destination)
     if amount == 0:
         #doing a sweep
         utxos = wallet_service.get_utxos_by_mixdepth()[mixdepth]
@@ -111,15 +112,15 @@ def direct_send(wallet_service, amount, mixdepth, destination, answeryes=False,
                 + "\n\nWARNING: This transaction if broadcasted will PERMANENTLY DESTROY your bitcoins\n"
         else:
             #regular sweep (non-burn)
-            fee_est = estimate_tx_fee(len(utxos), 1, txtype=txtype)
+            fee_est = estimate_tx_fee(len(utxos), 1, txtype=txtype, outtype=outtype)
             outs = [{"address": destination, "value": total_inputs_val - fee_est}]
     else:
         #not doing a sweep; we will have change
         #8 inputs to be conservative
-        initial_fee_est = estimate_tx_fee(8,2, txtype=txtype)
+        initial_fee_est = estimate_tx_fee(8,2, txtype=txtype, outtype=outtype)
         utxos = wallet_service.select_utxos(mixdepth, amount + initial_fee_est)
         if len(utxos) < 8:
-            fee_est = estimate_tx_fee(len(utxos), 2, txtype=txtype)
+            fee_est = estimate_tx_fee(len(utxos), 2, txtype=txtype, outtype=outtype)
         else:
             fee_est = initial_fee_est
         total_inputs_val = sum([va['value'] for u, va in utxos.items()])

jmclient/jmclient/wallet.py

@@ -24,10 +24,10 @@ from .configure import jm_single
 from .blockchaininterface import INF_HEIGHT
 from .support import select_gradual, select_greedy, select_greediest, \
     select, NotEnoughFundsException
-from .cryptoengine import TYPE_P2PKH, TYPE_P2SH_P2WPKH,\
+from .cryptoengine import TYPE_P2PKH, TYPE_P2SH_P2WPKH, TYPE_P2WSH,\
     TYPE_P2WPKH, TYPE_TIMELOCK_P2WSH, TYPE_SEGWIT_WALLET_FIDELITY_BONDS,\
     TYPE_WATCHONLY_FIDELITY_BONDS, TYPE_WATCHONLY_TIMELOCK_P2WSH, TYPE_WATCHONLY_P2WPKH,\
-    ENGINES
+    ENGINES, detect_script_type
 from .support import get_random_bytes
 from . import mn_encode, mn_decode
 import jmbitcoin as btc
@@ -47,7 +47,7 @@ class Mnemonic(MnemonicParent):
     def detect_language(cls, code):
         return "english"
 
-def estimate_tx_fee(ins, outs, txtype='p2pkh', extra_bytes=0):
+def estimate_tx_fee(ins, outs, txtype='p2pkh', outtype=None, extra_bytes=0):
     '''Returns an estimate of the number of satoshis required
     for a transaction with the given number of inputs and outputs,
     based on information from the blockchain interface.
@@ -68,11 +68,11 @@ def estimate_tx_fee(ins, outs, txtype='p2pkh', extra_bytes=0):
             " greater than absurd value " +
             btc.fee_per_kb_to_str(absurd_fee) + ", quitting.")
     if txtype in ['p2pkh', 'p2shMofN']:
-        tx_estimated_bytes = btc.estimate_tx_size(ins, outs, txtype) + extra_bytes
+        tx_estimated_bytes = btc.estimate_tx_size(ins, outs, txtype, outtype) + extra_bytes
         return int((tx_estimated_bytes * fee_per_kb)/Decimal(1000.0))
     elif txtype in ['p2wpkh', 'p2sh-p2wpkh']:
         witness_estimate, non_witness_estimate = btc.estimate_tx_size(
-            ins, outs, txtype)
+            ins, outs, txtype, outtype)
         non_witness_estimate += extra_bytes
         return int(int((
             non_witness_estimate + 0.25*witness_estimate)*fee_per_kb)/Decimal(1000.0))
@@ -412,6 +412,19 @@ class BaseWallet(object):
             return 'p2wpkh'
         assert False
 
+    def get_outtype(self, addr):
+        script_type = detect_script_type(
+            btc.CCoinAddress(addr).to_scriptPubKey())
+        if script_type == TYPE_P2PKH:
+            return 'p2pkh'
+        elif script_type == TYPE_P2WPKH:
+            return 'p2wpkh'
+        elif script_type == TYPE_P2SH_P2WPKH:
+            return 'p2sh-p2wpkh'
+        elif script_type == TYPE_P2WSH:
+            return 'p2wsh'
+        assert False
+
     def sign_tx(self, tx, scripts, **kwargs):
         """
         Add signatures to transaction for inputs referenced by scripts.
@@ -635,7 +648,7 @@ class BaseWallet(object):
         wallet, and returns [(index, CTxIn),..] for each.
         """
         retval = []
-        for i, txin in len(tx.vin):
+        for i, txin in enumerate(tx.vin):
             pub, msg = btc.extract_pubkey_from_witness(tx, i)
             if not pub:
                 # this can certainly occur since other inputs
@@ -1292,6 +1305,26 @@ class PSBTWalletMixin(object):
                     #key is ((privkey, locktime), engine) for timelocked addrs
                     key = (key[0][0], key[1])
                 privkeys.append(key)
+
+        # in the rare situation that we want to sign a psbt using private keys
+        # to utxos that we've stopped tracking, let's also find inputs that
+        # belong to us and add those private keys as well
+        for vin in new_psbt.inputs:
+            try:
+                path = self.script_to_path(vin.utxo.scriptPubKey)
+                key = self._get_key_from_path(path)
+                if key not in privkeys:
+                    privkeys.append(key)
+            except AssertionError:
+                # we can safely assume that an exception means we do not
+                # have the ability to sign for this input
+                continue
+            except AttributeError:
+                # shouldn't happen for properly constructed psbts
+                # however, psbts with no utxo information will raise
+                # an AttributeError exception. we simply ignore it.
+                continue
+
         jmckeys = list(btc.JMCKey(x[0][:-1]) for x in privkeys)
         new_keystore = btc.KeyStore.from_iterable(jmckeys)
 

jmclient/test/test_taker.py

@@ -511,17 +511,17 @@ def test_custom_change(setup_taker):
         # the address we intended with the right amount:
         custom_change_found = False
         for out in taker.latest_tx.vout:
-            # input utxo is 20M; amount is 2M; as per logs:
+            # input utxo is 200M; amount is 20M; as per logs:
             # totalin=200000000
-            # my_txfee=12930
+            # my_txfee=13050
             # makers_txfee=3000
-            # cjfee_total=12000 => changevalue=179975070
+            # cjfee_total=12000 => changevalue=179974950
             # note that there is a small variation in the size of
             # the transaction (a few bytes) for the different scriptPubKey
             # type, but this is currently ignored by the Taker, who makes
             # fee estimate purely based on the number of ins and outs;
             # this will never be too far off anyway.
-            if out.scriptPubKey == script and out.nValue == 179975070:
+            if out.scriptPubKey == script and out.nValue == 179974950:
                 # must be only one
                 assert not custom_change_found
                 custom_change_found = True

scripts/sendpayment.py

@@ -243,7 +243,7 @@ def main():
     if options.makercount == 0 and not bip78url:
         tx = direct_send(wallet_service, amount, mixdepth, destaddr,
                          options.answeryes, with_final_psbt=options.with_psbt,
-                         custom_change_addr=custom_change)
+                         optin_rbf=options.rbf, custom_change_addr=custom_change)
         if options.with_psbt:
             log.info("This PSBT is fully signed and can be sent externally for "
                      "broadcasting:")