Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of failed trades and offers #3566

Merged
merged 3 commits into from
Nov 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/main/java/bisq/core/app/BisqHeadlessApp.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ protected void setupHandlers() {
bisqSetup.setDisplaySecurityRecommendationHandler(key -> log.info("onDisplaySecurityRecommendationHandler"));
bisqSetup.setDisplayLocalhostHandler(key -> log.info("onDisplayLocalhostHandler"));
bisqSetup.setWrongOSArchitectureHandler(msg -> log.error("onWrongOSArchitectureHandler. msg={}", msg));
bisqSetup.setVoteResultExceptionHandler(voteResultException -> log.warn("voteResultException={}", voteResultException));
bisqSetup.setVoteResultExceptionHandler(voteResultException -> log.warn("voteResultException={}", voteResultException.toString()));
bisqSetup.setRejectedTxErrorMessageHandler(errorMessage -> log.warn("setRejectedTxErrorMessageHandler. errorMessage={}", errorMessage));

//TODO move to bisqSetup
corruptedDatabaseFilesHandler.getCorruptedDatabaseFiles().ifPresent(files -> log.warn("getCorruptedDatabaseFiles. files={}", files));
Expand Down
110 changes: 94 additions & 16 deletions core/src/main/java/bisq/core/app/BisqSetup.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import bisq.core.support.dispute.refund.refundagent.RefundAgentManager;
import bisq.core.support.traderchat.TraderChatManager;
import bisq.core.trade.TradeManager;
import bisq.core.trade.TradeTxException;
import bisq.core.trade.statistics.AssetTradeActivityCheck;
import bisq.core.trade.statistics.TradeStatisticsManager;
import bisq.core.user.Preferences;
Expand Down Expand Up @@ -107,6 +108,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
Expand Down Expand Up @@ -192,7 +194,8 @@ default void onRequestWalletPassword() {
spvFileCorruptedHandler, lockedUpFundsHandler, daoErrorMessageHandler, daoWarnMessageHandler,
filterWarningHandler, displaySecurityRecommendationHandler, displayLocalhostHandler,
wrongOSArchitectureHandler, displaySignedByArbitratorHandler,
displaySignedByPeerHandler, displayPeerLimitLiftedHandler, displayPeerSignerHandler;
displaySignedByPeerHandler, displayPeerLimitLiftedHandler, displayPeerSignerHandler,
rejectedTxErrorMessageHandler;
@Setter
@Nullable
private Consumer<Boolean> displayTorNetworkSettingsHandler;
Expand Down Expand Up @@ -601,28 +604,56 @@ private void initWallet() {
showFirstPopupIfResyncSPVRequestedHandler,
walletPasswordHandler,
() -> {
if (allBasicServicesInitialized)
if (allBasicServicesInitialized) {
checkForLockedUpFunds();
checkForInvalidMakerFeeTxs();
}
},
() -> walletInitialized.set(true));
}


private void checkForLockedUpFunds() {
btcWalletService.getAddressEntriesForTrade().stream()
.filter(e -> tradeManager.getSetOfAllTradeIds().contains(e.getOfferId()) &&
e.getContext() == AddressEntry.Context.MULTI_SIG)
.forEach(e -> {
Coin balance = e.getCoinLockedInMultiSig();
if (balance.isPositive()) {
String message = Res.get("popup.warning.lockedUpFunds",
formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
log.warn(message);
if (lockedUpFundsHandler != null) {
lockedUpFundsHandler.accept(message);
// We check if there are locked up funds in failed or closed trades
try {
Set<String> setOfAllTradeIds = tradeManager.getSetOfFailedOrClosedTradeIdsFromLockedInFunds();
btcWalletService.getAddressEntriesForTrade().stream()
.filter(e -> setOfAllTradeIds.contains(e.getOfferId()) &&
e.getContext() == AddressEntry.Context.MULTI_SIG)
.forEach(e -> {
Coin balance = e.getCoinLockedInMultiSig();
if (balance.isPositive()) {
String message = Res.get("popup.warning.lockedUpFunds",
formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
log.warn(message);
if (lockedUpFundsHandler != null) {
lockedUpFundsHandler.accept(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something preventing automatic opening of disputes here, rather than asking the trader to do it manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to reproduce that and if deposit tx is null then the user cannot open a dispute.

}
}
}
});
});
} catch (TradeTxException e) {
log.warn(e.getMessage());
if (lockedUpFundsHandler != null) {
lockedUpFundsHandler.accept(e.getMessage());
}
}
}

private void checkForInvalidMakerFeeTxs() {
// We check if we have open offers with no confidence object at the maker fee tx. That can happen if the
// miner fee was too low and the transaction got removed from mempool and got out from our wallet after a
// resync.
openOfferManager.getObservableList().forEach(e -> {
String offerFeePaymentTxId = e.getOffer().getOfferFeePaymentTxId();
if (btcWalletService.getConfidenceForTxId(offerFeePaymentTxId) == null) {
String message = Res.get("popup.warning.openOfferWithInvalidMakerFeeTx",
e.getOffer().getShortId(), offerFeePaymentTxId);
log.warn(message);
if (lockedUpFundsHandler != null) {
lockedUpFundsHandler.accept(message);
}
}
});
}

private void checkForCorrectOSArchitecture() {
Expand Down Expand Up @@ -651,13 +682,60 @@ private void initDomainServices() {

tradeManager.onAllServicesInitialized();

if (walletsSetup.downloadPercentageProperty().get() == 1)
if (walletsSetup.downloadPercentageProperty().get() == 1) {
checkForLockedUpFunds();
checkForInvalidMakerFeeTxs();
}

openOfferManager.onAllServicesInitialized();

balances.onAllServicesInitialized();

walletAppSetup.getRejectedTxException().addListener((observable, oldValue, newValue) -> {
// We delay as we might get the rejected tx error before we have completed the create offer protocol
UserThread.runAfter(() -> {
if (rejectedTxErrorMessageHandler != null && newValue != null && newValue.getTxId() != null) {
String txId = newValue.getTxId();
openOfferManager.getObservableList().stream()
.filter(openOffer -> txId.equals(openOffer.getOffer().getOfferFeePaymentTxId()))
.forEach(openOffer -> {
// We delay to avoid concurrent modification exceptions
UserThread.runAfter(() -> {
openOffer.getOffer().setErrorMessage(newValue.getMessage());
rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.openOffer.makerFeeTxRejected", openOffer.getId(), txId));
openOfferManager.removeOpenOffer(openOffer, () -> {
log.warn("We removed an open offer because the maker fee was rejected by the Bitcoin " +
"network. OfferId={}, txId={}", openOffer.getShortId(), txId);
}, log::warn);
}, 1);
});

tradeManager.getTradableList().stream()
.filter(trade -> trade.getOffer() != null)
.forEach(trade -> {
String details = null;
if (txId.equals(trade.getDepositTxId())) {
details = Res.get("popup.warning.trade.txRejected.deposit");
}
if (txId.equals(trade.getOffer().getOfferFeePaymentTxId()) || txId.equals(trade.getTakerFeeTxId())) {
details = Res.get("popup.warning.trade.txRejected.tradeFee");
}

if (details != null) {
// We delay to avoid concurrent modification exceptions
String finalDetails = details;
UserThread.runAfter(() -> {
trade.setErrorMessage(newValue.getMessage());
rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.trade.txRejected",
finalDetails, trade.getShortId(), txId));
tradeManager.addTradeToFailedTrades(trade);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to avoid losing the offer fee tx and revert this to an active offer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feat that is too complex to get it right. It should be exceptional case anyway. And users can request for reimbursement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point just getting things more stable is good. Perhaps we could look at it if it's recurring event that people lose their fee this way.

}, 1);
}
});
}
}, 3);
});

arbitratorManager.onAllServicesInitialized();
mediatorManager.onAllServicesInitialized();
refundAgentManager.onAllServicesInitialized();
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/bisq/core/app/WalletAppSetup.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package bisq.core.app;

import bisq.core.btc.exceptions.RejectedTxException;
import bisq.core.btc.setup.WalletsSetup;
import bisq.core.btc.wallet.WalletsManager;
import bisq.core.locale.Res;
Expand Down Expand Up @@ -71,6 +72,8 @@ public class WalletAppSetup {
@Getter
private final StringProperty btcInfo = new SimpleStringProperty(Res.get("mainView.footer.btcInfo.initializing"));
@Getter
private final ObjectProperty<RejectedTxException> rejectedTxException = new SimpleObjectProperty<>();
@Getter
private int numBtcPeers = 0;
@Getter
private final BooleanProperty useTorForBTC = new SimpleBooleanProperty();
Expand Down Expand Up @@ -132,7 +135,7 @@ void init(@Nullable Consumer<String> chainFileLockedExceptionHandler,
getNumBtcPeers(),
Res.get("mainView.footer.btcInfo.connectionFailed"),
getBtcNetworkAsString());
log.error(exception.getMessage());
log.error(exception.toString());
if (exception instanceof TimeoutException) {
getWalletServiceErrorMsg().set(Res.get("mainView.walletServiceErrorMsg.timeout"));
} else if (exception.getCause() instanceof BlockStoreException) {
Expand All @@ -141,6 +144,9 @@ void init(@Nullable Consumer<String> chainFileLockedExceptionHandler,
} else if (spvFileCorruptedHandler != null) {
spvFileCorruptedHandler.accept(Res.get("error.spvFileCorrupted", exception.getMessage()));
}
} else if (exception instanceof RejectedTxException) {
rejectedTxException.set((RejectedTxException) exception);
getWalletServiceErrorMsg().set(Res.get("mainView.walletServiceErrorMsg.rejectedTxException", exception.getMessage()));
} else {
getWalletServiceErrorMsg().set(Res.get("mainView.walletServiceErrorMsg.connectionError", exception.toString()));
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/bisq/core/btc/Balances.java
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ private void updateReservedBalance() {
}

private void updateLockedBalance() {
Stream<Trade> lockedTrades = Stream.concat(closedTradableManager.getLockedTradesStream(), failedTradesManager.getLockedTradesStream());
lockedTrades = Stream.concat(lockedTrades, tradeManager.getLockedTradesStream());
Stream<Trade> lockedTrades = Stream.concat(closedTradableManager.getTradesStreamWithFundsLockedIn(), failedTradesManager.getTradesStreamWithFundsLockedIn());
lockedTrades = Stream.concat(lockedTrades, tradeManager.getTradesStreamWithFundsLockedIn());
long sum = lockedTrades.map(trade -> btcWalletService.getAddressEntry(trade.getId(), AddressEntry.Context.MULTI_SIG)
.orElse(null))
.filter(Objects::nonNull)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* This file is part of Bisq.
*
* Bisq is free software: you can redistribute it and/or modify it
* under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or (at
* your option) any later version.
*
* Bisq is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
* License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with Bisq. If not, see <http://www.gnu.org/licenses/>.
*/

package bisq.core.btc.exceptions;

import org.bitcoinj.core.RejectMessage;

import lombok.Getter;

import javax.annotation.Nullable;

public class RejectedTxException extends RuntimeException {
@Getter
private final RejectMessage rejectMessage;
@Getter
@Nullable
private final String txId;

public RejectedTxException(String message, RejectMessage rejectMessage) {
super(message);
this.rejectMessage = rejectMessage;
txId = rejectMessage.getRejectedObjectHash() != null ? rejectMessage.getRejectedObjectHash().toString() : null;
}

@Override
public String toString() {
return "RejectedTxException{" +
"\n rejectMessage=" + rejectMessage +
",\n txId='" + txId + '\'' +
"\n} " + super.toString();
}
}
24 changes: 22 additions & 2 deletions core/src/main/java/bisq/core/btc/setup/WalletsSetup.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import bisq.core.app.BisqEnvironment;
import bisq.core.btc.BtcOptionKeys;
import bisq.core.btc.exceptions.RejectedTxException;
import bisq.core.btc.model.AddressEntry;
import bisq.core.btc.model.AddressEntryList;
import bisq.core.btc.nodes.BtcNetworkConfig;
Expand All @@ -44,6 +45,7 @@
import org.bitcoinj.core.Peer;
import org.bitcoinj.core.PeerAddress;
import org.bitcoinj.core.PeerGroup;
import org.bitcoinj.core.RejectMessage;
import org.bitcoinj.core.listeners.DownloadProgressTracker;
import org.bitcoinj.params.RegTestParams;
import org.bitcoinj.utils.Threading;
Expand Down Expand Up @@ -170,7 +172,9 @@ public WalletsSetup(RegTestHost regTestHost,
// Lifecycle
///////////////////////////////////////////////////////////////////////////////////////////

public void initialize(@Nullable DeterministicSeed seed, ResultHandler resultHandler, ExceptionHandler exceptionHandler) {
public void initialize(@Nullable DeterministicSeed seed,
ResultHandler resultHandler,
ExceptionHandler exceptionHandler) {
// Tell bitcoinj to execute event handlers on the JavaFX UI thread. This keeps things simple and means
// we cannot forget to switch threads when adding event handlers. Unfortunately, the DownloadListener
// we give to the app kit is currently an exception and runs on a library thread. It'll get fixed in
Expand Down Expand Up @@ -221,6 +225,20 @@ protected void onSetupCompleted() {
peerGroup.addBlocksDownloadedEventListener((peer, block, filteredBlock, blocksLeft) -> {
blocksDownloadedFromPeer.set(peer);
});

// Need to be Threading.SAME_THREAD executor otherwise BitcoinJ will skip that listener
peerGroup.addPreMessageReceivedEventListener(Threading.SAME_THREAD, (peer, message) -> {
if (message instanceof RejectMessage) {
UserThread.execute(() -> {
RejectMessage rejectMessage = (RejectMessage) message;
String msg = rejectMessage.toString();
log.warn(msg);
exceptionHandler.handleException(new RejectedTxException(msg, rejectMessage));
});
}
return message;
});

chain.addNewBestBlockListener(block -> {
connectedPeers.set(peerGroup.getConnectedPeers());
chainHeight.set(block.getHeight());
Expand Down Expand Up @@ -372,7 +390,9 @@ public void clearBackups() {
// Restore
///////////////////////////////////////////////////////////////////////////////////////////

public void restoreSeedWords(@Nullable DeterministicSeed seed, ResultHandler resultHandler, ExceptionHandler exceptionHandler) {
public void restoreSeedWords(@Nullable DeterministicSeed seed,
ResultHandler resultHandler,
ExceptionHandler exceptionHandler) {
checkNotNull(seed, "Seed must be not be null.");

backupWallets();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public void onTransactionConfidenceChanged(Wallet wallet, Transaction tx) {
// We are only interested in updates from unconfirmed txs and confirmed txs at the
// time when it gets into a block. Otherwise we would get called
// updateBsqWalletTransactions for each tx as the block depth changes for all.
if (tx.getConfidence().getDepthInBlocks() <= 1 &&
if (tx != null && tx.getConfidence() != null && tx.getConfidence().getDepthInBlocks() <= 1 &&
daoStateService.isParseBlockChainComplete()) {
updateBsqWalletTransactions();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,13 @@ public void resetAddressEntriesForOpenOffer(String offerId) {

public void resetAddressEntriesForPendingTrade(String offerId) {
swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.MULTI_SIG);
// Don't swap TRADE_PAYOUT as it might be still open in the last trade step to be used for external transfer
// We swap also TRADE_PAYOUT to be sure all is cleaned up. There might be cases where a user cannot send the funds
// to an external wallet directly in the last step of the trade, but the funds are in the Bisq wallet anyway and
// the dealing with the external wallet is pure UI thing. The user can move the funds to the wallet and then
// send out the funds to the external wallet. As this cleanup is a rare situation and most users do not use
// the feature to send out the funds we prefer that strategy (if we keep the address entry it might cause
// complications in some edge cases after a SPV resync).
swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.TRADE_PAYOUT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a risk of address reuse by re-labeling this address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was never used in that use case (failed trade), so its a fresh address.

}

public void swapAnyTradeEntryContextToAvailableEntry(String offerId) {
Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/bisq/core/dao/node/parser/BlockParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,8 @@ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingExceptio
genesisTotalSupply)
.ifPresent(txList::add));

if (System.currentTimeMillis() - startTs > 0)
log.info("Parsing {} transactions at block height {} took {} ms", rawBlock.getRawTxs().size(),
blockHeight, System.currentTimeMillis() - startTs);
log.info("Parsing {} transactions at block height {} took {} ms", rawBlock.getRawTxs().size(),
blockHeight, System.currentTimeMillis() - startTs);

daoStateService.onParseBlockComplete(block);
return block;
Expand Down
Loading