Skip to content
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

Validator DHT #494

Merged
merged 21 commits into from
Dec 23, 2023
Merged

Validator DHT #494

merged 21 commits into from
Dec 23, 2023

Conversation

kayabaNerve
Copy link
Member

Uses sc_authority_discovery and a new RPC route to let the coordinator find fellow validators.

Additionally adds an RPC route to retrieve their P2P addresses.
Prevents the coordinator from believing it has 3 peers when it has one.
@kayabaNerve kayabaNerve added feature New feature or request coordinator labels Dec 19, 2023
@kayabaNerve
Copy link
Member Author

kayabaNerve commented Dec 21, 2023

This 'works'.

The issue is it expects the IPs a node to be reachable over to be equal to the IPs a node's coordinator is reachable over.

This isn't held by our Docker setup.

@kayabaNerve
Copy link
Member Author

I think this does require removing dockertest for our own library :/

@kayabaNerve
Copy link
Member Author

Using recursive DockerTests, it's possible to achieve the multi-network effect desired. Unfortunately, recursive DockerTests is only feasible with orcalabs/dockertest-rs#10, hence the patch added (and failing deny CI).

Assuming the above CIs pass in an hour, we can add a deny allowance or wait for the PR to be merged.

@kayabaNerve
Copy link
Member Author

coordinator key_gen passed, showing this can work. Unfortunately, any successive tests within a binary fail. It can't find serai-node-(5 .. = 8).

full stack tests are deadlocking spawning everything. coordinator shows it's theoretically feasible. Still looking to what's up...

@kayabaNerve
Copy link
Member Author

full-stack dropped the shadowed outer_ops, not the lock outer_ops.

@kayabaNerve
Copy link
Member Author

outer_ops isn't supposed to be cleared within the test boundary yet also wasn't across the test boundary. That's the bug in coordinator.

@kayabaNerve
Copy link
Member Author

full-stack locally pass.

@kayabaNerve
Copy link
Member Author

batch and sign passed.

key_gen, the least complex, failed with

2023-12-22T12:10:38.8785413Z thread 'tests::key_gen::key_gen_test' panicked at /home/runner/.cargo/git/checkouts/dockertest-rs-f269f928e616902b/c0ea779/src/dockertest.rs:191:13:
2023-12-22T12:10:38.8787769Z docker daemon interaction error `failed to start container: driver failed programming external connectivity on endpoint dockertest-rs-coordinator-serai_node-8-mlbizwktdtnmnllxegoq (36b3f1f8e16ad154bddb03d31a7efc93120ad0220a5d946564feaf9ee73412cb): Error starting userland proxy: listen tcp4 0.0.0.0:32822: bind: address already in use`

I'm unsure if these tests are significantly more flakey or if this truly is a random fluke. We didn't actually use new networks. The coordinators attach to specific nodes' networks, so we aren't assigning two coordinators to the same node by coincidence (with an ID unique across tests). The nodes should've had their networking untouched and it sounds like the node is the one with the networking error.

Coordinator and node are on different ports...

Rerunning for further context.

@kayabaNerve
Copy link
Member Author

Will push a deny allowance. If CI passes without intervention, will merge.

@kayabaNerve
Copy link
Member Author

Passed without intervention. Assuming truly random fluke in CI timing which we can further debug later if necessary (it is a setup failure, not a test failure). Merging.

@kayabaNerve kayabaNerve merged commit b493e3e into develop Dec 23, 2023
13 checks passed
@kayabaNerve kayabaNerve deleted the validator-dht branch December 23, 2023 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coordinator feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant