Skip to content

Conversation

@ArneBab
Copy link
Contributor

@ArneBab ArneBab commented Jun 16, 2022

Changes that look like they should be included even if we do not merge PR #746 , but they need more review.

Eleriseth and others added 6 commits June 14, 2022 12:17
BEWARE. TOTALLY UNTESTED CODE PATH ENABLED.
This code never worked correctly since introduction in commit
f32e365 (svn r16190).
… bug fix, not the cleanup ]

and we got off with that only because shouldAcceptAnnounce was broken too.
…ounce was completely broken", not including the stuff about double announcement (which isn't relevant).
…es. Don't assert. Not the same as Eleriseth's patch, but thanks Eleriseth for pointing out the issue.
Copy link
Contributor

@vwoodzell vwoodzell left a comment

Choose a reason for hiding this comment

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

The new logic seems to be correct. Nice to enable a feature that's never worked. :) I can't comment on the effect that the feature might have.

@ArneBab
Copy link
Contributor Author

ArneBab commented Oct 2, 2022

Thank you for checking!

We might have to roll this out on one or two seednodes first to check whether it throttles announcements too much. I’ve seen a seednode have all peers in backoff, so this could be important to have (and an overall improvement).

@ArneBab ArneBab merged commit 194a410 into next Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants