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 flaky p2p-fullblocktest #2605

Merged
merged 2 commits into from
Jan 3, 2019

Conversation

codablock
Copy link

See individual commits.

Announcing as INV results in one GETHEADERS message per INV item with
every GETHEADERS containing the same block locator info. This in turn
results in many (1088 in p2p-fullblocktests) replies with each having i+1
headers. As every message is sent in one go, the message processing queue
gets overloaded, leading to GETDATA timeouts for blocks and thus failing
the tests.

Announcing the blocks directly with HEADERS avoids all this and results in
one GETDATA per announced header.

This will later conflict with backported improvements from Bitcoin. When we
get to this point, we can simply ignore changes on Dash's side and take
the improvements from Bitcoin (which also switch to header announcement,
but after some refactorings)
@codablock codablock added this to the 14.0 milestone Jan 3, 2019
Large reorgs as in the p2p-fullblocktest can cause very long long delays
for the ping that we're waiting for. This was not a problem before dashpay#2593,
as before this wait_until was waiting forever.
@codablock codablock force-pushed the pr_flaky_p2p_fullblocks_tests branch from 20d4caf to 44c4cf8 Compare January 3, 2019 06:33
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned with future merge conflicts you mentioned but I guess it makes sense to fix it and not wait for backports (or backport in no order and maybe cause more potential conflicts).

utACK

@UdjinM6 UdjinM6 merged commit 0648496 into dashpay:develop Jan 3, 2019
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.

None yet

2 participants