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

Allow spending of unconfirmed BSQ change outputs #2482

Merged
merged 12 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@ManfredKarrer
Copy link
Member

ManfredKarrer commented Mar 1, 2019

When creating a BSQ transaction (actually at commit time as we can create a tx and then
cancel it in the confirmation popup) we store the change output (only that not the other
possible BSQ output) in a persisted list. The BsqCoinSelector will take that list to
allow spending those coins. We use the txType to find the index of the cahnge output.
We only have one change output in the transactions created in Bisq. Multiple change
outputs would be valid but our goal is only increased usability in the Bisq app and it is
not related to validation rules.

We update out list at each new block confirmation.

With that approach we avoid too much dependencies to the BitcoinJ side.

  • Add UnconfirmedBsqChangeOutputListService and persisted UnconfirmedBsqChangeOutputList
    for storing unconfirmed outputs
  • Add lookup for unconfirmed BSQ change outputs at BsqCoinSelector and allow spending if
    found
  • Pass TxType for walletsManager.publishAndCommitBsqTx calls
  • Add TxType to bsqWalletService.commitTx
  • Refactor getPreparedSendTx methods for BSQ and BTC sending to one common method with a
    coinselector parameter.
  • Add getChangeAddress method to BsqWalletService to make change outputs more explicit
  • Add unconfirmedChangeBalance to onUpdateBalances handlers
  • Rename availableBalance to availableConfirmedBalance in onUpdateBalances
  • Unify onUpdateBalances parameter names
Allow spending of unconfirmed BSQ change outputs
When creating a BSQ transaction (actually at commit time as we can create a tx and then
cancel it in the confirmation popup) we store the change output (only that not the other
possible BSQ output) in a persisted list. The BsqCoinSelector will take that list to
allow spending those coins. We use the txType to find the index of the cahnge output.
We only have one change output in the transactions created in Bisq. Multiple change
outputs would be valid but our goal is only increased usability in the Bisq app and it is
not related to validation rules.

We update out list at each new block confirmation.

With that approach we avoid too much dependencies to the BitcoinJ side.

- Add UnconfirmedBsqChangeOutputListService and persisted UnconfirmedBsqChangeOutputList
for storing unconfirmed outputs
- Add lookup for unconfirmed BSQ change outputs at BsqCoinSelector and allow spending if
found
- Pass TxType for walletsManager.publishAndCommitBsqTx calls
- Add TxType to bsqWalletService.commitTx
- Refactor getPreparedSendTx methods for BSQ and BTC sending to one common method with a
coinselector parameter.
- Add getChangeAddress method to BsqWalletService to make change outputs more explicit
- Add unconfirmedChangeBalance to onUpdateBalances handlers
- Rename availableBalance to availableConfirmedBalance in onUpdateBalances
- Unify onUpdateBalances parameter names

@ManfredKarrer ManfredKarrer requested a review from sqrrm Mar 1, 2019

@sqrrm
Copy link
Member

sqrrm left a comment

I think this is a better way to handle it than through the parser. I tried using the parser and that's just too risky and hard to control in the end, this way is much more controlled and only very specifically designated change outputs can be spent without confirmation and parsing which is safer.

The problem would be that this is more fragile if we start changing anything in the BSQ parsing as this is now a separate kind of parsing that needs to be handled with great care.


// Only if its not existing yet in the dao state (unconfirmed) we use our unconfirmedBsqChangeOutputList to
// check if it is an own change output.
return unconfirmedBsqChangeOutputListService.hasTransactionOutput(output);

This comment has been minimized.

@sqrrm

sqrrm Mar 1, 2019

Member

Don't we need to check that it's also unspent or is the unconfirmed list only for unspent txs?

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Mar 1, 2019

Author Member

As we remove entries for all connected outputs of any inputs we should not get that problem (e.g. user spend utxo from previously unconfirmed tx). If we would miss one it would throw an error on the BitcoinJ level if we try to use an input which is already spent. But I will have a look to add an additional check.

*/
public void onCommitTx(Transaction tx, TxType txType, Wallet wallet) {
// We remove all potential connected outputs from our inputs as they would have been spent.
removeConnectedOutputsOfInputsOfTx(tx);

This comment has been minimized.

@sqrrm

sqrrm Mar 1, 2019

Member

Ok, I see the outputs are removed when spent, previous comment is then void since all transactions in the unconfirmed list are unspent.

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Mar 1, 2019

Author Member

Yes, if data hanlding stays in sync it should be ok. Still as we manage parallel data now there might be theoretically an issue (e.g. if persistence fails after remove). I will try to test that scenario to verify that we get an exception from BitcoinJ.

Show resolved Hide resolved ...sq/core/dao/state/unconfirmed/UnconfirmedBsqChangeOutputListService.java Outdated
Show resolved Hide resolved ...sq/core/dao/state/unconfirmed/UnconfirmedBsqChangeOutputListService.java

@ManfredKarrer ManfredKarrer added the in:dao label Mar 1, 2019

@ManfredKarrer ManfredKarrer self-assigned this Mar 1, 2019

@ManfredKarrer ManfredKarrer added this to the v0.9.5 milestone Mar 1, 2019

ManfredKarrer added some commits Mar 1, 2019

@ManfredKarrer ManfredKarrer requested a review from ripcurlx Mar 3, 2019

ManfredKarrer added some commits Mar 3, 2019

Fix BSQ balance display for unconfirmed change outputs
- Add verifiedBsqBalance and unconfirmedChangeBalance
- Remove totalBsqBalance
- Make text for different balances more explicit
Fix balance update in case at vote result. Rename methods
- To ensure the balance is updated in case we have a comp. request we
need to ensure that the vote result is completed before our balance
update is called.

- Remove updateBsqWalletTransactions call at constructor as nothing is
ready anyway here

- Refactor: Rename addBsqStateListener to addDaoStateListener
and removeBsqStateListener to removeDaoStateListener
Refactor: Rename onParseTxsCompleteAfterBatchProcessing to
onParseBlockCompleteAfterBatchProcessing

@ManfredKarrer ManfredKarrer marked this pull request as ready for review Mar 3, 2019

@ManfredKarrer

This comment has been minimized.

Copy link
Member Author

ManfredKarrer commented Mar 3, 2019

@sqrrm I fixed and improved balance display as well as hanlding of confiscated bonds. Can you review the open commits?

@ManfredKarrer ManfredKarrer requested a review from devinbileck Mar 4, 2019

@ripcurlx
Copy link
Member

ripcurlx left a comment

NACK - Besides the validation problems mentioned below for going over your available balance it looks fine.
bildschirmfoto 2019-03-04 um 11 13 10
@ManfredKarrer Is this error message correct? I don't think that is my available balance to send.
bildschirmfoto 2019-03-04 um 11 15 15
When I try to send an amount that I shouldn't be able to send I get this wrong mining fee warning popup.

@ManfredKarrer

This comment has been minimized.

Copy link
Member Author

ManfredKarrer commented Mar 4, 2019

@ripcurlx Thanks for the finding. Fixed with latest commit. Could you ACK now?

@ManfredKarrer ManfredKarrer merged commit c91792c into bisq-network:master Mar 4, 2019

@ManfredKarrer ManfredKarrer deleted the ManfredKarrer:allow-spending-unconfirmed-bsq-utxs branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.