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.

Sign up for GitHub

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

Jump to bottom

Validate BSQ fee payment using DAO tx info. #6425

Merged
merged 2 commits into from Dec 29, 2022
Merged

Validate BSQ fee payment using DAO tx info. #6425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1df3046
Validate BSQ fee payment using DAO tx info.
Nov 30, 2022
fa6beff
Apply code review changes.
Dec 29, 2022
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
32 changes: 21 additions & 11 deletions core/src/main/java/bisq/core/provider/mempool/MempoolService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,26 +96,36 @@ public void validateOfferMakerTx(OfferPayload offerPayload, Consumer<TxValidator
}

public void validateOfferMakerTx(TxValidator txValidator, Consumer<TxValidator> resultHandler) {
if (!isServiceSupported()) {
UserThread.runAfter(() -> resultHandler.accept(txValidator.endResult("mempool request not supported, bypassing", true)), 1);
return;
if (txValidator.getIsFeeCurrencyBtc() != null && txValidator.getIsFeeCurrencyBtc()) {
if (!isServiceSupported()) {
UserThread.runAfter(() -> resultHandler.accept(txValidator.endResult("mempool request not supported, bypassing", true)), 1);
return;
}
MempoolRequest mempoolRequest = new MempoolRequest(preferences, socks5ProxyProvider);
validateOfferMakerTx(mempoolRequest, txValidator, resultHandler);
} else {
// using BSQ for fees
UserThread.runAfter(() -> resultHandler.accept(txValidator.validateBsqFeeTx(true)), 1);
Copy link
Author

Choose a reason for hiding this comment

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

This BSQ fee check should be gated by a call to isServiceSupported()

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need it there. The MempoolService is not used to validate the BSQ fee because txValidator.validateBsqFeeTx(true) gets the transaction from the DaoStateService.

Copy link
Author

Choose a reason for hiding this comment

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

With the change to have BSQ fees checked using the DAO database rather than with data obtained from mempool.space, MempoolService is incorrectly named. (In fact, it was unfortunate that it was named referring to an implementation feature rather than functionality).

It would be more appropriately called TradeFeeCheckingService. Fee checking should be gated by isServiceSupported(), so that the filter and config settings can do their job in enabling/disabling the fee checking feature.

}
MempoolRequest mempoolRequest = new MempoolRequest(preferences, socks5ProxyProvider);
validateOfferMakerTx(mempoolRequest, txValidator, resultHandler);
}

public void validateOfferTakerTx(Trade trade, Consumer<TxValidator> resultHandler) {
validateOfferTakerTx(new TxValidator(daoStateService, trade.getTakerFeeTxId(), trade.getAmount(),
trade.isCurrencyForTakerFeeBtc(), filterManager), resultHandler);
trade.isCurrencyForTakerFeeBtc(), trade.getLockTime(), filterManager), resultHandler);
}

public void validateOfferTakerTx(TxValidator txValidator, Consumer<TxValidator> resultHandler) {
if (!isServiceSupported()) {
UserThread.runAfter(() -> resultHandler.accept(txValidator.endResult("mempool request not supported, bypassing", true)), 1);
return;
if (txValidator.getIsFeeCurrencyBtc() != null && txValidator.getIsFeeCurrencyBtc()) {
if (!isServiceSupported()) {
UserThread.runAfter(() -> resultHandler.accept(txValidator.endResult("mempool request not supported, bypassing", true)), 1);
return;
}
MempoolRequest mempoolRequest = new MempoolRequest(preferences, socks5ProxyProvider);
validateOfferTakerTx(mempoolRequest, txValidator, resultHandler);
} else {
// using BSQ for fees
resultHandler.accept(txValidator.validateBsqFeeTx(false));
}
MempoolRequest mempoolRequest = new MempoolRequest(preferences, socks5ProxyProvider);
validateOfferTakerTx(mempoolRequest, txValidator, resultHandler);
}

public void checkTxIsConfirmed(String txId, Consumer<TxValidator> resultHandler) {
Expand Down
86 changes: 24 additions & 62 deletions core/src/main/java/bisq/core/provider/mempool/TxValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import bisq.core.dao.governance.param.Param;
import bisq.core.dao.state.DaoStateService;
import bisq.core.dao.state.model.blockchain.Tx;
import bisq.core.filter.FilterManager;

import bisq.common.util.Tuple2;
Expand Down Expand Up @@ -56,12 +57,11 @@ public class TxValidator {

private final DaoStateService daoStateService;
private final FilterManager filterManager;
private long blockHeightAtOfferCreation; // Only set for maker.
private long feePaymentBlockHeight; // applicable to maker and taker fees
private final List<String> errorList;
private final String txId;
private Coin amount;
@Nullable
private Boolean isFeeCurrencyBtc = null;
private Boolean isFeeCurrencyBtc = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As isFeeCurrencyBtc is only used by one constructor and otherwise is not set (and not used) I think keeping it a nullable Boolean is better. In the use cases where its accessed the constructor is use with a non null value, so no need to initialize it with true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is one unused constructor as well. Better to remove that.

@Nullable
private Long chainHeight;
@Setter
Expand All @@ -85,13 +85,13 @@ public TxValidator(DaoStateService daoStateService,
String txId,
Coin amount,
@Nullable Boolean isFeeCurrencyBtc,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better remove the @nullable and use primitive boolean value as the callers always provide a non null value.

long blockHeightAtOfferCreation,
long feePaymentBlockHeight,
FilterManager filterManager) {
this.daoStateService = daoStateService;
this.txId = txId;
this.amount = amount;
this.isFeeCurrencyBtc = isFeeCurrencyBtc;
this.blockHeightAtOfferCreation = blockHeightAtOfferCreation;
this.feePaymentBlockHeight = feePaymentBlockHeight;
this.filterManager = filterManager;
this.errorList = new ArrayList<>();
this.jsonTxt = "";
Expand Down Expand Up @@ -119,8 +119,6 @@ public TxValidator parseJsonValidateMakerFeeTx(String jsonTxt, List<String> btcF
if (checkNotNull(isFeeCurrencyBtc)) {
status = checkFeeAddressBTC(jsonTxt, btcFeeReceivers)
&& checkFeeAmountBTC(jsonTxt, amount, true, getBlockHeightForFeeCalculation(jsonTxt));
} else {
status = checkFeeAmountBSQ(jsonTxt, amount, true, getBlockHeightForFeeCalculation(jsonTxt));
}
}
} catch (JsonSyntaxException e) {
Expand All @@ -132,6 +130,17 @@ public TxValidator parseJsonValidateMakerFeeTx(String jsonTxt, List<String> btcF
return endResult("Maker tx validation", status);
}

public TxValidator validateBsqFeeTx(boolean isMaker) {
Optional<Tx> tx = daoStateService.getTx(txId);
String statusStr = isMaker ? "Maker" : "Taker" + " tx validation";
if (tx.isEmpty()) {
log.info("DAO does not yet have the tx {}, bypassing check of burnt BSQ amount.", txId);
return endResult(statusStr, true);
} else {
return endResult(statusStr, checkFeeAmountBSQ(tx.get(), amount, isMaker, feePaymentBlockHeight));
}
}

public TxValidator parseJsonValidateTakerFeeTx(String jsonTxt, List<String> btcFeeReceivers) {
this.jsonTxt = jsonTxt;
boolean status = initialSanityChecks(txId, jsonTxt);
Expand All @@ -143,8 +152,6 @@ public TxValidator parseJsonValidateTakerFeeTx(String jsonTxt, List<String> btcF
if (isFeeCurrencyBtc) {
status = checkFeeAddressBTC(jsonTxt, btcFeeReceivers)
&& checkFeeAmountBTC(jsonTxt, amount, false, getBlockHeightForFeeCalculation(jsonTxt));
} else {
status = checkFeeAmountBSQ(jsonTxt, amount, false, getBlockHeightForFeeCalculation(jsonTxt));
}
}
} catch (JsonSyntaxException e) {
Expand Down Expand Up @@ -250,36 +257,24 @@ private boolean checkFeeAmountBTC(String jsonTxt, Coin tradeAmount, boolean isMa
return false;
}

// I think its better to postpone BSQ fee check once the BSQ trade fee tx is confirmed and then use the BSQ explorer to request the
// BSQ fee to check if it is correct.
// Otherwise the requirements here become very complicated and potentially impossible to verify as we don't know
// if inputs and outputs are valid BSQ without the BSQ parser and confirmed transactions.
private boolean checkFeeAmountBSQ(String jsonTxt, Coin tradeAmount, boolean isMaker, long blockHeight) {
JsonArray jsonVin = getVinAndVout(jsonTxt).first;
JsonArray jsonVout = getVinAndVout(jsonTxt).second;
JsonObject jsonVin0 = jsonVin.get(0).getAsJsonObject();
JsonObject jsonVout0 = jsonVout.get(0).getAsJsonObject();
JsonElement jsonVIn0Value = jsonVin0.getAsJsonObject("prevout").get("value");
JsonElement jsonVOut0Value = jsonVout0.getAsJsonObject().get("value");
if (jsonVIn0Value == null || jsonVOut0Value == null) {
throw new JsonSyntaxException("vin/vout missing data");
}
private boolean checkFeeAmountBSQ(Tx bsqTx, Coin tradeAmount, boolean isMaker, long blockHeight) {
Param minFeeParam = isMaker ? Param.MIN_MAKER_FEE_BSQ : Param.MIN_TAKER_FEE_BSQ;
long expectedFeeAsLong = calculateFee(tradeAmount,
isMaker ? getMakerFeeRateBsq(blockHeight) : getTakerFeeRateBsq(blockHeight),
minFeeParam).getValue();
long feeValue = getBsqBurnt(jsonVin, jsonVOut0Value.getAsLong(), expectedFeeAsLong);

long feeValue = bsqTx.getBurntBsq();
log.debug("BURNT BSQ maker fee: {} BSQ ({} sats)", (double) feeValue / 100.0, feeValue);
String description = String.format("Expected fee: %.2f BSQ, actual fee paid: %.2f BSQ",
(double) expectedFeeAsLong / 100.0, (double) feeValue / 100.0);
String description = String.format("Expected fee: %.2f BSQ, actual fee paid: %.2f BSQ, Trade amount: %s",
(double) expectedFeeAsLong / 100.0, (double) feeValue / 100.0, tradeAmount.toPlainString());

if (expectedFeeAsLong == feeValue) {
log.debug("The fee matched. " + description);
return true;
}

if (expectedFeeAsLong < feeValue) {
log.info("The fee was more than what we expected. " + description);
log.info("The fee was more than what we expected. " + description + " Tx:" + bsqTx.getId());
return true;
}

Expand Down Expand Up @@ -350,39 +345,6 @@ private static boolean initialSanityChecks(String txId, String jsonTxt) {
// we don't care if it is confirmed or not, just that it exists.
}

// a BSQ maker/taker fee transaction looks like this:
// BSQ INPUT 1 BSQ OUTPUT
// BSQ INPUT 2 BTC OUTPUT FOR RESERVED AMOUNT
// BSQ INPUT n BTC OUTPUT FOR CHANGE
// BTC INPUT 1
// BTC INPUT 2
// BTC INPUT n
// there can be any number of BSQ inputs and BTC inputs
// BSQ inputs always come first in the tx, followed by BTC for the collateral.
// the sum of all BSQ inputs minus the BSQ output is the burnt amount, or trading fee.
long getBsqBurnt(JsonArray jsonVin, long bsqOutValue, long expectedFee) {
// sum consecutive inputs until we have accumulated enough to cover the output + burnt amount
long bsqInValue = 0;
for (int txIndex = 0; txIndex < jsonVin.size() - 1; txIndex++) {
bsqInValue += jsonVin.get(txIndex).getAsJsonObject().getAsJsonObject("prevout").get("value").getAsLong();
if (bsqInValue - expectedFee >= bsqOutValue) {
break; // target reached - bsq input exceeds the output and expected burn amount
}
}
// guard against negative burn amount (i.e. only 1 tx input, or first in < first out)
long burntAmount = Math.max(0, bsqInValue - bsqOutValue);
// since we do not know which of the first 'n' are definitively BSQ inputs, sanity-check that the burnt amount
// is not too ridiculously high, as that would imply that we counted a BTC input.
if (burntAmount > 10 * expectedFee) {
log.error("The apparent BSQ fee burnt seems ridiculously high ({}) compared to expected ({})", burntAmount, expectedFee);
burntAmount = 0; // returning zero will flag the trade for manual review
}
if (burntAmount == 0) {
log.error("Could not obtain the burnt BSQ amount, trade will be flagged for manual review.");
}
return burntAmount;
}

private static long getTxConfirms(String jsonTxt, long chainHeight) {
long blockHeight = getTxBlockHeight(jsonTxt);
if (blockHeight > 0) {
Expand All @@ -395,8 +357,8 @@ private static long getTxConfirms(String jsonTxt, long chainHeight) {
// if the tx is not yet confirmed, use current block tip, if tx is confirmed use the block it was confirmed at.
private long getBlockHeightForFeeCalculation(String jsonTxt) {
// For the maker we set the blockHeightAtOfferCreation from the offer
if (blockHeightAtOfferCreation > 0) {
return blockHeightAtOfferCreation;
if (feePaymentBlockHeight > 0) {
return feePaymentBlockHeight;
}

long txBlockHeight = getTxBlockHeight(jsonTxt);
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/bisq/core/trade/model/bisq_v1/Trade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,10 @@ public Date getMaxTradePeriodDate() {
return new Date(getTradeStartTime() + getMaxTradePeriod());
}

public long getTradeAge() {
return System.currentTimeMillis() - getTradeStartTime();
}

private long getMaxTradePeriod() {
return offer.getPaymentMethod().getMaxTradePeriod();
}
Expand Down
Loading