From 3bb6c785ff1b6fdb1477dbbbc7dbe007cf2a01cd Mon Sep 17 00:00:00 2001 From: AdamISZ Date: Sun, 22 Jul 2018 16:13:37 +0200 Subject: [PATCH] Fixes bugs in restart-with-subset including #169; see PR for more --- jmclient/jmclient/client_protocol.py | 16 ++++-- jmclient/jmclient/taker.py | 39 ++++++++++++- jmclient/jmclient/taker_utils.py | 85 +++++++++++++++------------- jmdaemon/jmdaemon/daemon_protocol.py | 3 +- scripts/sendpayment.py | 35 ++++++++---- 5 files changed, 118 insertions(+), 60 deletions(-) diff --git a/jmclient/jmclient/client_protocol.py b/jmclient/jmclient/client_protocol.py index fb771c1..b3b214f 100644 --- a/jmclient/jmclient/client_protocol.py +++ b/jmclient/jmclient/client_protocol.py @@ -370,14 +370,20 @@ class JMTakerClientProtocol(JMClientProtocol): def on_JM_FILL_RESPONSE(self, success, ioauth_data): """Receives the entire set of phase 1 data (principally utxos) from the counterparties and passes through to the Taker for - tx construction, if successful. Then passes back the phase 2 - initiating data to the daemon. + tx construction. If there were sufficient makers, data is passed + over for exactly those makers that responded. If not, the list + of non-responsive makers is added to the permanent "ignored_makers" + list, but the Taker processing is bypassed and the transaction + is abandoned here (so will be picked up as stalled in multi-join + schedules). + In the first of the above two cases, after the Taker processes + the ioauth data and returns the proposed + transaction, passes the phase 2 initiating data to the daemon. """ ioauth_data = json.loads(ioauth_data) if not success: - nonresponders = ioauth_data - jlog.info("Makers didnt respond: " + str(nonresponders)) - self.client.add_ignored_makers(nonresponders) + jlog.info("Makers who didnt respond: " + str(ioauth_data)) + self.client.add_ignored_makers(ioauth_data) return {'accepted': True} else: jlog.info("Makers responded with: " + json.dumps(ioauth_data)) diff --git a/jmclient/jmclient/taker.py b/jmclient/jmclient/taker.py index b7ca872..5439b7f 100644 --- a/jmclient/jmclient/taker.py +++ b/jmclient/jmclient/taker.py @@ -77,11 +77,25 @@ class Taker(object): self.wallet = wallet self.schedule = schedule self.order_chooser = order_chooser + + #List (which persists between transactions) of makers + #who have not responded or behaved maliciously at any + #stage of the protocol. self.ignored_makers = [] if not ignored_makers else ignored_makers + #Used in attempts to complete with subset after second round failure: self.honest_makers = [] #Toggle: if set, only honest makers will be used from orderbook self.honest_only = False + + #Temporary (per transaction) list of makers that keeps track of + #which have responded, both in Stage 1 and Stage 2. Before each + #stage, the list is set to the full set of expected responders, + #and entries are removed when honest responses are received; + #emptiness of the list can be used to trigger completion of + #processing. + self.nonrespondants = [] + self.waiting_for_conf = False self.txid = None self.schedule_index = -1 @@ -104,6 +118,7 @@ class Taker(object): for the duration of the Taker run (so, the whole schedule). """ self.ignored_makers.extend(makers) + self.ignored_makers = list(set(self.ignored_makers)) def add_honest_makers(self, makers): """A maker who has shown willigness to complete the protocol @@ -112,6 +127,7 @@ class Taker(object): offers from thus-defined "honest" makers. """ self.honest_makers.extend(makers) + self.honest_makers = list(set(self.honest_makers)) def set_honest_only(self, truefalse): """Toggle; if set, offers will only be accepted @@ -173,6 +189,7 @@ class Taker(object): self.cjfee_total = 0 self.maker_txfee_contributions = 0 self.txfee_default = 5000 + self.latest_tx = None self.txid = None sweep = True if self.cjamount == 0 else False @@ -198,6 +215,11 @@ class Taker(object): return (False,) else: self.taker_info_callback("INFO", errmsg) + + #Initialization has been successful. We must set the nonrespondants + #now to keep track of what changed when we receive the utxo data + self.nonrespondants = self.orderbook.keys() + return (True, self.cjamount, commitment, revelation, self.orderbook) def filter_orderbook(self, orderbook, sweep=False): @@ -311,8 +333,10 @@ class Taker(object): """ if self.aborted: return (False, "User aborted") + + #Temporary list used to aggregate all ioauth data that must be removed rejected_counterparties = [] - #Enough data, but need to authorize against the btc pubkey first. + #Need to authorize against the btc pubkey first. for nick, nickdata in ioauth_data.iteritems(): utxo_list, auth_pub, cj_addr, change_addr, btc_sig, maker_pk = nickdata if not self.auth_counterparty(btc_sig, auth_pub, maker_pk): @@ -378,8 +402,16 @@ class Taker(object): self.cjfee_total += real_cjfee self.maker_txfee_contributions += self.orderbook[nick]['txfee'] self.maker_utxo_data[nick] = utxo_data + #We have succesfully processed the data from this nick: + try: + self.nonrespondants.remove(nick) + except Exception as e: + jlog.warn("Failure to remove counterparty from nonrespondants list: " + str(nick) + \ + ", error message: " + repr(e)) - #Apply business logic of how many counterparties are enough: + #Apply business logic of how many counterparties are enough; note that + #this must occur after the above ioauth data processing, since we only now + #know for sure that the data meets all business-logic requirements. if len(self.maker_utxo_data.keys()) < jm_single().config.getint( "POLICY", "minimum_makers"): self.taker_info_callback("INFO", "Not enough counterparties, aborting.") @@ -387,6 +419,9 @@ class Taker(object): "Not enough counterparties responded to fill, giving up") self.taker_info_callback("INFO", "Got all parts, enough to build a tx") + + #The list self.nonrespondants is now reset and + #used to track return of signatures for phase 2 self.nonrespondants = list(self.maker_utxo_data.keys()) my_total_in = sum([va['value'] for u, va in self.input_utxos.iteritems() diff --git a/jmclient/jmclient/taker_utils.py b/jmclient/jmclient/taker_utils.py index 82609b9..83b5afe 100644 --- a/jmclient/jmclient/taker_utils.py +++ b/jmclient/jmclient/taker_utils.py @@ -238,52 +238,57 @@ def tumbler_taker_finished_update(taker, schedulefile, tumble_log, options, taker.wallet.add_new_utxos(txd, txid) else: #a transaction failed, either because insufficient makers - #(acording to minimum_makers) responded in Stage 1, or not all - #makers responded in Stage 2. We'll first try to repeat without the + #(acording to minimum_makers) responded in Phase 1, or not all + #makers responded in Phase 2. We'll first try to repeat without the #troublemakers. - #Note that Taker.nonrespondants is always set to the full maker - #list at the start of Taker.receive_utxos, so it is always updated - #to a new list in each tx run. log.info("Schedule entry: " + str( taker.schedule[taker.schedule_index]) + \ " failed after timeout, trying again") taker.add_ignored_makers(taker.nonrespondants) - #Now we have to set the specific group we want to use, and hopefully - #they will respond again as they showed honesty last time. - taker.add_honest_makers(list(set( - taker.maker_utxo_data.keys()).symmetric_difference( - set(taker.nonrespondants)))) - #If no makers were honest, we can only tweak the schedule. - #If some were, we prefer the restart with them only: - if len(taker.honest_makers) != 0: - tumble_log.info("Transaction attempt failed, attempting to " - "restart with subset.") - tumble_log.info("The paramaters of the failed attempt: ") - tumble_log.info(str(taker.schedule[taker.schedule_index])) - #we must reset the number of counterparties, as well as fix who they - #are; this is because the number is used to e.g. calculate fees. - #cleanest way is to reset the number in the schedule before restart. - taker.schedule[taker.schedule_index][2] = len(taker.honest_makers) - retry_str = "Retrying with: " + str(taker.schedule[ - taker.schedule_index][2]) + " counterparties." - tumble_log.info(retry_str) - log.info(retry_str) - taker.set_honest_only(True) - taker.schedule_index -= 1 + #Is the failure in Phase 2? + if not taker.latest_tx is None: + #Now we have to set the specific group we want to use, and hopefully + #they will respond again as they showed honesty last time. + #Note that we must wipe the list first; other honest makers needn't + #have the right settings (e.g. max cjamount), so can't be carried + #over from earlier transactions. + taker.honest_makers = [] + taker.add_honest_makers(list(set( + taker.maker_utxo_data.keys()).symmetric_difference( + set(taker.nonrespondants)))) + #If insufficient makers were honest, we can only tweak the schedule. + #If enough were, we prefer the restart with them only: + log.info("Inside a Phase 2 failure; number of honest respondants was: " + str(len(taker.honest_makers))) + log.info("They were: " + str(taker.honest_makers)) + if len(taker.honest_makers) >= jm_single().config.getint( + "POLICY", "minimum_makers"): + tumble_log.info("Transaction attempt failed, attempting to " + "restart with subset.") + tumble_log.info("The paramaters of the failed attempt: ") + tumble_log.info(str(taker.schedule[taker.schedule_index])) + #we must reset the number of counterparties, as well as fix who they + #are; this is because the number is used to e.g. calculate fees. + #cleanest way is to reset the number in the schedule before restart. + taker.schedule[taker.schedule_index][2] = len(taker.honest_makers) + retry_str = "Retrying with: " + str(taker.schedule[ + taker.schedule_index][2]) + " counterparties." + tumble_log.info(retry_str) + log.info(retry_str) + taker.set_honest_only(True) + taker.schedule_index -= 1 + return - #a tumbler is aggressive in trying to complete; we tweak the schedule - #from this point in the mixdepth, then try again. However we only - #try this strategy if the previous (select-honest-only) strategy - #failed. - else: - tumble_log.info("Transaction attempt failed, tweaking schedule" - " and trying again.") - tumble_log.info("The paramaters of the failed attempt: ") - tumble_log.info(str(taker.schedule[taker.schedule_index])) - taker.schedule_index -= 1 - taker.schedule = tweak_tumble_schedule(options, taker.schedule, - taker.schedule_index, - taker.tdestaddrs) + #There were not enough honest counterparties. + #Tumbler is aggressive in trying to complete; we tweak the schedule + #from this point in the mixdepth, then try again. + tumble_log.info("Transaction attempt failed, tweaking schedule" + " and trying again.") + tumble_log.info("The paramaters of the failed attempt: ") + tumble_log.info(str(taker.schedule[taker.schedule_index])) + taker.schedule_index -= 1 + taker.schedule = tweak_tumble_schedule(options, taker.schedule, + taker.schedule_index, + taker.tdestaddrs) tumble_log.info("We tweaked the schedule, the new schedule is:") tumble_log.info(pprint.pformat(taker.schedule)) else: diff --git a/jmdaemon/jmdaemon/daemon_protocol.py b/jmdaemon/jmdaemon/daemon_protocol.py index 0c48ade..1997d93 100644 --- a/jmdaemon/jmdaemon/daemon_protocol.py +++ b/jmdaemon/jmdaemon/daemon_protocol.py @@ -632,7 +632,8 @@ class JMDaemonServerProtocol(amp.AMP, OrderbookWatch): """ if nick in self.crypto_boxes and self.crypto_boxes[nick] != None: return self.crypto_boxes[nick][1] - elif nick in self.active_orders and self.active_orders[nick] != None: + elif nick in self.active_orders and self.active_orders[nick] != None \ + and "crypto_box" in self.active_orders[nick]: return self.active_orders[nick]["crypto_box"] else: log.msg('something wrong, no crypto object, nick=' + nick + diff --git a/scripts/sendpayment.py b/scripts/sendpayment.py index e5959e1..3c4e9a5 100644 --- a/scripts/sendpayment.py +++ b/scripts/sendpayment.py @@ -189,24 +189,35 @@ def main(): clientfactory.getClient().clientStart) else: #a transaction failed; we'll try to repeat without the - #troublemakers - #Note that Taker.nonrespondants is always set to the full maker - #list at the start of Taker.receive_utxos, so it is always updated - #to a new list in each run. - print("We failed to complete the transaction. The following " - "makers didn't respond: ", taker.nonrespondants, - ", so we will retry without them.") - taker.add_ignored_makers(taker.nonrespondants) - #Now we have to set the specific group we want to use, and hopefully - #they will respond again as they showed honesty last time. + #troublemakers. + #If this error condition is reached from Phase 1 processing, + #and there are less than minimum_makers honest responses, we + #just give up (note that in tumbler we tweak and retry, but + #for sendpayment the user is "online" and so can manually + #try again). + #However if the error is in Phase 2 and we have minimum_makers + #or more responses, we do try to restart with the honest set, here. + if taker.latest_tx is None: + #can only happen with < minimum_makers; see above. + log.info("A transaction failed but there are insufficient " + "honest respondants to continue; giving up.") + reactor.stop() + return + #This is Phase 2; do we have enough to try again? taker.add_honest_makers(list(set( taker.maker_utxo_data.keys()).symmetric_difference( set(taker.nonrespondants)))) - if len(taker.honest_makers) == 0: - log.info("None of the makers responded honestly; " + if len(taker.honest_makers) < jm_single().config.getint( + "POLICY", "minimum_makers"): + log.info("Too few makers responded honestly; " "giving up this attempt.") reactor.stop() return + print("We failed to complete the transaction. The following " + "makers responded honestly: ", taker.honest_makers, + ", so we will retry with them.") + #Now we have to set the specific group we want to use, and hopefully + #they will respond again as they showed honesty last time. #we must reset the number of counterparties, as well as fix who they #are; this is because the number is used to e.g. calculate fees. #cleanest way is to reset the number in the schedule before restart.