feat(autonatv2): Implement autonat v2 #1

umgefahren · 2023-11-07T16:08:40Z

Description

This is a WIP implementation of AutoNATv2.

I would appreciate feedback by @mxinden and @thomaseizinger. You don't have to go into any specifics, I just wanted to check up if the direction is correct. A complete and formal PR will follow once everything is done.

🚧 Client - ConnectionHandler (~80% done)
🚧 Client - Behavior (~10% done)
🌰 Server - ConnectionHandler
🌰 Server - Behavior

I'm sorry it took me so long to give a sign of life. I sunk a lot of hours into doing it with request-response just to discover I need to do it without it. Additionally I couldn't work on it for the last week since I had to study for a midterm.

Notes & open questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

mxinden · 2023-11-07T17:35:31Z

I won't get to a review this week. Sorry.

On the meta level, is there some way this can be a pull request against the libp2p/rust-libp2p repository? Would allow everyone to discover and comment. @thomaseizinger probably has a good idea how to make this a stacked pull request.

thomaseizinger

I've left some high-level comments that will hopefully give you direction! :)

I think overall, I'd like autonat to work as follows:

Triggering autonat can happen for each ExternalAddrCandidate that we receive.
A successful discovery of an external address results in ExternalAddrConfirmed
Autonat servers can be discovered using ConnectionEvent::RemoteProtocolsChange by testing for the availability of /libp2p/autonat/2/dial-request.
We should probably test reachability of an address with at most ~3 autonat servers. Ideally, we just pick 3 random ones that we discovered.

Simplifications that we can make for now:

Test reachability for each candidate individually, no need to send multiple addresses at the same time.
Assume servers will be discovered using RemoteProtocolsChange. Adding servers manually can be done later.

Additionally, before we dive further into the implementation, I'd like to see some tests. Those should give us a direction to code against :)

protocols/autonatv2/src/client/behaviour.rs

protocols/autonatv2/src/client/handler.rs

protocols/autonatv2/src/request_response.rs

thomaseizinger · 2023-11-10T04:00:05Z

On the meta level, is there some way this can be a pull request against the libp2p/rust-libp2p repository? Would allow everyone to discover and comment. @thomaseizinger probably has a good idea how to make this a stacked pull request.

If one of us pushes the other transport-API refactoring as a branch to the main repo, then @umgefahren can open this PR against that branch there on rust-libp2p.

thomaseizinger · 2023-11-10T04:00:47Z

On the meta level, is there some way this can be a pull request against the libp2p/rust-libp2p repository? Would allow everyone to discover and comment. @thomaseizinger probably has a good idea how to make this a stacked pull request.

If one of us pushes the other transport-API refactoring as a branch to the main repo, then @umgefahren can open this PR against that branch there on rust-libp2p.

I am not sure how useful that is though because it means that @umgefahren can't make changes to the base PR as easily. I personally don't mind reviewing PRs in here.

umgefahren · 2023-11-12T10:35:30Z

I would prefer to keep it here. Maybe we can, once we figured out all the mayor bumps like this feedback, post a note somewhere as a RFC on this PR here.

I will rework the changes you proposed, but it will take me until next week. Just one question: Is my optimization I did for the DataResponses premature?

thomaseizinger · 2023-11-12T11:13:06Z

I would prefer to keep it here. Maybe we can, once we figured out all the mayor bumps like this feedback, post a note somewhere as a RFC on this PR here.

I will rework the changes you proposed, but it will take me until next week. Just one question: Is my optimization I did for the DataResponses premature?

Can you point me to the specific code?

thomaseizinger · 2023-11-12T12:24:39Z

I will rework the changes you proposed, but it will take me until next week.

Let's have @mxinden weigh in with his opinion too :)

mxinden · 2023-11-12T14:55:50Z

Triggering autonat can happen for each ExternalAddrCandidate that we receive.

Just need to add some rate limiting. Otherwise a third node can cause a client to continuously ask a server to probe a new address, or even worse, an address of a target.

mxinden · 2023-11-12T14:56:28Z

Assume servers will be discovered using RemoteProtocolsChange. Adding servers manually can be done later.

Sounds great. Zero input needed from the user.

mxinden

Direction looks good to me.

thomaseizinger · 2023-11-12T21:10:28Z

Triggering autonat can happen for each ExternalAddrCandidate that we receive.

Just need to add some rate limiting. Otherwise a third node can cause a client to continuously ask a server to probe a new address, or even worse, an address of a target.

Good point. How about we run it only every X seconds with all candidates reported since the last interval, sorted by their report frequency?

Direction looks good to me.

Do you mean the direction I am suggesting? E.g. two handlers instead of one and no user input to start with?

mxinden · 2023-11-16T15:16:44Z

Direction looks good to me.

Do you mean the direction I am suggesting? E.g. two handlers instead of one and no user input to start with?

Sorry for not being precise. Yes.

mxinden · 2023-11-16T15:17:20Z

Triggering autonat can happen for each ExternalAddrCandidate that we receive.

Just need to add some rate limiting. Otherwise a third node can cause a client to continuously ask a server to probe a new address, or even worse, an address of a target.

Good point. How about we run it only every X seconds with all candidates reported since the last interval, sorted by their report frequency?

That seems reasonable! Haven't given it enough thoughts to be able to suggest alteranatives.

.github/workflows/ci.yml

protocols/autonatv2/src/client/behaviour.rs

protocols/autonatv2/src/client/handler/request.rs

protocols/autonatv2/src/client/handler/dial_back.rs

protocols/autonatv2/src/client/handler/request.rs

protocols/autonatv2/src/client/handler.rs

mxinden · 2023-11-20T11:42:00Z

//CC @sukunrt given that you are the author of the specification and the go-libp2p implementation, you might be interested in the work here.

umgefahren · 2023-11-20T15:18:22Z

I think I have implemented all of your comments. Please let me know if you like the changes I made.

thomaseizinger

Great progress! The client-side looks really good already!

I've left a lot of small comments. The most important ones are around the API between handler and behaviour. I think the Error type should be restructured to make it easier for the behaviour to distinguish between the various error cases.

Feel free to move on to the server before addressing the comments. It would be good to have something working before we make it pretty or optimise it in other ways!

protocols/autonatv2/Cargo.toml

protocols/autonatv2/src/client/behaviour.rs

protocols/autonatv2/src/client/handler/dial_request.rs

protocols/autonatv2/src/client/handler/dial_back.rs

thomaseizinger · 2023-12-04T00:07:15Z

Let me know when you want another review @umgefahren ! :)

umgefahren · 2023-12-06T20:27:04Z

Hi @thomaseizinger,
Sorry that it took me so long, three reasons:

It is again more difficult than expected, now that I started to write tests I realized that.
There was a major mistake in the encoding/decoding section
I had Covid

The actual state is not stable at all and really just a save of a WIP. If you like, you can take a look at it, see if I'm moving into the write direction. However nothing passes on every try and I'm not sure if the DialOpts I have chosen are correct.

A major part is handling the case is dialing back something that doesn't support libp2p, I may have attacked it from the wrong angle, but when dialing 1.1.1.1 it just stalled. And on the other hand dialing back a libp2p node not supporting dial-back.

Have a great day! Maybe we can find time again to talk about these changes.

umgefahren · 2024-03-07T21:26:19Z

I still have a little legal problem. My contract states that I must dual license all my work under MIT and Apache-2.0. Do I need to change the license information on any of the crates I made changes, primarily libp2p-autonat?

thomaseizinger · 2024-03-07T21:32:08Z

I still have a little legal problem. My contract states that I must dual license all my work under MIT and Apache-2.0. Do I need to change the license information on any of the crates I made changes, primarily libp2p-autonat?

All of rust-libp2p is already dual licensed, no? Should be no problem :)

thomaseizinger · 2024-03-07T21:34:13Z

I had to change a Cow to a Vec to make the CI happy about the generated files. That's really a pity though, because I now had to completely scrap all alloc optimisations I made when sending the same piece of data over and over again.

Yeah there are some optimisation we could do if we'd modify the generated files. None of that is benchmarked though and for the moment, it is better to have guarantee that we are using the correct proto files.

It is unfortunate that we can't use your optimisations now.

umgefahren · 2024-03-07T21:34:55Z

But some of the crates have license = "MIT" in the Cargo.toml. Especially the libp2p-autonat crate. Is that "overwritten" by the dual license of the whole project?
But what about the copyright headers in every file? Do I need/should add those and should I make them out to me or the Filecoin Foundation?

umgefahren · 2024-03-07T21:35:37Z

It is unfortunate that we can't use your optimisations now.

It really is, but when someone complains about it I will put it in again.

thomaseizinger · 2024-03-07T21:41:29Z

Update refactor(core, swarm): Transport redesign libp2p/rust-libp2p#4568 with all the changes needed to the transport API

I don't think I made any to libp2p-core just the transport implementations. Should I cherry pick those?

Move this PR to rust-libp2p

I would rather do this. Move this whole PR into that one and merge everything all together.

If it isn't too difficult, I'd prefer it to be two PRs: one for the transport API changes and one for AutoNAT.

It should be pretty easy:

This branch already contains all the changes necessary when compared to master.
Use git subtree split to split all changes of the "autonat" directory into a separate branch.
Delete all of AutoNATv2 from the original branch

Now you should have two branches, one the contains everything but AutoNATv2 and one that only contains AutoNATv2 :)

Have a time-boxed go at it, if it turns out too difficult, we can do it in a single PR.

thomaseizinger · 2024-03-07T21:43:20Z

But some of the crates have license = "MIT" in the Cargo.toml. Especially the libp2p-autonat crate. Is that "overwritten" by the dual license of the whole project? But what about the copyright headers in every file? Do I need/should add those and should I make them out to me or the Filecoin Foundation?

I am not a lawyer but I think MIT is more per permissive than APL2 so we should be fine in relicensing it. Plus, I believe the repo license overrides that.

umgefahren · 2024-03-08T10:36:04Z

But some of the crates have license = "MIT" in the Cargo.toml. Especially the libp2p-autonat crate. Is that "overwritten" by the dual license of the whole project? But what about the copyright headers in every file? Do I need/should add those and should I make them out to me or the Filecoin Foundation?

I am not a lawyer but I think MIT is more per permissive than APL2 so we should be fine in relicensing it. Plus, I believe the repo license overrides that.

If it is overwritten anyway, we'll just keep it as it is.

umgefahren · 2024-03-08T12:55:08Z

I opened a new PR (#3) for just the changes in the transport stuff. Please go ahead and take a quick look at that. Why do I need to split up the autonatv2 changes though? Can't I just make that from this branch?

mxinden · 2024-03-12T09:47:18Z

If no smoke test issues are found I would consider this milestone done. That would require @mxinden's approval

Can you lay out all the next steps here? I am missing an overview.

I still have a little legal problem. My contract states that I must dual license all my work under MIT and Apache-2.0. Do I need to change the license information on any of the crates I made changes, primarily libp2p-autonat?

rust-libp2p is MIT licensed only.

https://github.com/libp2p/rust-libp2p/blob/master/LICENSE

I would assume that the Filecoin foundation is fine with that. Have you asked them?

sukunrt · 2024-03-12T13:53:35Z

@umgefahren
Sorry for the delay here. I'll set up a go-libp2p server this week and share here.

joshuef · 2024-04-09T02:21:01Z

Wee bump here. Would be great to know the status of this, as we'd looove to see it land. (thanks for all the work so far @umgefahren , @mxinden @thomaseizinger @sukunrt ! 🙇 ) (not sure if there's much we could do to be helpful here?)

thomaseizinger · 2024-04-09T04:31:46Z

Wee bump here. Would be great to know the status of this, as we'd looove to see it land. (thanks for all the work so far @umgefahren , @mxinden @thomaseizinger @sukunrt ! 🙇 ) (not sure if there's much we could do to be helpful here?)

I think the next bit is integration testing the two implementations for compatibility to see if the specification has any remaining holes :)

thomaseizinger · 2024-04-09T04:44:00Z

Why do I need to split up the autonatv2 changes though? Can't I just make that from this branch?

@umgefahren Yeah you can probably just merge upstream master into here once the transport changes are merged and the resulting diff should only be AutoNATv2.

sukunrt · 2024-04-09T06:56:56Z

This is blocked on me. I intend to start work again on AutoNAT v2 this week. Ideally I should be able to start testing the implementations this week.

stormshield-ebzh

Thanks for this great rework of autonat.
We started to test and it works fine.
However we found some minor issues
Some issue related to transport are reported in libp2p#4568
and others are in this PR

protocols/autonat/src/v2/client/handler/dial_request.rs

Co-authored-by: stormshield-ebzh <143415961+stormshield-ebzh@users.noreply.github.com>

umgefahren · 2024-05-08T11:25:53Z

Thanks for this great rework of autonat. We started to test and it works fine. However we found some minor issues Some issue related to transport are reported in libp2p#4568 and others are in this PR

Thank you for your review!! I hope I have addressed all of your remarks properly. If there are any remaining issues please let me know.

stormshield-ebzh

Still encountered errors related to stream close()

stormshield-ebzh · 2024-05-14T13:02:43Z

protocols/autonat/src/v2/protocol.rs

+        .send(msg)
+        .await
+        .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+    framed.close().await?;


We observed that the close() may fail if the client has already closed the stream
Suggestion: do not close at all (the stream will be closed when the stream is dropped)

In identify prococol , we have the same behaviour
https://github.com/libp2p/rust-libp2p/blob/ae0a187e4adf8946e9046c001154b364ca637ade/protocols/identify/src/protocol.rs#L147

I have addressed that. Is it working?

Yes ! No more errors related to stream closed too early

protocols/autonat/src/v2/protocol.rs

stormshield-ebzh · 2024-06-04T20:36:30Z

It seems that commit 9d84b7c removed many files from autonat/src/v2

These files are no more in implement-autonat-v2 nor transport-redesign

thomaseizinger reviewed Nov 10, 2023

View reviewed changes

mxinden reviewed Nov 12, 2023

View reviewed changes

thomaseizinger reviewed Nov 18, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

thomaseizinger reviewed Nov 19, 2023

View reviewed changes

protocols/autonatv2/src/client/behaviour.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Nov 19, 2023

View reviewed changes

protocols/autonatv2/src/client/behaviour.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Nov 19, 2023

View reviewed changes

protocols/autonatv2/src/client/behaviour.rs Outdated Show resolved Hide resolved