Set default tx_broadcast = random-peer

[chris-belcher]

This method is slightly more private with no other downsides I think

[chris-belcher]

Anyone have any objections to this? If not I'll merge it. It has no downsides only upsides.

[AdamISZ]

Sorry not much time for looking at stuff now but: if the peer doesn't broadcast, am I right in thinking we just timeout and restart the tx instead of broadcasting it ourself? I think my earlier thinking was to stick with self default for this reason, but I don't remember in detail and would have to read the code again.

[kristapsk]

I think fallbacking to broadcasting yourself is better than restarting, as whole coinjoins process may take time (minutes), are not guaranteed to succeed (sometimes happen that less than minimum default_makers makers respond), JM are mostly ran behind Tor anway and defaulting to tx_broadcast = random-peer is enough for other parties to not have any confidence that broadcaster of JM CJ tx is actually a taker side.

[AdamISZ]

I think fallbacking to broadcasting yourself is better than restarting

I strongly agree, all the more so because we have a "no-self" option (although I think it might not be implemented right now). Clearly timing out and restarting just because you chose a peer who didn't bother to broadcast is a bit shitty, which is why I'm mentioning it.

[PulpCattel]

@AdamISZ
Isn't already working as it should?

Based on #704, #677, and:

joinmarket-clientserver/jmclient/jmclient/taker.py

Lines 824 to 848 in b25d03c

	def handle_unbroadcast_transaction(self, txid, tx):
	""" The wallet service will handle dangling
	callbacks for transactions but we want to reattempt
	broadcast in case the cause of the problem is a
	counterparty who refused to broadcast it for us.
	"""
	if not self.wallet_service.check_callback_called(
	self.txid, self.unconfirm_callback, "unconfirmed",
	"transaction with txid: " + str(self.txid) + " not broadcast."):
	# we now know the transaction was not pushed, so we reinstigate
	# the cancelledcallback with the same logic as explained
	# in Taker.push():
	self.wallet_service.register_callbacks([self.unconfirm_callback],
	txid, "unconfirmed")
	if jm_single().config.get('POLICY', 'tx_broadcast') == "not-self":
	warnmsg = ("You have chosen not to broadcast from your own "
	"node. The transaction is NOT broadcast.")
	self.taker_info_callback("ABORT", warnmsg + "\nSee log for details.")
	# warning is arguably not correct but it will stand out more:
	jlog.warn(warnmsg)
	jlog.info(btc.human_readable_transaction(tx))
	return
	if not self.push_ourselves():
	jlog.error("Failed to broadcast transaction: ")
	jlog.info(btc.human_readable_transaction(tx))

I think we already fallback to broadcast ourselves, right?

[AdamISZ]

@PulpCattel Thanks for checking it out for me! Imagine remembering any of the code you wrote ...

Someone might want to investigate how this interacts with how often the stall monitor wakes up, but I guess it's probably ~ correct.

[AdamISZ]

So based on: the code already addresses my concern above, ACK to this PR.

[chris-belcher]

In hindsight I should've elaborated on the fallback, because I also examined that code which @PulpCattel linked before opening this PR

[chris-belcher]

One is the actual commit, the other is the merge which added that commit to the master branch

[AdamISZ]

@chris-belcher the merge of this PR broke one test in the test suite, specifically:

test_on_sig in jmclient.test.test_taker. The reason is not particularly interesting, but basically: in Taker.push, if we don't redirect to push_ourselves with the self option in tx_broadcast, then it checks self.maker_utxo_data to find a nick, but in that test setup, maker_utxo_data doesn't exist. It gets triggered on this last line, where the transaction is complete and it attempts to self-sign:

joinmarket-clientserver/jmclient/test/test_taker.py

Line 592 in 7f62bfe

taker.on_sig("cp3", sig5)

[AdamISZ]

@chris-belcher what's confusing?

[chris-belcher]

I'm not confused, it's just the closest thing to a sad or regret emoje.

[kristapsk]

@AdamISZ @chris-belcher Any plans to fix that test failure?

[AdamISZ]

It's fixed in #910 as a side-effect.