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

Fix initial data request handling #6947

Conversation

djing-chan
Copy link
Contributor

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

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
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
Only start requesting blocks after wallet is synced.
Update connection state when doing repeated block requests.
Improve logs.
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.

Copy link
Collaborator

@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

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.

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
Copy link

boring-cyborg bot commented Nov 24, 2023

Awesome work, congrats on your first merged pull request!

@djing-chan djing-chan deleted the fix-initial-data-request-handling branch December 10, 2023 04:14
@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
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.
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

Successfully merging this pull request may close these issues.

3 participants