[WIP] Add list to filter to set btc fee receiver addresses

[chimp1984]

This adds support for distributing trade fees in BTC to a list of addresses defined in the Filter object. The Filter can be set dynamically via the P2P network so it is a fast tool to adjust once the list should be altered. In case no values are provided or the user has not updated the default donation address from the DAO param is used.

This might be an option how we can handle the losses of the victims from the recent security issue. If the idea finds support among Bisq contributors and is accepted by the victims this would be a way how they get reimbursed over a period of time from the trade fees paid by users who use BTC instead of BSQ.

The selection is a simple random selection, so it would benefit those victims with smaller losses to get refunded earlier. We could alter the algorithm by using the index in the list as weight, so the first items have higher chances to get selected and thus create a more fair payout schedule. I leave this for a future addition if the idea finds support.

[cbeams]

bisq-network/proposals#209 was approved in Cycle 12, and indicates that we should use the final form of this PR to repay victims of the security incident.

Per bisq-network/proposals#209 (comment), we need reviews on this PR and to do whatever is necessary to get it into production quality. @ripcurlx, can you own making sure we get this done? (not necessarily that you do it yourself).

Note that the specific bitcoin addresses we'll be using for repayments are being collected at bisq-network/admin#76.

[cbeams]

Closely related to this is the mechanism by which we'll track repayments to each victim's address. See bisq-network/admin#77 for details, especially if you're interested in implementing it.

[stejbac]

I'm happy to help review and test the code - just having a look through it now.

[chimp1984]

We should make it clear to victims to use a Bitcoin Core address and not a Bisq address as they will receive a lot of small transactions and the Bisq wallet is not good at handling that and will become very heavy over time. Also other server based solutions should be avoided (even Trezor as they request the transactions from their server and it gets very slow as well with lots of txs). So safest way is to use Bitcoin Core IMO.

Also important that this address is not used for anything else by the victims (as stated at bisq-network/admin#77 (comment)).

The current algorithm is not fair to the weight of the payout. Its simple random selection so victims with smaller amounts would get earlier bailed out. I suggest a tuple of address and weight (can be derived from total amount the victim should receive).

Please note that this draft PR was kind of "poof of concept" and was not considered production quality (even it might be only the selection algorithm which requires a change).

[cd2357]

@chimp1984 makes some good points, which I think are hard to fulfill in an algorithm.

Taking a step back though, why not just pay these funds out manually? Seems to achieve the same effect, with much less effort, complexity and risk.

Arguments for:

• There are only 6 people affected
• It would also make tracking the payments easier
• more flexibility in
  • how much budget is used for this refund per cycle (Repay victims of the April 2020 security incident using BTC trading fees proposals#209 (comment))
  • who gets how much when (see concerns about fairness of of payout algo in PR description)
• if it's done by a human (and not by an algorithm), the "refund agent" could also talk to those people and cover @chimp1984 's points above (type of address, etc)

Haven't followed this discussion since the beginning (DAO proposal, etc) so I might be missing smth. And I surely don't have the full picture here. But just asking.

[cbeams]

We should make it clear to victims to use a Bitcoin Core address and not a Bisq address

Good point. I've just reached out to all victims about this.

[cbeams]

Both @sqrrm and @stejbac have agreed to drive the review and testing this implementation and have been assigned accordingly as reviewers. This work should also include implementing any weighting in the algorithm as suggested by @chimp1984 above.

[cbeams]

@cd2357 wrote:

why not just pay these funds out manually? Seems to achieve the same effect, with much less effort, complexity and risk.

The main reason is that it puts the manual distributor of those funds in a potentially sensitive legal position. None of us are lawyers, but we're always trying to avoid this kind of situation.

Arguments for:

• There are only 6 people affected

True; doing a single, manual, end-of-cycle batch payment to these six addresses wouldn't be hard.

• It would also make tracking the payments easier

Payment tracking is already handled by bisq-network/admin#77; there's nothing that need be made easier.

• more flexibility in how much budget is used for this refund per cycle (bisq-network/proposals#209 (comment))

This is a good point. It's basically an unresolved issue how we will manage this effectively under the current plan.

• more flexibility in who gets how much when (see concerns about fairness of of payout algo in PR description)

I'd argue this should still be algorithmic / deterministic / rules-based even if payouts are being done manually. Still, having a human do it means being able to double-check that payment amounts are actually in line with the fairness intended by those rules.

• if it's done by a human (and not by an algorithm), the "refund agent" could also talk to those people and cover @chimp1984 's points above (type of address, etc)

This isn't an issue. I'm already in communication with all 6 victims and have, as per #4150 (comment), already let them know about chimp's "type of address" comment.

---

In the end, I don't personally have a very strong argument why we shouldn't do this manually on a once-per-cycle basis. bisq-network/proposals#206 suggested a manual approach, but it involved a new "special refund agent" role and was more complicated, involving BSQ.

If the donation address holder were willing to do this work, I don't think I'd have a strong opposition to it. Even though we already decided via a DAO vote to go with the filter-based, automated approach, it's probably not too late to reconsider.

It is attractive to me to avoid the need entirely for this review and testing effort, and to redirect everyone's time to other, more productive activities. Another thing that is attractive about doing it manually is that it avoids concerns about what happens down the road when we're removing addresses that have been fully paid out. There is a certain risk that filter messages may not reach a node, and in the case of an address removal message failing to propagate, that node would keep making payments to the address indefinitely. We're aware of this problem and have acknowledged that we must lock down a solution before any addresses are close to being fully repaid. We should fix the underlying propagation/delivery reliability issue in any case, of course, but it would be good to remove this particular risk if we can.

So I'm open to this idea. I particularly like the fact that we can better manage how much gets paid out at the end of each cycle against our budget limits. I hate to re-open the implementation conversation and have to put this to another vote, but as per bisq-network/admin#78, we have more proposals and voting to do on this matter anyway.

Thoughts? @burningman2, wdyt?

[cd2357]

Thanks @cbeams for the detailed reply.

why not just pay these funds out manually? Seems to achieve the same effect, with much less effort, complexity and risk.

The main reason is that it puts the manual distributor of those funds in a potentially sensitive legal position. None of us are lawyers, but we're always trying to avoid this kind of situation.

Good point, this is already a killer argument (to me at least) against the manual approach.

Although also curious to hear what others think.

[sqrrm]

Might be good to have an end date here that would extend with new versions. That would protect against a faulty filter removal causing fees to go to an address that shouldn't get any.

Weightings could be added to the address to distribute the fees more evenly among the receiving addresses according to amount lost.

[stejbac]

I also think it would be a good idea to add an end date to help prevent overpaying.

With a single end date but no weightings, it would have to be set fairly near in the future to avoid overpaying the smallest amount due, so it might make things easier to administer with weightings as well. That way all the addresses fill up at roughly the same time (though there may be significant random fluctuation in the time taken - perhaps 5-10% as a rough guess).

[stejbac]

There are some missing changes to the unit tests OpenOfferManagerTest and UserPayloadModelVOTest which break the build. Other than that, I couldn't see any obvious problem when testing an Alice and Bob instance on regtest. (I also did some limited smoke testing on mainnet.)

I was not able to get the filter to propagate from one node to another on regtest (using --useDevPrivilegeKeys) and had to add them separately to each Alice/Bob/Mediator instance, though that almost certainly has nothing to do with these changes. I'm still looking into this - possibly intended behaviour of FilterManager?

[stejbac]

The @Getter field annotation here is redundant, as the class is already annotated with @Getter.

[stejbac]

Note: There is a conflict when rebasing onto master, as there was recently a new Filter field added:

repeated string bannedSignerPubKeys = 19;

[stejbac]

I also think it would be a good idea to add an end date to help prevent overpaying.

With a single end date but no weightings, it would have to be set fairly near in the future to avoid overpaying the smallest amount due, so it might make things easier to administer with weightings as well. That way all the addresses fill up at roughly the same time (though there may be significant random fluctuation in the time taken - perhaps 5-10% as a rough guess).

[burningman2]

The main reason is that it puts the manual distributor of those funds in a potentially sensitive legal position. None of us are lawyers, but we're always trying to avoid this kind of situation.

I think that is a strong argument against using the BM for a manual distribution.

Regarding overpayment and filter message not arriving issues:

• I am not aware that there are issues with missed filter messages though that is possible as they are P2P network messages and no guarantee for 100% consistency is given. I doubt its a realistic problem.
• Trade fee payments are tiny micropayments. Even if we overpay for a few days it will not change total amount much and given the open questions regarding conversion rate calculation it should be considered a minor issue if a victim gets 0.0001% more as intended...

[chimp1984]

@stejbac @sqrrm

Here is a quick'n'dirty implementation for a weighted algorithm using a list of address/amount tuples separated with a # (e.g. "3A8Zc1XioE2HRzYfbb5P8iemCS72M6vRJV#1.231, 1BVxNn3T12veSK6DgqwU4Hdn7QHcDDRag7#12.12345678").
Not tested (not even compiled)....
Feel free to make it more elegant but I think it should be pretty simple.

If you think a date restriction is needed you can add another entry to the tuple for the expiry date, after which the address is ignored. I doubt it is needed and think its better to keep it as simple as possible to avoid potential bugs.

  public static String getAddress(DaoFacade daoFacade, FilterManager filterManager) {
        // We keep default value as fallback in case no filter value is available or user has old version.
        String feeReceiver = daoFacade.getParamValue(Param.RECIPIENT_BTC_ADDRESS);

        Filter filter = filterManager.getFilter();
        if (filter != null) {
            List<String> feeReceivers = filter.getBtcFeeReceiverAddresses();
            if (feeReceivers != null && !feeReceivers.isEmpty()) {
                AtomicLong totalSum = new AtomicLong();
                List<Long> amountList = new ArrayList<>();
                List<Tuple2<String, Long>> feeReceiverTupleList = feeReceivers.stream()
                        .map(e -> {
                            try {
                                String[] tokens = e.split("#");
                                String address = tokens[0];                             // Victim's receiver address
                                Long amount = Coin.parseCoin(tokens[1]).longValue();    // Total amount the victim should receive
                                totalSum.addAndGet(amount);
                                amountList.add(amount);
                                return new Tuple2<>(address, amount);
                            } catch (Throwable t) {
                                // If input format is not as expected we ignore entry
                                return null;
                            }
                        })
                        .filter(Objects::nonNull)
                        .collect(Collectors.toList());

                long target = Math.abs(new SecureRandom().nextLong()) % totalSum.get();
                int aggregated = 0;
                for (int i = 0; i < amountList.size(); i++) {
                    aggregated += amountList.get(i);
                    if (target <= aggregated) {
                        feeReceiver = feeReceiverTupleList.get(i).first;
                        break;
                    }
                }
            }
        }
        return feeReceiver;
    }

[stejbac]

I created a patch (on top of 1b86a7d) breaking up the above method into 3 separate methods to make it a bit more testable, by avoiding the RNG nondeterminism. I also added some unit tests and fixed a slight issue with the above that an exception is thrown if all the recipient address+weight pairs are unparseable, instead of just some of them.

Feel free not to use it or to make further changes. I tested the patch on regtest and it seems to work with the weighted addresses. I'm also fairly sure the above would have worked as well.

It seems that my earlier confusion about the filter not being successfully broadcast was due to FilterManager exposing two separate Filter properties: UserPayload.developersFilter (stored locally, shown in the filter view and broadcast when adding a new filter) and FilterManager.filterProperty (loaded by each peer from the network and used for the actual filtering). Adding a filter on one node (say the mediator) did successfully apply it to the receiver address selection on the other nodes upon further testing, so I'm fairly sure there's no problem.

Add_weights_to_fee_receiver_addresses.patch.txt

[cbeams]

Update: @sqrrm will own completing the review process with this PR and getting it merged, and I've assigned him accordingly. This PR is not a candidate for inclusion in the forthcoming v1.3.5, but may be possible to get into v1.3.6.

[sqrrm]

Features merged with #4294