fix(network): Ignore out of order Address Book changes, unless they are concurrent

[teor2345]

Motivation

We want to ignore earlier address book changes which are delayed, so we don't accidentally reset the peer state to an earlier value. But some changes can be concurrent, so the change application order doesn't always match the network order.

Close #6672

Specifications

There is no formal specification for peer network behaviour, but in general, the peer's internal address book state should reflect the actual behaviour of its peers.

Complex Code or Requirements

This code runs concurrently, it is accessed from both the address book updater task and the candidate set peer dialler task.

Changes can arrive earlier or later than their network order because of:

• waiting on the address book mutex
• async runtime or OS thread scheduling
• monotonic clock resolution, or OS or hardware time bugs
• wall clock time updates

The application can also independently initiate a change to the connection state, which can be simultaneous (or before) a change from the peer. In this case, the application-provided change needs to take priority. (Because it is typically failing the connection.)

Solution

Fix the code that applies changes to the address book, so that:

• Concurrent changes are applied if they progress the connection state, but ignored if they revert it
• Much earlier changes are ignored, even if they progress the connection state

Refactor the code that applies changes so it is easier to read.

Add TODOs for further changes that are not required to fix this security issue.

Testing

• Add tests for the failure and success cases for the new checks, the extra success cases are:
  • Much later changes are applied, even if they revert the connection state
• Allow tests to provide synthetic times for address book updates
  • This resolves some technical debt caused by a partial refactor
  • Calling time APIs less will also improve performance on some systems

There are existing tests for the other kinds of changes.

Review

This is one of the more complex audit changes that we have left to do. I'm happy to do a video review if that helps!

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

Follow Up Work

Make it much harder for remote peers to delay changes by flooding the change channel:

• fix(net): Rate-limit MetaAddrChange::Responded from peers #6738

[codecov]

Codecov Report

Merging #6717 (826395e) into main (fadddae) will decrease coverage by 0.08%.
The diff coverage is 89.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6717      +/-   ##
==========================================
- Coverage   78.14%   78.07%   -0.08%     
==========================================
  Files         310      308       -2     
  Lines       40874    40958      +84     
==========================================
+ Hits        31942    31978      +36     
- Misses       8932     8980      +48     

[arya2]

This is looking great, thank you for fixing this.

I like the workaround for the imprecise timestamps (though I'd like to review it more thoroughly).

[arya2]

Thank you!

[teor2345]

@oxarbitrage I'm going to go ahead and merge this PR now to avoid merge conflicts.

Feel free to review it or skip the review, whatever you work out with Pili.