Limit number of unconfirmed offers

[freimair]

Fixes #3705

It seems that Bisq can reach a critical mass of unconfirmed transactions. Exceeding this mass causes all sorts of failures. In the corresponding issue it has been suggested to just limit the number of unconfirmed transactions.

After thorough discussion with @chimp1984, @sqrrm, @wiz and @ripcurlx, we agreed on a first step, namely, to introduce Bisqs own hard limit, get it into the next release and ultimately see if we can reduce support cases (currently 8-10 cases per month out of 2k5 trades)

This PR introduces Bisqs own hard limit of 20 unconfirmed transactions. If the limit is exceeded during

• offer creation at the makers side,
• taking the offer at the takers side,
• and taking the offer at the makers side,
  processes are stopped so that no harm can be done.

Considerations

• Although these errors are non-blocking and not meant to be reportet, the error handling framework does not allow for a more correct interaction with the user. We agreed on (@ripcurlx) creating an independent issue regarding the GUI error handling and keep this PR focused. The error handling is tracked in Fix errorhandling around offercreation and take offer procedures #4079.
• Note that there is hardly any synchronization in there and race conditions are to be expected. First, because of how Bisq uses BitcoinJ, there is no correct sync possible. Second, the hard limit is set to 20 which is less than the actual default hard limit of the bitcoin network (25) to allow for some race conditions. Third, this PR is designed to reduce the number of support requests. If time raises the need for more proper handling, we shall do it then.
• I considered extending the protocol to properly report the error. However, by doing so, newer clients cannot take offers of older clients because of how protobuf works. Again, if the need arises over time, we shall do it.

Discussions, findings and strategies

To all the experts out there, here are some points to be discussed

• does this "hard limit" only affect offers (I guess the fee txs) or does it affect all txs. The question is should we create a more general safety net?
• is that something to be fixed in bitcoinj?

Please note that I am no expert in the BTC parts of the Bisq code plus the PR is currently largely untested. It is just there to get things moving.

EDIT: seems to be related to bisq-network/proposals#132 and #3489

EDIT2: update after talking to @chimp1984 and @sqrrm

• future impact

  • once the API can create offers and trading bots are used, this can get critical fast

• brainstormed strategies

  • report trade fail to trading partner
    • minor changes to the trade protocol implementation (check!)
    • what happens if the trade succeeds afterwards? because transactions are accepted later?
    • need for a cancellation feature
      • how to revert already committed transactions..
  • only publish offers once fee transaction is confirmed
    • hugely impacts usability because it can take a lot of time until transactions get confirmed (eg. 1h to confirmation, 5 offers take 5h to publish (implement a queue?)
    • might take way to long and thus, might break the trade protocol (does it during offer creation?)
    • does not prevent Takers deposit transaction rejected for 'too-long-mempool-chain' causing issues #3705 from happening
  • make best effort to prevent error
    • introduce Bisqs own hard limit for pending transactions
      • check before offer is created
      • check before offer is taken
        • on the takers side
        • on the makers side triggered by OfferAvailableRequest + error code in case we are close to the limit (maybe not necessary (yet), because maker only creates the fee transactions in sequence, multisig transactions coming in later might result in a tree (check!))
    • does not work with external wallet
      • however, someone using an external wallet might not be using the same wallet for multiple trades for privacy
      • if the wallet funding transaction fails, it is out of scope for Bisq? (for now at least?)
  • others?

EDIT3: update after a call with @wiz:

#3705 (comment)

[huey735]

I'm not a coder let alone expert but echoing my comments on #3489 (comment)

I'm vehemently against relaying offers with not-broadcast or unconfirmed maker fee tx.

From my understanding this PR aims to reduce current issues but I as I see we need to first workout the current model in which the network takes the maker's messages for granted without any check that the make-offer transaction has even been properly broadcast.

[freimair]

@huey735 that is a valid point. if I get you that would compile down to

• not broadcasting offers until the makertx has been confirmed
• not accepting offers that have unconfirmed makertx

the hard limit for the offer maker is still there but then, only the maker herself will get a too-long-mempool-chain-error when creating the umpteenth offer in sequence. (?)

[huey735]

@freimair it would go a long way if the network could at least check that they saw the make-offer transaction on their mempools (this could be delegated to the seed nodes). But at the moment even that doesn't happen. So no for offers with unbroadcast make-offer transactions.

As for broadcast but unconfirmed make-offer transactions. This limit may not make sense for the maker?! They are responsible for 1 of the 3 transactions relevant to this issue. If they make 10 offers with all broadcast but unconfirmed transactions and 10 different traders take the offers before they've been confirmed would this issue affect the maker?
@devinbileck description only described the taker's side.

I don't know which transaction of the following the maker keeps track of:

1. make-offer tx
2. take-offer tx
3. multisig deposit

Assuming from the issue this PR is trying to address, the taker carries transactions 2 and 3.

[freimair]

can it have some positive effect if we only broadcast the offer when we see our own maker fee confirmed? And shouldn't we do that anyways?

[huey735]

@freimair Possibly. ping @wiz and @ripcurlx

[wiz]

can it have some positive effect if we only broadcast the offer when we see our own maker fee confirmed? And shouldn't we do that anyways?

Bisq was clearly written with the intention of immediately broadcasting offers with unconfirmed funding TX, which is totally fine as long as the funding TX is actually in the mempool and wasn't rejected by the network. IMO all Bisq nodes should verify this but since they are SPV that's not easy.

[wiz]

@freimair I have another suggestion. Have both parties send the full serialized Bitcoin TX to each other. at the time of offer taking and have both parties re-broadcast the counterparty's TX to their Bitcoin nodes - if they do not receive either a "broadcast successful" or "already broadcasted" response, they know the TX is invalid.

[sqrrm]

utACK