From 267becf612961e5215aa594237afa9d4715d8590 Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Sat, 13 Jul 2024 16:28:06 +0200 Subject: [PATCH 1/6] Fix minor quadratic time bug in getBurningManCandidatesByName Avoid streaming over the entire proposals list to find a matching txId, for every 'Issuance' & 'CompensationProposal' pair used to construct and add a compensation model to the burn output model of each candidate. Instead, stream over the proposals list once, doing lookups by txId of each matching issuance, which uses the TreeMap 'DaoState.issuanceMap', thereby taking O(n*log(n)) time. --- .../dao/burningman/BurningManService.java | 90 +++++++++---------- .../dao/burningman/BurningManServiceTest.java | 6 +- 2 files changed, 48 insertions(+), 48 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java index 41b7dd034a5..3236234daf9 100644 --- a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java +++ b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java @@ -31,10 +31,11 @@ import bisq.core.dao.state.model.blockchain.TxOutput; import bisq.core.dao.state.model.governance.CompensationProposal; import bisq.core.dao.state.model.governance.Issuance; -import bisq.core.dao.state.model.governance.IssuanceType; import bisq.network.p2p.storage.P2PDataStorage; +import bisq.common.util.Tuple2; + import javax.inject.Inject; import javax.inject.Singleton; @@ -52,8 +53,8 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import java.util.function.BiConsumer; import java.util.stream.Collectors; -import java.util.stream.Stream; import lombok.extern.slf4j.Slf4j; @@ -123,45 +124,40 @@ Map getBurningManCandidatesByName(int chainHeight, Map> proofOfBurnOpReturnTxOutputByHash = getProofOfBurnOpReturnTxOutputByHash(chainHeight); // Add contributors who made a compensation request - daoStateService.getIssuanceSetForType(IssuanceType.COMPENSATION).stream() - .filter(issuance -> issuance.getChainHeight() <= chainHeight) - .forEach(issuance -> { - getCompensationProposalsForIssuance(issuance).forEach(compensationProposal -> { - String name = compensationProposal.getName(); - BurningManCandidate candidate = burningManCandidatesByName.computeIfAbsent(name, n -> new BurningManCandidate()); - - // Issuance - Optional customAddress = compensationProposal.getBurningManReceiverAddress(); - boolean isCustomAddress = customAddress.isPresent(); - Optional receiverAddress; - if (isCustomAddress) { - receiverAddress = customAddress; - } else { - // We take change address from compensation request - receiverAddress = daoStateService.getTx(compensationProposal.getTxId()) - .map(this::getAddressFromCompensationRequest); - } - if (receiverAddress.isPresent()) { - int issuanceHeight = issuance.getChainHeight(); - long issuanceAmount = getIssuanceAmountForCompensationRequest(issuance); - int cycleIndex = cyclesInDaoStateService.getCycleIndexAtChainHeight(issuanceHeight); - if (isValidCompensationRequest(name, cycleIndex, issuanceAmount)) { - long decayedIssuanceAmount = getDecayedCompensationAmount(issuanceAmount, issuanceHeight, chainHeight); - long issuanceDate = daoStateService.getBlockTime(issuanceHeight); - candidate.addCompensationModel(CompensationModel.fromCompensationRequest(receiverAddress.get(), - isCustomAddress, - issuanceAmount, - decayedIssuanceAmount, - issuanceHeight, - issuance.getTxId(), - issuanceDate, - cycleIndex)); - } - } - addBurnOutputModel(chainHeight, proofOfBurnOpReturnTxOutputByHash, name, candidate); - }); - } - ); + forEachCompensationIssuance(chainHeight, (issuance, compensationProposal) -> { + String name = compensationProposal.getName(); + BurningManCandidate candidate = burningManCandidatesByName.computeIfAbsent(name, n -> new BurningManCandidate()); + + // Issuance + Optional customAddress = compensationProposal.getBurningManReceiverAddress(); + boolean isCustomAddress = customAddress.isPresent(); + Optional receiverAddress; + if (isCustomAddress) { + receiverAddress = customAddress; + } else { + // We take change address from compensation request + receiverAddress = daoStateService.getTx(compensationProposal.getTxId()) + .map(this::getAddressFromCompensationRequest); + } + if (receiverAddress.isPresent()) { + int issuanceHeight = issuance.getChainHeight(); + long issuanceAmount = getIssuanceAmountForCompensationRequest(issuance); + int cycleIndex = cyclesInDaoStateService.getCycleIndexAtChainHeight(issuanceHeight); + if (isValidCompensationRequest(name, cycleIndex, issuanceAmount)) { + long decayedIssuanceAmount = getDecayedCompensationAmount(issuanceAmount, issuanceHeight, chainHeight); + long issuanceDate = daoStateService.getBlockTime(issuanceHeight); + candidate.addCompensationModel(CompensationModel.fromCompensationRequest(receiverAddress.get(), + isCustomAddress, + issuanceAmount, + decayedIssuanceAmount, + issuanceHeight, + issuance.getTxId(), + issuanceDate, + cycleIndex)); + } + } + addBurnOutputModel(chainHeight, proofOfBurnOpReturnTxOutputByHash, name, candidate); + }); // Add output receivers of genesis transaction daoStateService.getGenesisTx() @@ -266,15 +262,17 @@ Map> getProofOfBurnOpReturnTxOutputByHas return map; } - private Stream getCompensationProposalsForIssuance(Issuance issuance) { - return proposalService.getProposalPayloads().stream() + private void forEachCompensationIssuance(int chainHeight, BiConsumer action) { + proposalService.getProposalPayloads().stream() .map(ProposalPayload::getProposal) - .filter(proposal -> issuance.getTxId().equals(proposal.getTxId())) .filter(proposal -> proposal instanceof CompensationProposal) - .map(proposal -> (CompensationProposal) proposal); + .flatMap(proposal -> daoStateService.getIssuance(proposal.getTxId()) + .filter(issuance -> issuance.getChainHeight() <= chainHeight) + .map(issuance -> new Tuple2<>(issuance, (CompensationProposal) proposal)) + .stream()) + .forEach(pair -> action.accept(pair.first, pair.second)); } - private String getAddressFromCompensationRequest(Tx tx) { ImmutableList txOutputs = tx.getTxOutputs(); // The compensation request tx has usually 4 outputs. If there is no BTC change its 3 outputs. diff --git a/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java b/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java index 97f3fb03d0c..8f45e840604 100644 --- a/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java +++ b/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java @@ -120,10 +120,12 @@ private void addProofOfBurnTxs(Tx... txs) { } private void addCompensationIssuanceAndPayloads(Collection> tuples) { - when(daoStateService.getIssuanceSetForType(IssuanceType.COMPENSATION)) - .thenReturn(tuples.stream().map(t -> t.first).collect(Collectors.toSet())); + var issuanceMap = tuples.stream() + .collect(Collectors.toMap(t -> t.first.getTxId(), t -> t.first)); when(proposalService.getProposalPayloads()) .thenReturn(tuples.stream().map(t -> t.second).collect(Collectors.toCollection(FXCollections::observableArrayList))); + when(daoStateService.getIssuance(Mockito.anyString())) + .thenAnswer((Answer>) inv -> Optional.ofNullable(issuanceMap.get(inv.getArgument(0, String.class)))); } @SafeVarargs From aa7aedd0b1fefdabe482129b768c8be72b7b39ae Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Sat, 13 Jul 2024 16:52:19 +0200 Subject: [PATCH 2/6] Make getActiveBurningManCandidates return a list instead of a set This avoids needless hashing & equality comparisons of instances of 'BurningManCandidate', which are quite large mutable objects (so should probably use reference equality anyway, and not be used as keys). Also rearrange a couple of (package) private methods. --- .../dao/burningman/BtcFeeReceiverService.java | 3 +- .../dao/burningman/BurningManService.java | 68 +++++++++---------- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java b/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java index a4ef0a9ed62..012eca75b4a 100644 --- a/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java +++ b/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java @@ -27,7 +27,6 @@ import com.google.common.annotations.VisibleForTesting; -import java.util.ArrayList; import java.util.List; import java.util.Random; import java.util.stream.Collectors; @@ -69,7 +68,7 @@ private void applyBlock(Block block) { /////////////////////////////////////////////////////////////////////////////////////////// public String getAddress() { - List activeBurningManCandidates = new ArrayList<>(burningManService.getActiveBurningManCandidates(currentChainHeight)); + List activeBurningManCandidates = burningManService.getActiveBurningManCandidates(currentChainHeight); if (activeBurningManCandidates.isEmpty()) { // If there are no compensation requests (e.g. at dev testing) we fall back to the default address return burningManService.getLegacyBurningManAddress(currentChainHeight); diff --git a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java index 3236234daf9..fb782db1ce9 100644 --- a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java +++ b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java @@ -205,52 +205,21 @@ Map getBurningManCandidatesByName(int chainHeight, return burningManCandidatesByName; } - private static int imposeCaps(Collection burningManCandidates, boolean limitCappingRounds) { - List candidatesInDescendingBurnCapRatio = new ArrayList<>(burningManCandidates); - candidatesInDescendingBurnCapRatio.sort(Comparator.comparing(BurningManCandidate::getBurnCapRatio).reversed()); - double thresholdBurnCapRatio = 1.0; - double remainingBurnShare = 1.0; - double remainingCapShare = 1.0; - int cappingRound = 0; - for (BurningManCandidate candidate : candidatesInDescendingBurnCapRatio) { - double invScaleFactor = remainingBurnShare / remainingCapShare; - double burnCapRatio = candidate.getBurnCapRatio(); - if (remainingCapShare <= 0.0 || burnCapRatio <= 0.0 || burnCapRatio < invScaleFactor || - limitCappingRounds && burnCapRatio < 1.0) { - cappingRound++; - break; - } - if (burnCapRatio < thresholdBurnCapRatio) { - thresholdBurnCapRatio = invScaleFactor; - cappingRound++; - } - candidate.imposeCap(cappingRound, candidate.getBurnAmountShare() / thresholdBurnCapRatio); - remainingBurnShare -= candidate.getBurnAmountShare(); - remainingCapShare -= candidate.getMaxBoostedCompensationShare(); - } - return cappingRound; - } - String getLegacyBurningManAddress(int chainHeight) { return daoStateService.getParamValue(Param.RECIPIENT_BTC_ADDRESS, chainHeight); } - Set getActiveBurningManCandidates(int chainHeight) { + List getActiveBurningManCandidates(int chainHeight) { return getActiveBurningManCandidates(chainHeight, !DelayedPayoutTxReceiverService.isProposal412Activated()); } - Set getActiveBurningManCandidates(int chainHeight, boolean limitCappingRounds) { + List getActiveBurningManCandidates(int chainHeight, boolean limitCappingRounds) { return getBurningManCandidatesByName(chainHeight, limitCappingRounds).values().stream() .filter(burningManCandidate -> burningManCandidate.getCappedBurnAmountShare() > 0) .filter(candidate -> candidate.getReceiverAddress().isPresent()) - .collect(Collectors.toSet()); + .collect(Collectors.toList()); } - - /////////////////////////////////////////////////////////////////////////////////////////// - // Private - /////////////////////////////////////////////////////////////////////////////////////////// - Map> getProofOfBurnOpReturnTxOutputByHash(int chainHeight) { Map> map = new HashMap<>(); daoStateService.getProofOfBurnOpReturnTxOutputs().stream() @@ -262,6 +231,11 @@ Map> getProofOfBurnOpReturnTxOutputByHas return map; } + + /////////////////////////////////////////////////////////////////////////////////////////// + // Private + /////////////////////////////////////////////////////////////////////////////////////////// + private void forEachCompensationIssuance(int chainHeight, BiConsumer action) { proposalService.getProposalPayloads().stream() .map(ProposalPayload::getProposal) @@ -378,4 +352,30 @@ private long getDecayedBurnedAmount(long amount, int issuanceHeight, int chainHe private long getDecayedGenesisOutputAmount(long amount) { return Math.round(amount * GENESIS_OUTPUT_AMOUNT_FACTOR); } + + private static int imposeCaps(Collection burningManCandidates, boolean limitCappingRounds) { + List candidatesInDescendingBurnCapRatio = new ArrayList<>(burningManCandidates); + candidatesInDescendingBurnCapRatio.sort(Comparator.comparing(BurningManCandidate::getBurnCapRatio).reversed()); + double thresholdBurnCapRatio = 1.0; + double remainingBurnShare = 1.0; + double remainingCapShare = 1.0; + int cappingRound = 0; + for (BurningManCandidate candidate : candidatesInDescendingBurnCapRatio) { + double invScaleFactor = remainingBurnShare / remainingCapShare; + double burnCapRatio = candidate.getBurnCapRatio(); + if (remainingCapShare <= 0.0 || burnCapRatio <= 0.0 || burnCapRatio < invScaleFactor || + limitCappingRounds && burnCapRatio < 1.0) { + cappingRound++; + break; + } + if (burnCapRatio < thresholdBurnCapRatio) { + thresholdBurnCapRatio = invScaleFactor; + cappingRound++; + } + candidate.imposeCap(cappingRound, candidate.getBurnAmountShare() / thresholdBurnCapRatio); + remainingBurnShare -= candidate.getBurnAmountShare(); + remainingCapShare -= candidate.getMaxBoostedCompensationShare(); + } + return cappingRound; + } } From 508ab1f8e25670764c7d1717936d326b4244ce92 Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Sat, 13 Jul 2024 19:03:37 +0200 Subject: [PATCH 3/6] Make receiver address deterministic if >1 comp. requests in cycle Handle the exceptional case of a receiver address chosen from a cycle where the candidate somehow got more than one compensation proposal accepted. Either the last custom address or first issuance change address is supposed to be chosen for the receiver address, but in case of a tie at that vote result height, take the address that comes first in lexicographic order. --- .../burningman/model/BurningManCandidate.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java b/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java index ed65a207df0..4ceb867f89e 100644 --- a/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java +++ b/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java @@ -32,6 +32,7 @@ import java.util.Set; import java.util.stream.Collectors; +import lombok.AccessLevel; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -56,6 +57,7 @@ public class BurningManCandidate { // enforce the version by the filter to ensure users have updated. // See: https://github.com/bisq-network/bisq/issues/6699 @EqualsAndHashCode.Exclude + @Getter(AccessLevel.NONE) protected Optional mostRecentAddress = Optional.empty(); private final Set burnOutputModels = new HashSet<>(); @@ -82,11 +84,6 @@ public Optional getReceiverAddress(boolean isBugfix6699Activated) { return isBugfix6699Activated ? receiverAddress : mostRecentAddress; } - public Optional getMostRecentAddress() { - // Lombok getter is set for class, so we would get a getMostRecentAddress but want to ensure it's not accidentally used. - throw new UnsupportedOperationException("getMostRecentAddress must not be used. Use getReceiverAddress instead."); - } - public void addBurnOutputModel(BurnOutputModel burnOutputModel) { if (burnOutputModels.contains(burnOutputModel)) { return; @@ -115,16 +112,20 @@ public void addCompensationModel(CompensationModel compensationModel) { .anyMatch(CompensationModel::isCustomAddress); if (hasAnyCustomAddress) { // If any custom address was defined, we only consider custom addresses and sort them to take the - // most recent one. + // most recent one. If more than one compensation request from a candidate somehow got accepted in + // the same cycle, break the tie by choosing the lexicographically smallest custom address. receiverAddress = compensationModels.stream() .filter(CompensationModel::isCustomAddress) - .max(Comparator.comparing(CompensationModel::getHeight)) + .max(Comparator.comparing(CompensationModel::getHeight) + .thenComparing(Comparator.comparing(CompensationModel::getAddress).reversed())) .map(CompensationModel::getAddress); } else { // If no custom addresses ever have been defined, we take the change address of the compensation request - // and use the earliest address. This helps to avoid change of address with every new comp. request. + // and use the earliest address (similarly taking the lexicographically smallest in the unlikely case of + // a tie). This helps to avoid change of address with every new comp. request. receiverAddress = compensationModels.stream() - .min(Comparator.comparing(CompensationModel::getHeight)) + .min(Comparator.comparing(CompensationModel::getHeight) + .thenComparing(CompensationModel::getAddress)) .map(CompensationModel::getAddress); } From c52fe0e605ea357fd25e8806e22f216cb89b5972 Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Sun, 14 Jul 2024 17:29:06 +0200 Subject: [PATCH 4/6] Remove date checks for Bugfix 6699 & Proposal 412 activation These are both redundant now and will always return true. Also add a missing past check for Proposal 412 activation to 'RefundManager', instead of just defaulting to the current date, in case of any very old disputes involving DPTs created prior to the activation. --- .../dao/burningman/BtcFeeReceiverService.java | 4 +--- .../dao/burningman/BurningManService.java | 4 ++-- .../DelayedPayoutTxReceiverService.java | 24 ++++++++----------- .../burningman/model/BurningManCandidate.java | 3 +-- .../support/dispute/refund/RefundManager.java | 7 +++--- 5 files changed, 18 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java b/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java index 012eca75b4a..a83dce82bd2 100644 --- a/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java +++ b/core/src/main/java/bisq/core/dao/burningman/BtcFeeReceiverService.java @@ -96,9 +96,7 @@ public String getAddress() { // the burningManCandidates as we added for the legacy BM an entry at the end. return burningManService.getLegacyBurningManAddress(currentChainHeight); } - // For the fee selection we do not need to wait for activation date of the bugfix for - // the receiver address (https://github.com/bisq-network/bisq/issues/6699) as it has no impact on the trade protocol. - return activeBurningManCandidates.get(winnerIndex).getReceiverAddress(true) + return activeBurningManCandidates.get(winnerIndex).getReceiverAddress() .orElse(burningManService.getLegacyBurningManAddress(currentChainHeight)); } diff --git a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java index fb782db1ce9..eb5c7a7a1e7 100644 --- a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java +++ b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java @@ -116,7 +116,7 @@ public BurningManService(DaoStateService daoStateService, /////////////////////////////////////////////////////////////////////////////////////////// Map getBurningManCandidatesByName(int chainHeight) { - return getBurningManCandidatesByName(chainHeight, !DelayedPayoutTxReceiverService.isProposal412Activated()); + return getBurningManCandidatesByName(chainHeight, false); } Map getBurningManCandidatesByName(int chainHeight, boolean limitCappingRounds) { @@ -210,7 +210,7 @@ String getLegacyBurningManAddress(int chainHeight) { } List getActiveBurningManCandidates(int chainHeight) { - return getActiveBurningManCandidates(chainHeight, !DelayedPayoutTxReceiverService.isProposal412Activated()); + return getActiveBurningManCandidates(chainHeight, false); } List getActiveBurningManCandidates(int chainHeight, boolean limitCappingRounds) { diff --git a/core/src/main/java/bisq/core/dao/burningman/DelayedPayoutTxReceiverService.java b/core/src/main/java/bisq/core/dao/burningman/DelayedPayoutTxReceiverService.java index 0917587538d..c30f0c8a3fb 100644 --- a/core/src/main/java/bisq/core/dao/burningman/DelayedPayoutTxReceiverService.java +++ b/core/src/main/java/bisq/core/dao/burningman/DelayedPayoutTxReceiverService.java @@ -59,15 +59,6 @@ public class DelayedPayoutTxReceiverService implements DaoStateListener { // See: https://github.com/bisq-network/proposals/issues/412 public static final Date PROPOSAL_412_ACTIVATION_DATE = Utilities.getUTCDate(2024, GregorianCalendar.MAY, 1); - public static boolean isBugfix6699Activated() { - return new Date().after(BUGFIX_6699_ACTIVATION_DATE); - } - - @SuppressWarnings("BooleanMethodIsAlwaysInverted") - public static boolean isProposal412Activated() { - return new Date().after(PROPOSAL_412_ACTIVATION_DATE); - } - // We don't allow to get further back than 767950 (the block height from Dec. 18th 2022). static final int MIN_SNAPSHOT_HEIGHT = Config.baseCurrencyNetwork().isRegtest() ? 0 : 767950; @@ -131,15 +122,17 @@ public int getBurningManSelectionHeight() { public List> getReceivers(int burningManSelectionHeight, long inputAmount, long tradeTxFee) { - return getReceivers(burningManSelectionHeight, inputAmount, tradeTxFee, isBugfix6699Activated()); + return getReceivers(burningManSelectionHeight, inputAmount, tradeTxFee, true, true); } public List> getReceivers(int burningManSelectionHeight, long inputAmount, long tradeTxFee, - boolean isBugfix6699Activated) { + boolean isBugfix6699Activated, + boolean isProposal412Activated) { checkArgument(burningManSelectionHeight >= MIN_SNAPSHOT_HEIGHT, "Selection height must be >= " + MIN_SNAPSHOT_HEIGHT); - Collection burningManCandidates = burningManService.getActiveBurningManCandidates(burningManSelectionHeight); + Collection burningManCandidates = burningManService.getActiveBurningManCandidates(burningManSelectionHeight, + !isProposal412Activated); // We need to use the same txFeePerVbyte value for both traders. // We use the tradeTxFee value which is calculated from the average of taker fee tx size and deposit tx size. @@ -162,8 +155,8 @@ public List> getReceivers(int burningManSelectionHeight, } long spendableAmount = getSpendableAmount(burningManCandidates.size(), inputAmount, txFeePerVbyte); - // We only use outputs > 1000 sat or at least 2 times the cost for the output (32 bytes). - // If we remove outputs it will be spent as miner fee. + // We only use outputs >= 1000 sat or at least 2 times the cost for the output (32 bytes). + // If we remove outputs it will be distributed to the remaining receivers. long minOutputAmount = Math.max(DPT_MIN_OUTPUT_AMOUNT, txFeePerVbyte * 32 * 2); // Sanity check that max share of a non-legacy BM is 20% over MAX_BURN_SHARE (taking into account potential increase due adjustment) long maxOutputAmount = Math.round(spendableAmount * (BurningManService.MAX_BURN_SHARE * 1.2)); @@ -178,6 +171,9 @@ public List> getReceivers(int burningManSelectionHeight, }) .sum(); + // FIXME: The small outputs should be filtered out before adjustment, not afterwards. Otherwise, outputs of + // amount just under 1000 sats or 64 * fee-rate could get erroneously included and lead to significant + // underpaying of the DPT (by perhaps around 5-10% per erroneously included output). List> receivers = burningManCandidates.stream() .filter(candidate -> candidate.getReceiverAddress(isBugfix6699Activated).isPresent()) .map(candidate -> { diff --git a/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java b/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java index 4ceb867f89e..0367430a33d 100644 --- a/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java +++ b/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java @@ -18,7 +18,6 @@ package bisq.core.dao.burningman.model; import bisq.core.dao.burningman.BurningManService; -import bisq.core.dao.burningman.DelayedPayoutTxReceiverService; import bisq.common.util.DateUtil; @@ -77,7 +76,7 @@ public BurningManCandidate() { } public Optional getReceiverAddress() { - return getReceiverAddress(DelayedPayoutTxReceiverService.isBugfix6699Activated()); + return getReceiverAddress(true); } public Optional getReceiverAddress(boolean isBugfix6699Activated) { diff --git a/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java b/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java index 0108eb24023..800deb88f31 100644 --- a/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java +++ b/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java @@ -56,7 +56,6 @@ import org.bitcoinj.core.NetworkParameters; import org.bitcoinj.core.Transaction; -import org.bitcoinj.core.TransactionConfidence; import org.bitcoinj.core.TransactionInput; import org.bitcoinj.core.TransactionOutPoint; import org.bitcoinj.core.TransactionOutput; @@ -195,7 +194,7 @@ public void onDisputeResultMessage(DisputeResultMessage disputeResultMessage) { checkNotNull(chatMessage, "chatMessage must not be null"); Optional disputeOptional = findDispute(disputeResult); String uid = disputeResultMessage.getUid(); - if (!disputeOptional.isPresent()) { + if (disputeOptional.isEmpty()) { log.warn("We got a dispute result msg but we don't have a matching dispute. " + "That might happen when we get the disputeResultMessage before the dispute was created. " + "We try again after 2 sec. to apply the disputeResultMessage. TradeId = " + tradeId); @@ -333,11 +332,13 @@ public void verifyDelayedPayoutTxReceivers(Transaction delayedPayoutTx, Dispute int selectionHeight = dispute.getBurningManSelectionHeight(); boolean wasBugfix6699ActivatedAtTradeDate = dispute.getTradeDate().after(DelayedPayoutTxReceiverService.BUGFIX_6699_ACTIVATION_DATE); + boolean wasProposal412ActivatedAtTradeDate = dispute.getTradeDate().after(DelayedPayoutTxReceiverService.PROPOSAL_412_ACTIVATION_DATE); List> delayedPayoutTxReceivers = delayedPayoutTxReceiverService.getReceivers( selectionHeight, inputAmount, dispute.getTradeTxFee(), - wasBugfix6699ActivatedAtTradeDate); + wasBugfix6699ActivatedAtTradeDate, + wasProposal412ActivatedAtTradeDate); log.info("Verify delayedPayoutTx using selectionHeight {} and receivers {}", selectionHeight, delayedPayoutTxReceivers); checkArgument(delayedPayoutTx.getOutputs().size() == delayedPayoutTxReceivers.size(), "Size of outputs and delayedPayoutTxReceivers must be the same"); From 99165e7bd9dc28e213f250002e97ae1f3f781a02 Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Sun, 14 Jul 2024 18:45:09 +0200 Subject: [PATCH 5/6] Check receiver address validity when calculating burn shares Set the burn cap of a candidate to zero if he has an invalid receiver address, that is, one that bitcoinj cannot parse. This prevents trade failure when creating the DPT, by making such BM inactive and distributing their share to the other BM. (Setting the burn cap to zero is a little more robust than simply filtering out such candidates, as 'BurningManService' handles subsequent share redistribution better than '(DelayedPayoutTx|BtcFee)ReceiverService'.) While this case should normally never occur, due to UI validation of the custom receiver address, there are at least two ways a BM could invalidate his own receiver address if so inclined: 1) He could simply bypass the UI validation; 2) He could manually create a compensation issuance tx with a change address type unrecognised by bitcoinj, such as P2TR, as the address field is pulled straight from the RPC JSON by each full DAO node. Thus, it is necessary to check both change and custom addresses. --- .../dao/burningman/BurningManService.java | 2 +- .../burningman/model/BurningManCandidate.java | 29 +++++++++++- .../dao/burningman/BurningManServiceTest.java | 45 ++++++++++++++++++- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java index eb5c7a7a1e7..70bdc8fffdb 100644 --- a/core/src/main/java/bisq/core/dao/burningman/BurningManService.java +++ b/core/src/main/java/bisq/core/dao/burningman/BurningManService.java @@ -216,7 +216,7 @@ List getActiveBurningManCandidates(int chainHeight) { List getActiveBurningManCandidates(int chainHeight, boolean limitCappingRounds) { return getBurningManCandidatesByName(chainHeight, limitCappingRounds).values().stream() .filter(burningManCandidate -> burningManCandidate.getCappedBurnAmountShare() > 0) - .filter(candidate -> candidate.getReceiverAddress().isPresent()) + .filter(BurningManCandidate::isReceiverAddressValid) .collect(Collectors.toList()); } diff --git a/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java b/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java index 0367430a33d..3501e83fb78 100644 --- a/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java +++ b/core/src/main/java/bisq/core/dao/burningman/model/BurningManCandidate.java @@ -18,6 +18,7 @@ package bisq.core.dao.burningman.model; import bisq.core.dao.burningman.BurningManService; +import bisq.core.util.validation.BtcAddressValidator; import bisq.common.util.DateUtil; @@ -36,6 +37,8 @@ import lombok.Getter; import lombok.extern.slf4j.Slf4j; +import javax.annotation.Nullable; + /** * Contains all relevant data for a burningman candidate (any contributor who has made a compensation request or was * a receiver of a genesis output). @@ -50,9 +53,13 @@ public class BurningManCandidate { private double compensationShare; // Share of accumulated decayed compensation amounts in relation to total issued amounts protected Optional receiverAddress = Optional.empty(); + @EqualsAndHashCode.Exclude + @Getter(AccessLevel.NONE) + @Nullable + private Boolean receiverAddressValid; // For deploying a bugfix with mostRecentAddress we need to maintain the old version to avoid breaking the - // trade protocol. We use the legacyMostRecentAddress until the activation date where we + // trade protocol. We use the legacy mostRecentAddress until the activation date where we // enforce the version by the filter to ensure users have updated. // See: https://github.com/bisq-network/bisq/issues/6699 @EqualsAndHashCode.Exclude @@ -106,6 +113,7 @@ public void addCompensationModel(CompensationModel compensationModel) { accumulatedDecayedCompensationAmount += compensationModel.getDecayedAmount(); accumulatedCompensationAmount += compensationModel.getAmount(); + receiverAddressValid = null; boolean hasAnyCustomAddress = compensationModels.stream() .anyMatch(CompensationModel::isCustomAddress); @@ -135,6 +143,17 @@ public void addCompensationModel(CompensationModel compensationModel) { .map(CompensationModel::getAddress); } + public boolean isReceiverAddressValid() { + // Since address parsing is a little slow (due to use of exception control flow in bitcoinj), cache the + // result of the validation check and clear the cache for every compensation model added to the candidate. + Boolean receiverAddressValid = this.receiverAddressValid; + if (receiverAddressValid == null) { + BtcAddressValidator validator = new BtcAddressValidator(); + this.receiverAddressValid = receiverAddressValid = validator.validate(receiverAddress.orElse(null)).isValid; + } + return receiverAddressValid; + } + public Set getAllAddresses() { return compensationModels.stream().map(CompensationModel::getAddress).collect(Collectors.toSet()); } @@ -196,7 +215,12 @@ public double getBurnCapRatio() { } public double getMaxBoostedCompensationShare() { - return Math.min(BurningManService.MAX_BURN_SHARE, compensationShare * BurningManService.ISSUANCE_BOOST_FACTOR); + // Set the burn cap to zero if the receiver address is missing or invalid (which can never + // happen by accident, due to checks in the UI). This is preferable to simply excluding such + // receivers from the active burning men, as it minimises the chance of funds going to the LBM, + // or DPT outputs failing to pass a sanity check after redistributing the receiver's share. + return isReceiverAddressValid() ? + Math.min(BurningManService.MAX_BURN_SHARE, compensationShare * BurningManService.ISSUANCE_BOOST_FACTOR) : 0.0; } @Override @@ -207,6 +231,7 @@ public String toString() { ",\r\n accumulatedDecayedCompensationAmount=" + accumulatedDecayedCompensationAmount + ",\r\n compensationShare=" + compensationShare + ",\r\n receiverAddress=" + receiverAddress + + ",\r\n receiverAddressValid=" + isReceiverAddressValid() + ",\r\n mostRecentAddress=" + mostRecentAddress + ",\r\n burnOutputModels=" + burnOutputModels + ",\r\n accumulatedBurnAmount=" + accumulatedBurnAmount + diff --git a/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java b/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java index 8f45e840604..7f95745aa48 100644 --- a/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java +++ b/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java @@ -27,6 +27,7 @@ import bisq.core.dao.state.model.governance.CompensationProposal; import bisq.core.dao.state.model.governance.Issuance; import bisq.core.dao.state.model.governance.IssuanceType; +import bisq.core.locale.Res; import bisq.common.util.Tuple2; @@ -68,6 +69,11 @@ import static org.mockito.Mockito.when; public class BurningManServiceTest { + private static final String VALID_P2SH_ADDRESS = "3AdD7ZaJQw9m1maN39CeJ1zVyXQLn2MEHR"; + private static final String VALID_P2WPKH_ADDRESS = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4"; + private static final String VALID_P2TR_ADDRESS = "bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0"; // unsupported + private static final String INVALID_ADDRESS = "invalid_address"; + @Test public void testGetDecayedAmount() { long amount = 100; @@ -104,6 +110,7 @@ public class BurnShareTest { @BeforeEach public void setUp() { + Res.setup(); when(cyclesInDaoStateService.getChainHeightOfPastCycle(800000, BurningManService.NUM_CYCLES_BURN_AMOUNT_DECAY)) .thenReturn(750000); when(cyclesInDaoStateService.getChainHeightOfPastCycle(800000, BurningManService.NUM_CYCLES_COMP_REQUEST_DECAY)) @@ -133,6 +140,34 @@ private void addCompensationIssuanceAndPayloads(Tuple2 () -> { + assertEquals(0.25, candidate.getBurnAmountShare()); + assertEquals(candidate.getMaxBoostedCompensationShare(), candidate.getCappedBurnAmountShare()); + })); + } + @ValueSource(booleans = {true, false}) @ParameterizedTest(name = "[{index}] limitCappingRounds={0}") public void testGetBurningManCandidatesByName_inactiveAndExpiredCandidates(boolean limitCappingRounds) { @@ -341,8 +376,16 @@ private static Tuple2 compensationIssuanceAndPayload( String txId, int chainHeight, long amount) { + return compensationIssuanceAndPayload(name, txId, chainHeight, amount, VALID_P2WPKH_ADDRESS); + } + + private static Tuple2 compensationIssuanceAndPayload(String name, + String txId, + int chainHeight, + long amount, + String receiverAddress) { var issuance = new Issuance(txId, chainHeight, amount, null, IssuanceType.COMPENSATION); - var extraDataMap = Map.of(CompensationProposal.BURNING_MAN_RECEIVER_ADDRESS, "receiverAddress"); + var extraDataMap = Map.of(CompensationProposal.BURNING_MAN_RECEIVER_ADDRESS, receiverAddress); var proposal = new CompensationProposal(name, "link", Coin.valueOf(amount), "bsqAddress", extraDataMap); return new Tuple2<>(issuance, new ProposalPayload(proposal.cloneProposalAndAddTxId(txId))); } From b10ccd28680213918d786dcfbad357f4eb8e2edc Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 18 Jul 2024 00:14:18 +0200 Subject: [PATCH 6/6] Reject witness >v0 addresses incorrectly encoded as Bech32 Reject any custom receiver address which wasn't encoded as Bech32m if the witness program version is greater than zero. These are currently accepted by bitcoinj but are now invalid and would fail to parse if our fork was updated to understand Bech32m, to support sending to P2TR addresses, which the upstream version appears to. (Thus, the presence of such malformed receivers would not be an issue at present, but might cause complications in the future.) --- .../core/util/validation/BtcAddressValidator.java | 4 ++-- .../core/dao/burningman/BurningManServiceTest.java | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/bisq/core/util/validation/BtcAddressValidator.java b/core/src/main/java/bisq/core/util/validation/BtcAddressValidator.java index 2820d5d81ae..7eb74025cb6 100644 --- a/core/src/main/java/bisq/core/util/validation/BtcAddressValidator.java +++ b/core/src/main/java/bisq/core/util/validation/BtcAddressValidator.java @@ -47,9 +47,9 @@ private ValidationResult validateBtcAddress(String input) { return new ValidationResult(true); } try { - Address.fromString(Config.baseCurrencyNetworkParameters(), input); + Address.fromString(Config.baseCurrencyNetworkParameters(), input).getOutputScriptType(); return new ValidationResult(true); - } catch (AddressFormatException e) { + } catch (AddressFormatException | IllegalStateException e) { return new ValidationResult(false, Res.get("validation.btc.invalidFormat")); } } diff --git a/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java b/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java index 7f95745aa48..25167697a61 100644 --- a/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java +++ b/core/src/test/java/bisq/core/dao/burningman/BurningManServiceTest.java @@ -73,6 +73,9 @@ public class BurningManServiceTest { private static final String VALID_P2WPKH_ADDRESS = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4"; private static final String VALID_P2TR_ADDRESS = "bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0"; // unsupported private static final String INVALID_ADDRESS = "invalid_address"; + // Valid Bech32 encoding of a witness v2 program, which is standard in outputs, but anyone-can-spend (as of 2024). + // Bitcoinj should be upgraded to reject such addresses, as only Bech32m should be used for witness v1 and above. + private static final String BADLY_ENCODED_ADDRESS = "bc1zw508d6qejxtdg4y5r3zarvaryvg6kdaj"; @Test public void testGetDecayedAmount() { @@ -147,13 +150,15 @@ public void testGetBurningManCandidatesByName_invalidReceiverAddresses(boolean l compensationIssuanceAndPayload("alice", "0000", 790000, 10000, VALID_P2SH_ADDRESS), compensationIssuanceAndPayload("bob", "0001", 790000, 10000, VALID_P2WPKH_ADDRESS), compensationIssuanceAndPayload("carol", "0002", 790000, 10000, VALID_P2TR_ADDRESS), - compensationIssuanceAndPayload("dave", "0003", 790000, 10000, INVALID_ADDRESS) + compensationIssuanceAndPayload("dave", "0003", 790000, 10000, INVALID_ADDRESS), + compensationIssuanceAndPayload("earl", "0004", 790000, 10000, BADLY_ENCODED_ADDRESS) ); addProofOfBurnTxs( proofOfBurnTx("alice", "1000", 790000, 10000), proofOfBurnTx("bob", "1001", 790000, 10000), proofOfBurnTx("carol", "1002", 790000, 10000), - proofOfBurnTx("dave", "1003", 790000, 10000) + proofOfBurnTx("dave", "1003", 790000, 10000), + proofOfBurnTx("earl", "1004", 790000, 10000) ); var candidateMap = burningManService.getBurningManCandidatesByName(800000, limitCappingRounds); @@ -161,9 +166,10 @@ public void testGetBurningManCandidatesByName_invalidReceiverAddresses(boolean l assertEquals(0.11, candidateMap.get("bob").getMaxBoostedCompensationShare()); assertEquals(0.0, candidateMap.get("carol").getMaxBoostedCompensationShare()); assertEquals(0.0, candidateMap.get("dave").getMaxBoostedCompensationShare()); + assertEquals(0.0, candidateMap.get("earl").getMaxBoostedCompensationShare()); assertAll(candidateMap.values().stream().map(candidate -> () -> { - assertEquals(0.25, candidate.getBurnAmountShare()); + assertEquals(0.2, candidate.getBurnAmountShare()); assertEquals(candidate.getMaxBoostedCompensationShare(), candidate.getCappedBurnAmountShare()); })); }