diff --git a/jmclient/jmclient/configure.py b/jmclient/jmclient/configure.py index c2cb920..8a63ead 100644 --- a/jmclient/jmclient/configure.py +++ b/jmclient/jmclient/configure.py @@ -294,6 +294,35 @@ accept_commitment_broadcasts = 1 #Location of your commitments.json file (stores commitments you've used #and those you want to use in future), relative to the scripts directory. commit_file_location = cmtdata/commitments.json + +[PAYJOIN] +# for the majority of situations, the defaults +# need not be altered - they will ensure you don't pay +# a significantly higher fee. +# MODIFICATION OF THESE SETTINGS IS DISADVISED. + +# Payjoin protocol version; currently only '1' is supported. +payjoin_version = 1 + +# servers can change their destination address by default (0). +# if '1', they cannot. Note that servers can explicitly request +# that this is activated, in which case we respect that choice. +disable_output_substitution = 0 + +# "default" here indicates that we will allow the receiver to +# increase the fee we pay by: +# 1.2 * (our_fee_rate_per_vbyte * vsize_of_our_input_type) +# (see https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#span_idfeeoutputspanFee_output) +# (and 1.2 to give breathing room) +# which indicates we are allowing roughly one extra input's fee. +# If it is instead set to an integer, then that many satoshis are allowed. +# Additionally, note that we will also set the parameter additionafeeoutputindex +# to that of our change output, unless there is none in which case this is disabled. +max_additional_fee_contribution = default + +# this is the minimum satoshis per vbyte we allow in the payjoin +# transaction; note it is decimal, not integer. +min_fee_rate = 1.1 """ #This allows use of the jmclient package with a diff --git a/jmclient/jmclient/payjoin.py b/jmclient/jmclient/payjoin.py index 07bb5dc..9a0ec23 100644 --- a/jmclient/jmclient/payjoin.py +++ b/jmclient/jmclient/payjoin.py @@ -5,7 +5,8 @@ from twisted.web.client import (Agent, readBody, ResponseFailed, from twisted.web.iweb import IPolicyForHTTPS from twisted.internet.ssl import CertificateOptions from twisted.web.http_headers import Headers - +import urllib.parse as urlparse +from urllib.parse import urlencode import json from pprint import pformat from jmbase import BytesProducer @@ -71,7 +72,7 @@ class JMPayjoinManager(object): pj_state = JM_PJ_NONE def __init__(self, wallet_service, mixdepth, destination, - amount, server, output_sub_allowed=True): + amount, server, disable_output_substitution=False): assert isinstance(wallet_service, WalletService) # payjoin is not supported for non-segwit wallets: assert isinstance(wallet_service.wallet, @@ -88,7 +89,7 @@ class JMPayjoinManager(object): assert amount > 0 self.amount = amount self.server = server - self.output_sub_allowed = output_sub_allowed + self.disable_output_substitution = disable_output_substitution self.pj_state = self.JM_PJ_INIT self.payment_tx = None self.initial_psbt = None @@ -97,6 +98,7 @@ class JMPayjoinManager(object): # change is initialized as None # in case there is no change: self.change_out = None + self.change_out_index = None def set_payment_tx_and_psbt(self, in_psbt): assert isinstance(in_psbt, btc.PartiallySignedTransaction) @@ -142,7 +144,7 @@ class JMPayjoinManager(object): # for now: found_payment = 0 assert len(self.payment_tx.vout) in [1, 2] - for out in self.payment_tx.vout: + for i, out in enumerate(self.payment_tx.vout): if out.nValue == self.amount and \ btc.CCoinAddress.from_scriptPubKey( out.scriptPubKey) == self.destination: @@ -151,6 +153,7 @@ class JMPayjoinManager(object): # store this for our balance check # for receiver proposal self.change_out = out + self.change_out_index = i if not found_payment == 1: return False @@ -161,16 +164,29 @@ class JMPayjoinManager(object): business logic of the payjoin. We must check in detail that what the server proposes does not unfairly take money from us, and also conforms to acceptable structure. + See https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#senders-payjoin-proposal-checklist We perform the following checks of the receiver proposal: - 1. Check that there are more inputs (i.e. some were contributed). - 2. Does it contain our inputs, unchanged? - 3. Does it contain our payment output, with amount increased? - 4. Are the other inputs finalized, and of the correct type? - 5. Is the feerate unchanged within tolerance? - 6. Does it contain no xpub information or derivation information? - 7. Are the sequence numbers unchanged (and all the same) for the inputs? - 8. Is the nLockTime and version unchanged? - 9. Is the extra fee we pay in reduced change output less than a doubling? + 1. Does it contain our inputs, unchanged? + 2. if output substitution was disabled: + check that the payment output (same scriptPubKey) has + amount equal to or greater than original tx. + if output substition is not disabled: + no check here (all of index, sPK and amount may be altered) + 3. Are the other inputs (if they exist) finalized, and of the correct type? + 4. Is the absolute fee >= fee of original tx? + 5. Check that the feerate of the transaction is not less than our minfeerate + (after signing - we have the signed version here). + 6. If we have a change output, check that: + - the change output still exists, exactly once + - amount subtracted from self.change_out is less than or equal to + maxadditionalfeecontribution. + - Check that the MAFC is only going to fee: check difference between + new fee and old fee is >= MAFC + We do not need to further check against number of new inputs, since + we already insisted on only paying for one. + 7. Does it contain no xpub information or derivation information? + 8. Are the sequence numbers unchanged (and all the same) for the inputs? + 9. Is the nLockTime and version unchanged? If all the above checks pass we will consider this valid, and cosign. Returns: @@ -181,9 +197,6 @@ class JMPayjoinManager(object): orig_psbt = self.initial_psbt assert isinstance(orig_psbt, btc.PartiallySignedTransaction) # 1 - if len(in_pbst.inputs) <= len(orig_psbt.inputs): - return (False, "Receiver did not contribute inputs to payjoin.") - # 2 ourins = [(i.prevout.hash, i.prevout.n) for i in orig_psbt.unsigned_tx.vin] found = [0] * len(ourins) receiver_input_indices = [] @@ -196,22 +209,24 @@ class JMPayjoinManager(object): if any([f != 1 for f in found]): return (False, "Receiver proposed PSBT does not contain our inputs.") + # 2 + if self.disable_output_substitution: + found_payment = 0 + for out in in_pbst.unsigned_tx.vout: + if btc.CCoinAddress.from_scriptPubKey(out.scriptPubKey) == \ + self.destination and out.nValue >= self.amount: + found_payment += 1 + if found_payment != 1: + return (False, "Our payment output not found exactly once or " + "with wrong amount.") # 3 - found = 0 - for out in in_pbst.unsigned_tx.vout: - if btc.CCoinAddress.from_scriptPubKey(out.scriptPubKey) == \ - self.destination and out.nValue >= self.amount: - found += 1 - if found != 1: - return (False, "Our payment output not found exactly once or " - "with wrong amount.") - # 4 for ind in receiver_input_indices: # check the input is finalized if not self.wallet_service.is_input_finalized(in_pbst.inputs[ind]): return (False, "receiver input is not finalized.") # check the utxo field of the input and see if the # scriptPubKey is of the right type. + # TODO this can be genericized to arbitrary wallets in future. spk = in_pbst.inputs[ind].utxo.scriptPubKey if isinstance(self.wallet_service.wallet, SegwitLegacyWallet): try: @@ -227,7 +242,7 @@ class JMPayjoinManager(object): "Receiver input type does not match ours.") else: assert False - # 5 + # 4, 5 # To get the feerate of the psbt proposed, we use the already-signed # version (so all witnesses filled in) to calculate its size, # then compare that with the fee, and do the same for the @@ -237,49 +252,52 @@ class JMPayjoinManager(object): except ValueError: return (False, "receiver proposed tx has negative fee.") nonpayjoin_tx_fee = self.initial_psbt.get_fee() + if proposed_tx_fee < nonpayjoin_tx_fee: + return (False, "receiver proposed transaction has lower fee.") proposed_tx_size = signed_psbt_for_fees.extract_transaction( ).get_virtual_size() - nonpayjoin_tx_size = self.initial_psbt.extract_transaction( - ).get_virtual_size() proposed_fee_rate = proposed_tx_fee / float(proposed_tx_size) log.debug("proposed fee rate: " + str(proposed_fee_rate)) - nonpayjoin_fee_rate = nonpayjoin_tx_fee / float(nonpayjoin_tx_size) - log.debug("nonpayjoin fee rate: " + str(nonpayjoin_fee_rate)) - diff_rate = abs(proposed_fee_rate - nonpayjoin_fee_rate)/nonpayjoin_fee_rate - if diff_rate > 0.2: - log.error("Bad fee rate differential: " + str(diff_rate)) - return (False, "fee rate of payjoin tx is more than 20% different " - "from inital fee rate, rejecting.") + if proposed_fee_rate < float( + jm_single().config.get("PAYJOIN", "min_fee_rate")): + return (False, "receiver proposed transaction has too low " + "feerate: " + str(proposed_fee_rate)) # 6 + if self.change_out: + found_change = 0 + for out in in_pbst.unsigned_tx.vout: + if out.scriptPubKey == self.change_out.scriptPubKey: + found_change += 1 + actual_contribution = self.change_out.nValue - out.nValue + if actual_contribution > in_pbst.get_fee( + ) - self.initial_psbt.get_fee(): + return (False, "Our change output is reduced more" + " than the fee is bumped.") + mafc = get_max_additional_fee_contribution(self) + if actual_contribution > mafc: + return (False, "Proposed transactions requires " + "us to pay more additional fee that we " + "agreed to: " + str(mafc) + " sats.") + # note this check is only if the initial tx had change: + if found_change != 1: + return (False, "Our change output was not found " + "exactly once.") + # 7 if in_pbst.xpubs: return (False, "Receiver proposal contains xpub information.") - # 7 - # we created all inputs with one sequence number, make sure everything - # agrees - # TODO - discussion with btcpayserver devs, docs will be updated, - # server will agree with client in future. For now disabling check - # (it's a very complicated issue, surprisingly!) - #seqno = self.initial_psbt.unsigned_tx.vin[0].nSequence - #for inp in in_pbst.unsigned_tx.vin: - # if inp.nSequence != seqno: - # return (False, "all sequence numbers are not the same.") # 8 + seqno = self.initial_psbt.unsigned_tx.vin[0].nSequence + for inp in in_pbst.unsigned_tx.vin: + if inp.nSequence != seqno: + return (False, "all sequence numbers are not the same.") + # 9 if in_pbst.unsigned_tx.nLockTime != \ self.initial_psbt.unsigned_tx.nLockTime: return (False, "receiver proposal has altered nLockTime.") if in_pbst.unsigned_tx.nVersion != \ self.initial_psbt.unsigned_tx.nVersion: return (False, "receiver proposal has altered nVersion.") - # 9 - if proposed_tx_fee >= 2 * nonpayjoin_tx_fee: - return (False, "receiver's tx fee is too large (possibly " - "too many extra inputs.") - # as well as the overall fee, check our pay-out specifically: - for out in in_pbst.unsigned_tx.vout: - if out.scriptPubKey == self.change_out.scriptPubKey: - found += 1 - if self.change_out.nValue - out.nValue > nonpayjoin_tx_fee: - return (False, "Our change output was reduced too much.") + # all checks passed return (True, None) def set_payjoin_psbt(self, in_psbt, signed_psbt_for_fees): @@ -367,11 +385,32 @@ def parse_payjoin_setup(bip21_uri, wallet_service, mixdepth): # this will throw for any invalid address: destaddr = btc.CCoinAddress(destaddr) server = decoded["pj"] - os_allowed = True + disable_output_substitution = False if "pjos" in decoded and decoded["pjos"] == "0": - os_allowed = False + disable_output_substitution = True return JMPayjoinManager(wallet_service, mixdepth, destaddr, amount, server, - output_sub_allowed=os_allowed) + disable_output_substitution=disable_output_substitution) + +def get_max_additional_fee_contribution(manager): + """ See definition of maxadditionalfeecontribution in BIP 78. + """ + max_additional_fee_contribution = jm_single( + ).config.get("PAYJOIN", "max_additional_fee_contribution") + if max_additional_fee_contribution == "default": + # calculate the fee bumping allowed according to policy: + if isinstance(manager.wallet_service.wallet, SegwitLegacyWallet): + vsize = 91 + elif isinstance(manager.wallet_service.wallet, SegwitWallet): + vsize = 68 + else: + raise Exception("Payjoin only supported for segwit wallets") + original_fee_rate = manager.initial_psbt.get_fee()/float( + manager.initial_psbt.extract_transaction().get_virtual_size()) + log.debug("Initial nonpayjoin transaction feerate is: " + str(original_fee_rate)) + max_additional_fee_contribution = int(original_fee_rate * 1.2 * vsize) + log.debug("From which we calculated a max additional fee " + "contribution of: " + str(max_additional_fee_contribution)) + return max_additional_fee_contribution def send_payjoin(manager, accept_callback=None, info_callback=None, tls_whitelist=None): @@ -416,13 +455,47 @@ def send_payjoin(manager, accept_callback=None, contextFactory=WhitelistContextFactory(tls_whitelist)) body = BytesProducer(payment_psbt.to_base64().encode("utf-8")) + + #Set the query parameters for the request: + + # construct the URI from the given parameters + pj_version = jm_single().config.getint("PAYJOIN", + "payjoin_version") + params = {"v": pj_version} + + disable_output_substitution = "false" + if manager.disable_output_substitution: + disable_output_substitution = "true" + else: + if jm_single().config.getint("PAYJOIN", + "disable_output_substitution") == 1: + disable_output_substitution = "true" + params["disableoutputsubstitution"] = disable_output_substitution + + # to determine the additionalfeeoutputindex in cases where we have + # change and we are allowing fee bump, we examine the initial tx: + if manager.change_out: + params["additionalfeeoutputindex"] = manager.change_out_index + params["maxadditionalfeecontribution"] = \ + get_max_additional_fee_contribution(manager) + + min_fee_rate = float(jm_single().config.get("PAYJOIN", "min_fee_rate")) + params["minfeerate"] = min_fee_rate + + destination_url = manager.server.encode("utf-8") + url_parts = list(urlparse.urlparse(destination_url)) + print("From destination url: ", destination_url, " got urlparts: ", url_parts) + url_parts[4] = urlencode(params).encode("utf-8") + print("after insertion, url_parts is: ", url_parts) + destination_url = urlparse.urlunparse(url_parts) # TODO what to use as user agent? - d = agent.request(b"POST", manager.server.encode("utf-8"), - Headers({"Content-Type": ['text/plain']}), + d = agent.request(b"POST", destination_url, + Headers({"User-Agent": ["Twisted Web Client Example"], + "Content-Type": ["text/plain"]}), bodyProducer=body) d.addCallback(receive_payjoin_proposal_from_server, manager) - # note that the errback (here "noresponse") is *not* triggered + # note that the errback (here "noResponse") is *not* triggered # by a server rejection (which is accompanied by a non-200 # status code returned), but by failure to communicate. def noResponse(failure): diff --git a/scripts/sendpayment.py b/scripts/sendpayment.py index 79dd521..19b5808 100755 --- a/scripts/sendpayment.py +++ b/scripts/sendpayment.py @@ -66,7 +66,7 @@ def main(): #without schedule file option, use the arguments to create a schedule #of a single transaction sweeping = False - payjoinurl = None + bip78url = None if options.schedule == '': if btc.is_bip21_uri(args[1]): parsed = btc.decode_bip21_uri(args[1]) @@ -78,19 +78,19 @@ def main(): destaddr = parsed['address'] if 'jmnick' in parsed: if "pj" in parsed: - parser.error("Cannot specify both BIP79++ and Joinmarket " + parser.error("Cannot specify both BIP78 and Joinmarket " "peer-to-peer payjoin at the same time!") sys.exit(EXIT_ARGERROR) options.p2ep = parsed['jmnick'] elif "pj" in parsed: # note that this is a URL; its validity # checking is deferred to twisted.web.client.Agent - payjoinurl = parsed["pj"] + bip78url = parsed["pj"] # setting makercount only for fee sanity check. # note we ignore any user setting and enforce N=0, # as this is a flag in the code for a non-JM coinjoin; - # for the fee sanity check, note that BIP79++ currently - # will only allow very small fee changes, so N=0 won't + # for the fee sanity check, note that BIP78 currently + # will only allow small fee changes, so N=0 won't # be very inaccurate. jmprint("Attempting to pay via payjoin.", "info") options.makercount = 0 @@ -211,7 +211,7 @@ def main(): log.info("Estimated miner/tx fees for this coinjoin amount: {:.1%}" .format(exp_tx_fees_ratio)) - if options.makercount == 0 and not options.p2ep and not payjoinurl: + if options.makercount == 0 and not options.p2ep and not bip78url: tx = direct_send(wallet_service, amount, mixdepth, destaddr, options.answeryes, with_final_psbt=options.with_psbt) if options.with_psbt: @@ -322,7 +322,7 @@ def main(): taker = P2EPTaker(options.p2ep, wallet_service, schedule, callbacks=(None, None, p2ep_on_finished_callback)) - elif payjoinurl: + elif bip78url: # TODO sanity check wallet type is segwit manager = parse_payjoin_setup(args[1], wallet_service, options.mixdepth) reactor.callWhenRunning(send_payjoin, manager, tls_whitelist=["127.0.0.1"]) diff --git a/test/payjoinclient.py b/test/payjoinclient.py index 764b8b6..7296985 100644 --- a/test/payjoinclient.py +++ b/test/payjoinclient.py @@ -22,7 +22,7 @@ if __name__ == "__main__": pjurl = "http://127.0.0.1:8080" else: pjurl = "https://127.0.0.1:8080" - bip21uri = "bitcoin:2N7CAdEUjJW9tUHiPhDkmL9ukPtcukJMoxK?amount=0.3&pj=" + pjurl + bip21uri = "bitcoin:2N7CAdEUjJW9tUHiPhDkmL9ukPtcukJMoxK?amount=0.3&pj=" + pjurl wallet_path = get_wallet_path(wallet_name, None) if jm_single().config.get("POLICY", "native") == "true": walletclass = SegwitWallet diff --git a/test/regtest_joinmarket.cfg b/test/regtest_joinmarket.cfg index 4d22331..785ea2c 100644 --- a/test/regtest_joinmarket.cfg +++ b/test/regtest_joinmarket.cfg @@ -62,3 +62,32 @@ minimum_makers = 1 listunspent_args = [0] max_sats_freeze_reuse = -1 +[PAYJOIN] +# for the majority of situations, the defaults +# need not be altered - they will ensure you don't pay +# a significantly higher fee. +# MODIFICATION OF THESE SETTINGS IS DISADVISED. + +# Payjoin protocol version; currently only '1' is supported. +payjoin_version = 1 + +# servers can change their destination address by default (0). +# if '1', they cannot. Note that servers can explicitly request +# that this is activated, in which case we respect that choice. +disable_output_substitution = 0 + +# "default" here indicates that we will allow the receiver to +# increase the fee we pay by: +# 1.2 * (our_fee_rate_per_vbyte * vsize_of_our_input_type) +# (see https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#span_idfeeoutputspanFee_output) +# (and 1.2 to give breathing room) +# which indicates we are allowing roughly one extra input's fee. +# If it is instead set to an integer, then that many satoshis are allowed. +# Additionally, note that we will also set the parameter additionafeeoutputindex +# to that of our change output, unless there is none in which case this is disabled. +max_additional_fee_contribution = default + +# this is the minimum satoshis per vbyte we allow in the payjoin +# transaction; note it is decimal, not integer. +min_fee_rate = 1.1 +