Page MenuHomePhabricator

[avalanche] Fix an edge case that can assert when updating a node

Authored by Fabien on Thu, Jul 8, 11:33.


Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
rABC7bcc423b2c59: [avalanche] Fix an edge case that can assert when updating a node

There is currently no real-world way to trigger this issue, but this will become possible once the node/peer binding is completed.

When a peer is removed, the nodes are removed as well unless they have an inflight query. This is done so if the node is bound to another peer at a later time is is not overqueried. But the the code updating the node will only work when the node moves from an existing peer to another existing one, and assert if the previous peer no longer exists.
This diff prevents this assertion and add a test that would raise the assertion before this patch.

Ref T1634.

Test Plan
ninja check-avalanche

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Thu, Jul 8, 11:33
deadalnix added inline comments.
86 ↗(On Diff #29106)

Shouldn't this be patched too? If both needs to be patched, shouldn't removeNodeFromPeer be patched instead?

PiRK added a subscriber: PiRK.

Reviewed and tested, with and without the patch.

This revision is now accepted and ready to land.Thu, Jul 8, 12:45

Make removeNodeFromPeer valid when the peer no longer exist.
Add a test case for removeNode as well.

575 ↗(On Diff #29109)

You should check that the node is still present here.

New test produces error as expected, patch fixes it as expected.

Check the nodes are not removed with the peer in the test

This revision was landed with ongoing or failed builds.Thu, Jul 8, 13:04
This revision was automatically updated to reflect the committed changes.