Fix broadcast issues #165
Fix broadcast issues #165
Conversation
- We handle the timeout by default with a call onSuccess to reduce potential problems with delayed broadcasts.
We set the depositTx at success handler before state change gets triggered. In regtest mode the success handler might get called immediately, so it could cause an error if client code triggered by state change expecting the depositTx would see it not set.
- In case the tx broadcast did not completed in time we call the onTimeout handler which logs a waring but optimistically assume that the broadcast will still succeed later and call onSuccess. This strategy has advantages over calling onFailure as it might be more often the case that the broadcast was just slow but successful later then that the broadcast fails at all.
| private final Wallet wallet; | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
(Optional) It's great that the docstring was added here, but maybe there could be some description, either here or in a class-level docstring, of what the TxBroadCastTimeoutException means semantically?
There was a problem hiding this comment.
I added a comment in next PR.
| "We optimistically assume that the tx broadcast succeeds later and call onSuccess on the " + | ||
| "callback handler. This behaviour carries less potential problems than if we would trigger " + | ||
| "a failure (e.g. which would cause a failed create offer attempt of failed take offer attempt).\n" + | ||
| "We have no guarantee how long it will take to get the information that sufficiently BTC " + |
There was a problem hiding this comment.
Nit: Where we say sufficiently BTC nodes, it seems more clear to instead say sufficently many Bitcoin nodes.
| "In normal situations " + | ||
| "that's very fast but in some cases it can take minutes (mostly related to Tor connection " + | ||
| "issues). So if we just go on in the application logic and treat it as successful and the " + | ||
| "tx will be broadcasted successfully later all is fine.\n" + |
| "that's very fast but in some cases it can take minutes (mostly related to Tor connection " + | ||
| "issues). So if we just go on in the application logic and treat it as successful and the " + | ||
| "tx will be broadcasted successfully later all is fine.\n" + | ||
| "If it will fail to get broadcasted, " + |
| "If it will fail to get broadcasted, " + | ||
| "it will lead to a failure state, the same as if we would trigger a failure due the timeout." + | ||
| "So we can assume that this behaviour will lead to less problems as otherwise.\n" + | ||
| "Long term we should implement better monitoring for Tor and the provided Bitcoin nodes to " + |
There was a problem hiding this comment.
(Optional) I think the text of this log message is really useful to remember why we are doing this. But maybe it would be better to discuss future how we could make future improvements etc in the form of a TODO comment in the code, or a Github issue?
| "find out why those delays happen and add some rollback behaviour to the app state in case " + | ||
| "the tx will never get broadcasted.", | ||
| exception.toString(), | ||
| txId, |
There was a problem hiding this comment.
There seems to be three arguments after the main string given, but I only see one {} in the string itself. It seems that either there are some missing {} where txId and exception.getDelay() should be placed in the string, or those arguments should be dropped from log.warn().
| // The commit is required in case the tx is spent by a follow up tx as otherwise there would be an | ||
| // InsufficientMoneyException thrown. But in some test scenarios we also got issues with wallet | ||
| // inconsistency if the tx was committed twice. It should be prevented by the maybeCommitTx methods but | ||
| // not 100% if that is always the case. Just added that comment to make clear that this might be a risky |
There was a problem hiding this comment.
Nit: I think the comment mentioning why the comment was added is a little too meta.. maybe instead of:
Just added that comment to make clear that this might be a risky strategy and might need improvement if we get problems.
We could say:
This might be risky and we should re-evalute the choices we made here if there's future issues.
There was a problem hiding this comment.
Ah maybe there was a misunderstanding. It was meant: The wallet.maybeCommitTx() call is required...
| // inconsistency if the tx was committed twice. It should be prevented by the maybeCommitTx methods but | ||
| // not 100% if that is always the case. Just added that comment to make clear that this might be a risky | ||
| // strategy and might need improvement if we get problems. | ||
| exception.getWallet().maybeCommitTx(tx); |
There was a problem hiding this comment.
(Optional) It feels a little odd to have the TxBroadcastTimeoutException class know about the Wallet.. I understand why we need that reference here, but it feels like it's not a concern that the exception class should need to handle.
There was a problem hiding this comment.
Yes i know, did not find a better solution though...
| @@ -94,8 +126,8 @@ public void onSuccess(@Nullable Transaction result) { | |||
| UserThread.execute(() -> callback.onSuccess(tx)); | |||
| } else { | |||
| stopAndRemoveTimer(txId); | |||
| UserThread.execute(() -> callback.onFailure(new TxBroadcastException("We got an onSuccess callback for " + | |||
| "a broadcast which got already triggered the timeout.", txId))); | |||
| log.warn("We got an onSuccess callback for a broadcast which got already triggered " + | |||
There was a problem hiding this comment.
Nit: Instead of for a broadcast which got already triggered the timeout it seems more clear to say for a broadcast which already triggered the timeout.
| @@ -92,6 +92,7 @@ protected void run() { | |||
| @Override | |||
| public void onSuccess(Transaction transaction) { | |||
| if (!completed) { | |||
| trade.setDepositTx(transaction); | |||
There was a problem hiding this comment.
(Optional) It looks surprising to find the transaction referenced in this onSuccess() handler, when we are in a context of creating the very same depositTx.. I understand from conversation with @ManfredKarrer that this edit was necessary when testing on regtest, where the onSuccess() handler would fire before the trade.setDepositTx(depositTx) line on L118. Maybe a comment could be added here to explain this, to help future maintainers understand?
There was a problem hiding this comment.
Will add a comment
|
Issues fixed at: b3add47 |
Depends on:
bisq-network/bisq-common#37