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.

Sign up for GitHub

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

Jump to bottom

Don't allow trade start if BitcoinJ is not fully synced #4764

Merged
merged 1 commit into from Nov 11, 2020
Merged

Don't allow trade start if BitcoinJ is not fully synced #4764

merged 1 commit into from Nov 11, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2020 •
edited by ghost
Loading

Adds a check that if chain height is more than 3 blocks of the reported height from bitcoin peers -> it is considered a serious error and the user is prevented from taking or creating an offer, also any open orders are prevented from being taken as it would cause a protocol error.

It displays a popup message informing the user that Bisq is not currently synced, advising them to wait for one Bitcoin block and if necessary to do an SPV resync.

Additionally under Settings/Network, a field has been added to show the chain height of Bisq vs the Peer group.

Fixes: #4727
Fixes: #4707

Screenshots

image

Network info showing chain height:
image

Logged upon attempting to take or create offer when not synced:
image

@ghost

This comment has been minimized.

Sign in to view
@chimp1984
Copy link
Contributor

@jmacxx Thanks for picking that up!

Have you seen the recent changes I did for the footer to show the latest BTC block? Not sure if that can help for the popup msg... I think its merged in master...

The Network info label for chain height might be too technical (Peer group).

I think it should be enough to apply it only to the trade use case. Until now we never had it and only with the introduction of the locktime we got an issue with not being on the latest block.

@ghost
Copy link
Author

ghost commented Nov 8, 2020

Ahh, I had not seen your PR for putting chainHeight in the footer, it is not yet merged. What I've noticed is that chainHeight property is 0 until the first new block comes in after startup which can be a 10, 20, or 30 minutes wait. Perhaps it should be initialized; getChain().getBestChainHeight() returns the current block number.

I'll remove the label in network settings and rework this PR as you suggested, thanks.

@chimp1984
Copy link
Contributor

chimp1984 commented Nov 8, 2020 •
edited
Loading

What I've noticed is that chainHeight property is 0 until the first new block comes in after startup which can be a 10, 20, or 30 minutes wait. Perhaps it should be initialized; getChain().getBestChainHeight() returns the current block number.

Oh, yes that should be fixed. Feel free to add a fix for that.

@chimp1984
Copy link
Contributor

I'll remove the label in network settings

Maybe its good to have it there, just a bit less technical label... something like "Latest BTC block height"

@chimp1984
Copy link
Contributor

I think its merged in master...

No, it is part of #4749
9bfdea1

@ghost
Copy link
Author

ghost commented Nov 9, 2020

I've made the changes above - in addition any open orders will also be prevented from being taken in the error case where chain is out of sync.

@ghost ghost changed the title [WIP] Don't allow trade start if BitcoinJ is not fully synced Don't allow trade start if BitcoinJ is not fully synced Nov 9, 2020
@ghost ghost marked this pull request as ready for review November 9, 2020 21:14
@ripcurlx ripcurlx added this to the v1.5.1 milestone Nov 10, 2020
chimp1984
chimp1984 previously approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

core/src/main/java/bisq/core/btc/setup/WalletsSetup.java Outdated Show resolved Hide resolved
Don't allow trade start if BitcoinJ is not fully synced
0ddd333 
Adds a check that chain height is within 3 blocks of the reported
height from bitcoin peers -> if not the user cannot take an offer
or have an existing offer taken.  It shows a message informing the
user that Bisq is not currently synced, advising them to do an
SPV resync if necessary.

Additionally under Settings/Network a field has been added
to show the chain height of Bisq vs the Peer group.

Added after discussion with chimp1984:
- Correct initialization of chainHeight property
- Rename "Latest BTC block height" display field for clarity
- Enforce chain sync rule for Take Offer scenario
- Enforce chain synch rule for Check offer availability scenario
- change method name to be clearer
chimp1984
chimp1984 approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

ripcurlx
ripcurlx approved these changes Nov 11, 2020
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@ripcurlx ripcurlx merged commit 0c47cf0 into bisq-network:master Nov 11, 2020
This was referenced Nov 26, 2020
Closed
Closed
Closed
@ghost ghost mentioned this pull request Mar 17, 2021
Merged
@ghost ghost deleted the fix_issue_4727 branch May 29, 2022 22:45
@ghost ghost mentioned this pull request Sep 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.2
2 participants
@chimp1984 @ripcurlx