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

Fix initial data request handling #6947

Merged
merged 8 commits into from Nov 24, 2023
Merged

Fix initial data request handling #6947

merged 8 commits into from Nov 24, 2023

Conversation

djing-chan
Copy link
Contributor

@djing-chan djing-chan commented Nov 20, 2023 •
edited

This should fix the issues with dao hash conflicts when starting a node which was missing many blocks.

Now we wait until the updated data requests do not report that data is still missing (truncated flag). Only after that (when at least one seed responded with a non-truncated response) we consider the network initialized and trigger client code.

In the dao block requests there are some fixes as well which should make the block deliver more stable.

This work is based on @jmacxx draft PR #6946.

jmacxx and others added 8 commits November 13, 2023 19:03
Download all P2P data before announcing P2P ready.
b8d1c92
@djing-chan
Change log level to debug for non-essential logs
8ee67e5 
Set loge level for org.berndpruenster.netlayer.tor.Tor to WARN (we get repeated logs about HS announced to network from netlayer - would be better to change in netlayer).
Make data request/response logs more visible with line breaks
@djing-chan
Increase num repeated request limit from 10 to 20.
8aa9d7c 
Make logmore visible.
@djing-chan
Use onDataReceived instead of onUpdatedDataReceived for considering p…
08118e3 
…2p network state as initialized
@djing-chan
Merge branch 'improve-logs' into fix-initial-data-request-handling
c8fd897
@djing-chan
Improve logs for block requests
536a29f
@djing-chan
Add flag to avoid multiple calls of onDataReceived
34857c9 
Only start requesting blocks after wallet is synced.
Update connection state when doing repeated block requests.
Improve logs.
@djing-chan
Increase package name length before it gets shortened
688f4f7
@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Nov 20, 2023

Thanks for opening this pull request!

Please check out our contributor checklist and check if Travis or Codacy found any issues with your PR. Also make sure your commits are signed, and that you applied Bisq's code style and formatting.

A maintainer will add an is:priority label to your PR if it is up for compensation. Please see our Bisq Q1 2020 Update post for more details.

HenrikJannsen
HenrikJannsen approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@HenrikJannsen HenrikJannsen left a comment

Choose a reason for hiding this comment

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

ACK

ghost
ghost approved these changes Nov 20, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

ACK
Tested for 1.5 days (mainnet) :

  • new data directory, lite DAO node
  • new data directory, full DAO node
  • existing data directory
  • checked the case where new blocks are mined during initial blocks download
  • verify self calculated DAO hashes after sync
  • seed node
  • seed node, new data directory.

There is one issue where the trade statistics do not appear updated in the GUI until after restart -> that is an existing issue not something introduced by this PR change. I mention it here because it is likely to be noticed by others when testing this.

@djing-chan
Copy link
Contributor Author

@jmacxx Great thanks for the extensive testing!!!

@ghost
Copy link

ghost commented Nov 23, 2023

NB: I think this PR needs to be thoroughly soak-tested for several weeks before considering safe to merge. I've started that soak testing and encourage others to do so as well.

alejandrogarcia83
alejandrogarcia83 approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@alejandrogarcia83 alejandrogarcia83 left a comment

Choose a reason for hiding this comment

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

utACK

@alejandrogarcia83 alejandrogarcia83 merged commit 95e7493 into bisq-network:master Nov 24, 2023
3 checks passed
@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Nov 24, 2023

Awesome work, congrats on your first merged pull request!

This was referenced Nov 24, 2023
Open
Closed
@djing-chan djing-chan deleted the fix-initial-data-request-handling branch December 10, 2023 04:14
@djing-chan djing-chan mentioned this pull request Dec 24, 2023
Closed
@alejandrogarcia83 alejandrogarcia83 added this to the v1.9.15 milestone Dec 31, 2023
alvasw added a commit to alvasw/bisq that referenced this pull request Jan 6, 2024
@alvasw
Create GetBlocksRequestHandler's reply list once
1e78902 
Previously, the onGetBlocksRequest method created an ArrayList and two
LinkedList before creating the GetBlocksResponse. The first two lists
were never used. PR bisq-network#6947 introduced the second LinkedList creation.
With this change, the GetBlocksRequestHandler only creates a single
LinkedList.
@alvasw alvasw mentioned this pull request Jan 6, 2024
Merged
@alvasw alvasw mentioned this pull request Jan 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HenrikJannsen HenrikJannsen HenrikJannsen approved these changes

@alejandrogarcia83 alejandrogarcia83 alejandrogarcia83 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.9.15
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@djing-chan @HenrikJannsen @alejandrogarcia83