Page MenuHomePhabricator

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

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

Details

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

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

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Jul 8 2021, 11:33
deadalnix added inline comments.
src/avalanche/peermanager.cpp
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.Jul 8 2021, 12:45

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

src/avalanche/test/peermanager_tests.cpp
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.Jul 8 2021, 13:04
This revision was automatically updated to reflect the committed changes.