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

Send delayed payouts to the correct BurningMan address #6382

Closed
wants to merge 1 commit into from
Closed

Send delayed payouts to the correct BurningMan address #6382

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 18, 2022

Sending to the old BM address is seen as problematic by support staff e.g. @refund-agent2. This PR fixes it.

Why some delayed payouts are sent to the old burningman:

  • When a trade is initiated, the BTC seller creates/signs the delayed payout tx choosing the destination address by: processModel.getDaoFacade().getParamValue(Param.RECIPIENT_BTC_ADDRESS);
  • The buyer signs and validates it.
  • If the buyer does not agree that the seller's proposed delayed payout tx looks right, they will fail the trade. The buyer allows any prior known value for BM recipient address.

Scenario 1: Seller DAO enabled, Buyer DAO enabled.
The normal case. Both parties agree that 34VLFgtFKAtwTdZ5rengTT2g2zC99sWQLC is the appropriate delayed payout address.

Scenario 2: Seller DAO enabled, Buyer DAO disabled.
Seller creates the delayed payout tx using the current DAO parameter value 34VLFgtFKAtwTdZ5rengTT2g2zC99sWQLC. Buyer will accept it because all known BM addresses are explicitly hard coded. See TradeDataValidation.java#L82-L88 and DaoFacade.java#L790-L792

Scenario 3: Seller DAO disabled, Buyer DAO enabled.
Seller creates the delayed payout tx using the old parameter value of 1BVxNn3T12veSK6DgqwU4Hdn7QHcDDRag7. When the buyer validates, it sees that address is known as one of the past valid DAO values and therefore accepts it. If the trade goes to arbitration the delayed payout is sent to the old burningman.

Scenario 4: Seller DAO disabled, Buyer DAO disabled.
Seller creates the delayed payout tx using the old parameter value of 1BVxNn3T12veSK6DgqwU4Hdn7QHcDDRag7. Buyer will accept it because all known BM addresses are explicitly hard coded. See TradeDataValidation.java#L82-L88 and DaoFacade.java#L790-L792


The problem (scenario 3&4) can be resolved by calling DelayedPayoutAddressProvider.getDelayedPayoutAddress() - which until now is implemented but unused. This PR makes use of it so all scenarios use the most up-to-date BM address.

public static String getDelayedPayoutAddress(DaoFacade daoFacade) {
String address = daoFacade.getParamValue(Param.RECIPIENT_BTC_ADDRESS);
if (isOutdatedAddress(address)) {
log.warn("Outdated delayed payout address. " +
"This can be the case if the DAO is deactivated or if the user has an invalid DAO state." +
"We set the address to the recent one (BM3_ADDRESS).");
return getAddress();
}
return address;
}
public static boolean isOutdatedAddress(String address) {
return address.equals(INITIAL_BM_ADDRESS) ||
address.equals(BM2019_ADDRESS) ||
address.equals(BM2_ADDRESS);
}
public static String getAddress() {
return BM3_ADDRESS;

@ghost
Copy link
Author

ghost commented Oct 18, 2022

Mainnet test results of delayed payout tx (seller DAO disabled)

PRE:

  "vout": [
    {
      "value": 0.00650000,
      "n": 0,
      "scriptPubKey": {
        "asm": "OP_DUP OP_HASH160 732b213f56728395cbc738fb3c8b3d9649aa6729 OP_EQUALVERIFY OP_CHECKSIG",
        "hex": "76a914732b213f56728395cbc738fb3c8b3d9649aa672988ac",
        "reqSigs": 1,
        "type": "pubkeyhash",
        "addresses": [
          "1BVxNn3T12veSK6DgqwU4Hdn7QHcDDRag7"
        ]
      }
    }
  ]

POST:

  "vout": [
    {
      "value": 0.00650000,
      "n": 0,
      "scriptPubKey": {
        "asm": "OP_HASH160 1eb2afdea60e402a78506e75987d188b19462de2 OP_EQUAL",
        "hex": "a9141eb2afdea60e402a78506e75987d188b19462de287",
        "reqSigs": 1,
        "type": "scripthash",
        "addresses": [
          "34VLFgtFKAtwTdZ5rengTT2g2zC99sWQLC"
        ]
      }
    }
  ]

@ripcurlx ripcurlx added this to the v1.9.7 milestone Oct 19, 2022
@HenrikJannsen
Copy link
Collaborator

bisq-network/proposals#390 will make that PR irrelevant.

@ghost ghost closed this Nov 7, 2022
@ripcurlx ripcurlx removed this from the v1.9.7 milestone Dec 12, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants