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

Embed Robustness into mixnet.py #61

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

youngjoon-lee
Copy link
Contributor

@youngjoon-lee youngjoon-lee commented Jan 31, 2024

We have discussed lots of time about the robustness layer, regarding its role and its name: #57 (comment). Now, I think it is time for us to make a conclusion about this.

Through many discussions, I end up with thinking that we don't need to define the Robustness class in this executable spec explicitly.

As I understand, the robustness layer has been suggested to clearly separate config reconstruction logic from the core mixnet logic. I totally agree with it. However, having an explicit Robustness component has made confusions. Some wondered whether it performs or will perform a role for components other than mixnet, and if so, what that role is. Also, if it's only for mixnet (at least in v1), others wondered why we shouldn't define the config reconstruction logic within the area of mixnet.

I also agree with these confusions. This topic might be an implementation detail that doesn't need to be discussed in the spec level.
So, in this PR, I'm suggesting a simpler structure. Config reconstruction is done in mixnet.py that is the top-most class of mixnet and has APIs for interactions with upper components. The actual mix/networking logic is defined in mixclient.py and mixnode.py.
I believe this structure is quite clear and will not cause further confusions.

Please provide your opinion here.
I'd recommend you to focus on the structure for now. There are some code that needs to be refactored, but I tried to minimize diffs to show the structure clearly. I'm gonna refine code after we made a conclusion.

@youngjoon-lee youngjoon-lee self-assigned this Jan 31, 2024
@youngjoon-lee youngjoon-lee changed the base branch from master to mixnet-integrate January 31, 2024 10:45
Copy link
Collaborator

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is simpler and better.

Base automatically changed from mixnet-integrate to master February 5, 2024 08:04
Copy link
Contributor

@zeegomo zeegomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked in detail at the code but it looks ok at first sight and I 100% agree with the premises

@alvatar
Copy link

alvatar commented Feb 5, 2024

I see that the code uses the naming "config", while we are discussing the concept of robustness. As mentioned in Notion, I think robustness is not a very accurate naming for what this does, so I think the code is in better place now.

Copy link

@madxor madxor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Regarding the robustness layer, the initial idea was to extract configuration and management of the mixnet out of the mixnet layer. So maybe the "mixnet management layer/module/subsystem" name would suit it better or at least be less confusing. I was envisioning that in the farther perspective the "mixnet management layer/module/subsystem" would be responsible for learning how to set the mixnet based on the data coming from the consensus/network/coordination layers. Hence, having a separate layer in the future might be more beneficial but I see that it's not necessary at the moment.

@youngjoon-lee youngjoon-lee merged commit 5dd7b27 into master Feb 8, 2024
1 check passed
@youngjoon-lee youngjoon-lee deleted the mixnet-no-robustness branch February 8, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants