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

Delay broadcasting of taker fee tx #2488

Merged
merged 34 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@ManfredKarrer
Copy link
Member

ManfredKarrer commented Mar 2, 2019

To avoid the issue with lost taker fees if the take-offer attempt fails,
we delay the publishing of the taker fee tx just before the deposit tx
gets published.
The taker fee tx must not be committed to the wallet to avoid that the
wallet would require a resync in case the tx is not published.
If the tx is not committed the addresses used there are not considered
as used and that would cause issues with the address entry context
assignment for the deposit tx. To avoid those issues we need to force
the creation of new addresses used for the multisig and payout address
entries. To be sure that happens before any of the following tasks make
use of those address entries we do that already in the createTakerFee
task.
For BSQ fee tx it behaves similar but needs separate testing.

ManfredKarrer added some commits Mar 1, 2019

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
Delay broadcasting of taker fee tx
To avoid the issue with lost taker fees if the take-offer attempt fails,
we delay the publishing of the taker fee tx just before the deposit tx
gets published.
The taker fee tx must not be committed to the wallet to avoid that the
wallet would require a resync in case the tx is not published.
If the tx is not committed the addresses used there are not considered
as used and that would cause issues with the address entry context
assignment for the deposit tx. To avoid those issues we need to force
the creation of new addresses used for the multisig and payout address
entries. To be sure that happens before any of the following tasks make
use of those address entries we do that already in the createTakerFee
task.
For BSQ fee tx it behaves similar but needs separate testing.

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

@ManfredKarrer

This comment has been minimized.

Copy link
Member Author

ManfredKarrer commented Mar 2, 2019

@ripcurlx This would require good testing. We can discuss if it should go into the upcoming release. As it would fix an important issue with lsot taker fees I think it would be worth to try to get it in.

ManfredKarrer added some commits Mar 2, 2019

Add altcoin payment method for live trading
- Add LiveAsset account, payment method, AccountPayload
- Extract super classes for normal CryptoCurrenyAccount and payload and
LiveAssetAccount and payload
- Add isAsset method
- Add button for creating a live asset account

As it is a bit tricky to use the AccountForm for both methods and add a
checkbox there so defined which payment method to use I added a button
to add an AccountForm with the LiveAssets passed. This is just
temporary to be able to test a bit more and see if there are any
critical issues. We should unify that form but that might require a bit
of refactoring of the CryptoCurrencyForm.

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 4, 2019

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

Reduce timeout to 90 sec
- We had it initially at 60 sec. and increased it as attempt to fight
those timeout errors, but it did not help. So letting the user wait
longer as needed does not make sense.

ManfredKarrer added some commits Mar 4, 2019

Increase TTL for offer from 7 to 8 min.
We got reports that offers got removed and re-added even if the maker
had good network connections. Seems the network did not propagate the
refresh msg in time or get too crowded. Increasing the TTl should help
to make those cases more rare. To decrease the refresh rate from 5 min
to 4 min. might be more risky as it would create much more traffic.
@ripcurlx
Copy link
Member

ripcurlx left a comment

ACK - Tested it by canceling send publish deposit tx. No fees where lost and if I run the same trade without canceling everything works as expected.

ManfredKarrer added some commits Mar 2, 2019

Delay broadcasting of taker fee tx
To avoid the issue with lost taker fees if the take-offer attempt fails,
we delay the publishing of the taker fee tx just before the deposit tx
gets published.
The taker fee tx must not be committed to the wallet to avoid that the
wallet would require a resync in case the tx is not published.
If the tx is not committed the addresses used there are not considered
as used and that would cause issues with the address entry context
assignment for the deposit tx. To avoid those issues we need to force
the creation of new addresses used for the multisig and payout address
entries. To be sure that happens before any of the following tasks make
use of those address entries we do that already in the createTakerFee
task.
For BSQ fee tx it behaves similar but needs separate testing.
Reduce timeout to 90 sec
- We had it initially at 60 sec. and increased it as attempt to fight
those timeout errors, but it did not help. So letting the user wait
longer as needed does not make sense.

@ripcurlx ripcurlx force-pushed the ManfredKarrer:avoid-taker-fee-publishing-for-failed-trades branch from ed6fa7b to db4c6f5 Mar 4, 2019

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Mar 4, 2019

@ManfredKarrer I rebased your PR and solved the conflict. Besides the two missing merge commits to master, also the OfferPayloud TTL change commit is gone as it was changed by you to 9 minutes already in another PR.

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Mar 4, 2019

I leave it to you to merge, if you want to have another look at my reworked commit history.

ArqTras and others added some commits Mar 4, 2019

Merge branch 'master' into avoid-taker-fee-publishing-for-failed-trades
# Conflicts:
#	core/src/main/java/bisq/core/offer/OfferPayload.java
Merge branch 'allow-spending-unconfirmed-bsq-utxs'
# Conflicts:
#	core/src/main/java/bisq/core/trade/protocol/tasks/taker/CreateTakerFeeTx.java
Merge remote-tracking branch 'origin/avoid-taker-fee-publishing-for-f…
…ailed-trades' into avoid-taker-fee-publishing-for-failed-trades

# Conflicts:
#	core/src/main/java/bisq/core/trade/protocol/tasks/taker/TakerPublishFeeTx.java

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

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@ManfredKarrer ManfredKarrer deleted the ManfredKarrer:avoid-taker-fee-publishing-for-failed-trades 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.