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

Start trading window timer after deposit transaction confirmation #1170

Closed
cbeams opened this issue Jan 9, 2018 · 9 comments
Closed

Start trading window timer after deposit transaction confirmation #1170

cbeams opened this issue Jan 9, 2018 · 9 comments
Assignees
Milestone

Comments

@cbeams
Copy link
Member

cbeams commented Jan 9, 2018

Currently, Bisq's trading window timer starts when an offer is taken. Usually this works out fine, but during blockchain congestion, Bisq's maker, taker and deposit transactions can take many hours or even days to confirm, causing the trading window to be exceeded and often resulting in drawn out arbitration cases and canceled trades.

A simple solution is to start the trading window timer only after the deposit transaction has confirmed. Indeed, this should have been the implementation all along—it just hadn't become important until recently.

See description in #1171 for details.

@cbeams cbeams added this to the v0.6.3 milestone Jan 9, 2018
@cbeams
Copy link
Member Author

cbeams commented Jan 9, 2018

@ManfredKarrer, I've assigned you to this issue and slated it for v0.6.3, as I believe we agreed that this change should get into the next release.

@ManfredKarrer
Copy link
Member

Seems to be a bit more tricky as I thought.
https://groups.google.com/forum/#!forum/bitcoinj
Also will require a bit of testing as the trade period is checked and used in various places.

I prefer to not include that in the 0.6.3 release.
Added my current WIP code here: https://github.com/bisq-network/exchange/tree/TradePeriodStartsAfterConfirmation

@cbeams
Copy link
Member Author

cbeams commented Jan 11, 2018

I assume you meant this post on the bitcoinj forum: https://groups.google.com/forum/#!topic/bitcoinj/05TJbxAwKXs

I've removed this from v0.6.3 and will add to v0.6.4 to make sure this important issue stays on our radar.

@cbeams cbeams modified the milestones: v0.6.3, v0.6.4 Jan 11, 2018
@cbeams cbeams modified the milestones: v0.6.4, v0.6.5 Jan 24, 2018
@cbeams
Copy link
Member Author

cbeams commented Jan 24, 2018

@ManfredKarrer, I've removed this from v0.6.4 (as it's already shipped), and tentatively added it to v0.6.5. Do you think you can get to this? It is quite an important measure for avoiding future messes when blockchain congestion rears its head again.

@ManfredKarrer
Copy link
Member

@cbeams Will try.... Though wanted to focus back on DAO stuff.

@ManfredKarrer
Copy link
Member

Required a fix in BitcoinJ to get the block time (bisq-network/bitcoinj#11).

Behavior is following:

  • If deposit tx is unconfirmed the trade duration is set to max trade duration of payment method
  • If deposit tx is confirmed we use block time where tx got included. That time can differ +/- 2 hours.
    To handle that possible time offsets we use following rules:
    If block date is in future (Date in Bitcoin blocks can be off by max. 2 hours) we use our current date.
    If block date is before our trade date we use our trade date.

@ManfredKarrer
Copy link
Member

Implemented via PR #1287

@cbeams
Copy link
Member Author

cbeams commented Jan 31, 2018

Implemented via PR #1287

@ManfredKarrer, it's better to do this sort of thing on the PR side. Include the text Fixes #1287 in the description of the PR and the referenced issue will be closed automatically if and when the PR is merged.

@cbeams
Copy link
Member Author

cbeams commented Jan 31, 2018

Closing as complete now that #1287 has been merged.

@cbeams cbeams closed this as completed Jan 31, 2018
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

No branches or pull requests

2 participants