-
Notifications
You must be signed in to change notification settings - Fork 72
Impl DHT bootnodes support for all nodes #1344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WASM runtime size check:Compared to target branchdancebox runtime: 1896 KB (no changes) ✅ flashbox runtime: 1128 KB (no changes) ✅ dancelight runtime: 2656 KB (-16 KB) 🚨 starlight runtime: 2568 KB (+4 KB) 🚨 container chain template simple runtime: 1472 KB (no changes) ✅ container chain template frontier runtime: 1832 KB (no changes) ✅ |
|
|
||
| #[allow(deprecated)] | ||
| use sc_executor::NativeElseWasmExecutor; | ||
| use sc_network::request_responses::IncomingRequest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of places with 2 different use styles - combined and separate lines.
Should we follow one of them? Or should we use different ones depending on specific conditions? Like here - you have aggregated use and you add separated lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly rustfmt config for this is nightly only and has issues.
I'm in favor of combined style unless it has different attribute (#[allow(deprecated)] here only applies to the NativeElseWasmExecutor import. grouping also allows not repeating the attribute for multiple uses)
| } = start_bootnode_params; | ||
|
|
||
| // Advertise parachain bootnode address in relay chain DHT | ||
| start_bootnode_tasks(StartBootnodeTasksParams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for this?
Maybe pick something from here if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it as a new zombienet suite. We don't have any rust tests using zombienet_sdk and I don't want to be the first to add one.
The test will simply spawn tanssi-relay but with --no-mdns flag to disable peer discovery, and no bootnodes in chain spec or in tanssi pallet data preservers. Then we can simply check that all collators find a bootnode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests but they don't work because collators don't have the bootnode discovery task. I will try to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, but now I added data preservers as well so I need to test that 🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I did the data preservers test manually, because to register a data preserver you need to put your bootnode address on chain, and it is hard to enforce that other nodes don't look at the on chain address. I verified that the address is advertised correctly, and other nodes discover that address (verified by looking at trace logs), but I didn't verify that other nodes can connect to the data preserver using that address only.
But not for collators nor data preservers
f6e252a to
e8f397f
Compare
| .database_params | ||
| .database = Some(Database::ParityDb); | ||
|
|
||
| // TODO: need to modify anything from here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this TODO still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed yes, will remove the TODO
| ] | ||
| }, | ||
| { | ||
| "name": "zombie_tanssi_relay_dht_bootnodes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider this feature as important to run on merge once ready @tmpolaczyk ? I think we should, but I trust you on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. Because this is an "optional" feature, it's an additional way for nodes to discover peers. But if we don't test it explicitly, it is possible that it breaks and we never notice, precisely because its optional. Maybe adding this to the weekly run is enough, if I haven't done that already.
paritytech/polkadot-sdk#8072
Latest polkadot-sdk release includes a new feature that allows parachain nodes to advertise their IP address in the relay chain DHT (kademlia). This allows other nodes to discover peers without a centralized peer list.
In this PR we implement it for all nodes, including collators and data preservers. Container chain collators do not advertise their IP address, but they do discover bootnodes for the chain they are assigned to. The reason for not advertising is that collators may rotate frequently, and also that they don't have the full block history. So collators can only be valid targets for warp sync, not for full sync.