From e61e523f1e66a3c3e4276e4f3589d81a87b8ce27 Mon Sep 17 00:00:00 2001 From: AdamISZ Date: Fri, 13 Jul 2018 17:57:11 +0200 Subject: [PATCH] add more detailed comments to maker and daemon code --- jmclient/jmclient/maker.py | 37 +++++++++++++++++++ jmdaemon/jmdaemon/daemon_protocol.py | 55 +++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/jmclient/jmclient/maker.py b/jmclient/jmclient/maker.py index 9ef1436..0ae3117 100644 --- a/jmclient/jmclient/maker.py +++ b/jmclient/jmclient/maker.py @@ -29,6 +29,12 @@ class Maker(object): self.aborted = False def try_to_create_my_orders(self): + """Because wallet syncing is not synchronous(!), + we cannot calculate our offers until we know the wallet + contents, so poll until BlockchainInterface.wallet_synced + is flagged as True. TODO: Use a deferred, probably. + Note that create_my_orders() is defined by subclasses. + """ if not jm_single().bc_interface.wallet_synced: return self.offerlist = self.create_my_orders() @@ -91,6 +97,11 @@ class Maker(object): return (True, utxos, auth_pub, cj_addr, change_addr, btc_sig) def on_tx_received(self, nick, txhex, offerinfo): + """Called when the counterparty has sent an unsigned + transaction. Sigs are created and returned if and only + if the transaction passes verification checks (see + verify_unsigned_tx()). + """ try: tx = btc.deserialize(txhex) except IndexError as e: @@ -123,6 +134,13 @@ class Maker(object): return (True, sigs) def verify_unsigned_tx(self, txd, offerinfo): + """This code is security-critical. + Before signing the transaction the Maker must ensure + that all details are as expected, and most importantly + that it receives the exact number of coins to expected + in total. The data is taken from the offerinfo dict and + compared with the serialized txhex. + """ tx_utxo_set = set(ins['outpoint']['hash'] + ':' + str( ins['outpoint']['index']) for ins in txd['ins']) @@ -131,6 +149,8 @@ class Maker(object): cjaddr_script = btc.address_to_script(cjaddr) changeaddr = offerinfo["changeaddr"] changeaddr_script = btc.address_to_script(changeaddr) + #Note: this value is under the control of the Taker, + #see comment below. amount = offerinfo["amount"] cjfee = offerinfo["offer"]["cjfee"] txfee = offerinfo["offer"]["txfee"] @@ -139,12 +159,25 @@ class Maker(object): if not tx_utxo_set.issuperset(my_utxo_set): return (False, 'my utxos are not contained') + #The three lines below ensure that the Maker receives + #back what he puts in, minus his bitcointxfee contribution, + #plus his expected fee. These values are fully under + #Maker control so no combination of messages from the Taker + #can change them. + #(mathematically: amount + expected_change_value is independent + #of amount); there is not a (known) way for an attacker to + #alter the amount (note: !fill resubmissions *overwrite* + #the active_orders[dict] entry in daemon), but this is an + #extra layer of safety. my_total_in = sum([va['value'] for va in utxos.values()]) real_cjfee = calc_cj_fee(ordertype, cjfee, amount) expected_change_value = (my_total_in - amount - txfee + real_cjfee) jlog.info('potentially earned = {}'.format(real_cjfee - txfee)) jlog.info('mycjaddr, mychange = {}, {}'.format(cjaddr, changeaddr)) + #The remaining checks are needed to ensure + #that the coinjoin and change addresses occur + #exactly once with the required amts, in the output. times_seen_cj_addr = 0 times_seen_change_addr = 0 for outs in txd['outs']: @@ -164,6 +197,10 @@ class Maker(object): return (True, None) def modify_orders(self, to_cancel, to_announce): + """This code is called on unconfirm and confirm callbacks, + and replaces existing orders with new ones, or just cancels + old ones. + """ jlog.info('modifying orders. to_cancel={}\nto_announce={}'.format( to_cancel, to_announce)) for oid in to_cancel: diff --git a/jmdaemon/jmdaemon/daemon_protocol.py b/jmdaemon/jmdaemon/daemon_protocol.py index 6451559..0c48ade 100644 --- a/jmdaemon/jmdaemon/daemon_protocol.py +++ b/jmdaemon/jmdaemon/daemon_protocol.py @@ -227,6 +227,9 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @JMFill.responder def on_JM_FILL(self, amount, commitment, revelation, filled_offers): + """Takes the necessary data from the Taker and initiates the Stage 1 + interaction with the Makers. + """ if not (self.jm_state == 1 and isinstance(amount, int) and amount >=0): return {'accepted': False} self.cjamount = amount @@ -245,6 +248,9 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @JMMakeTx.responder def on_JM_MAKE_TX(self, nick_list, txhex): + """Taker sends the prepared unsigned transaction + to all the Makers in nick_list + """ if not self.jm_state == 4: log.msg("Make tx was called in wrong state, rejecting") return {'accepted': False} @@ -262,6 +268,10 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @JMAnnounceOffers.responder def on_JM_ANNOUNCE_OFFERS(self, to_announce, to_cancel, offerlist): + """Called by Maker to reset his current offerlist; + Daemon decides what messages (cancel, announce) to + send to the message channel. + """ if self.role != "MAKER": return to_announce = json.loads(to_announce) @@ -275,6 +285,10 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @JMIOAuth.responder def on_JM_IOAUTH(self, nick, utxolist, pubkey, cjaddr, changeaddr, pubkeysig): + """Daemon constructs full !ioauth message to be sent on message + channel based on data from Maker. Relevant data (utxos, addresses) + are stored in the active_orders dict keyed by the nick of the Taker. + """ nick, utxolist, pubkey, cjaddr, changeaddr, pubkeysig = [_byteify( x) for x in nick, utxolist, pubkey, cjaddr, changeaddr, pubkeysig] if not self.role == "MAKER": @@ -301,6 +315,11 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @JMTXSigs.responder def on_JM_TX_SIGS(self, nick, sigs): + """Signatures that the Maker has produced + are passed here to the daemon as a list and + broadcast one by one. TODO: could shorten this, + have more than one sig per message. + """ sigs = _byteify(json.loads(sigs)) for sig in sigs: self.mcc.prepare_privmsg(nick, "sig", sig) @@ -323,7 +342,17 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @maker_only def on_order_fill(self, nick, oid, amount, taker_pk, commit): - """Handled locally in daemon. + """Handled locally in daemon. This is the start of + communication with the Taker. Does the following: + + * Immediately rejects if commitment is invalid or already used. + * Checks that the fill is against a valid offer. + * Establishes encryption with a new ephemeral keypair + * Creates the amount, commitment and keypair fields in + active_orders[nick] (or resets if already existing). + + Processing will only return to the Maker once the conversation + up to !ioauth is complete. """ if nick in self.active_orders: log.msg("Restarting transaction for nick: " + nick) @@ -353,6 +382,10 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): except NaclError as e: log.msg("Unable to set up cryptobox with counterparty: " + repr(e)) self.mcc.send_error(nick, "Invalid nacl pubkey: " + taker_pk) + return + #Note this sets the *whole* dict, old entries (e.g. changeaddr) + #are removed, so we can't have a conflict between old and new + #versions of active_orders[nick] self.active_orders[nick] = {"crypto_box": crypto_box, "kp": kp, "offer": offer, @@ -362,6 +395,11 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @maker_only def on_seen_auth(self, nick, commitment_revelation): + """Passes to Maker the !auth message from the Taker, + for processing. This will include validating the PoDLE + commitment revelation against the existing commitment, + which was already stored in active_orders[nick]. + """ if not nick in self.active_orders: return ao =self.active_orders[nick] @@ -406,10 +444,15 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): @maker_only def on_push_tx(self, nick, txhex): + """Not yet implemented; ignore rather than raise. + """ log.msg('received pushtx message, ignoring, TODO') @maker_only def on_seen_tx(self, nick, txhex): + """Passes the txhex to the Maker for verification + and signing. Note the security checks occur in Maker. + """ if nick not in self.active_orders: return #we send a copy of the entire "active_orders" entry except the cryptobox, @@ -535,6 +578,16 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): self.mcc.prepare_privmsg(counterparty, 'hp2', commit) def respondToIoauths(self, accepted): + """Sends the full set of data from the Makers to the + Taker after processing of first stage is completed, + using the JMFillResponse command. But if the responses + were not accepted (including, not sufficient number + of responses), we send the list of Makers who did not + respond to the Taker, instead of the ioauth data, + so that the Taker can keep track of non-responders + (although note this code is not yet quite ideal, see + comments below). + """ if self.jm_state != 2: #this can be called a second time on timeout, in which case we #do nothing