-
Notifications
You must be signed in to change notification settings - Fork 32
Add CI simulation test for genesis generation to finalization #194
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: main
Are you sure you want to change the base?
Conversation
|
add CI task for the same |
This will be on0hold till Kai Pr 198 gets in, so I ca properly refactor |
|
Hello @g11tech @gballet @GrapeBaBa both nodes are using networkId = 0, which causes them to access the same global SWARM_STATE in the Rust libp2p bridge that sort of creates a race condition and memory corruption and .networkId = 0 is (hardcoded) |
@g11tech it seems a hack only used for testing such as |
Am getting this segfaults error, should I spawn external process for this so to avoid the infinite loop in node.run() or am I getting something wrong here @g11tech @gballet @GrapeBaBa
|
|
This is what I get when running it once. I don't know if the segmentation fault is responsible for the timeout at this point. In another run, I was able to see that this has to do with an invalid ListArray / some issue in SSZ. Make sure your PR is rebased on top of masters, some bugs have been fixed in this general area recently. |
I rebased but am not sure why am getting this @gballet |
|
seems like this is segfauling in create and run network, for which another thread is spun so I am not sure how tokio/rust runtimes exactly behaves so ether first create the two networks how we do in in beam sim and the pass them to the two nodes, or spin the two nodes as different processes |
e27d60f to
ad05b56
Compare
pkgs/cli/src/main.zig
Outdated
| } | ||
|
|
||
| test { | ||
| _ = @import("test/genesis_to_finalization_test.zig"); |
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.
this is an integraton test so it needs to be bundles in simtest (create a common file to import these tests in the test/integration.zig (rename the other integration test file and import it as well)
pkgs/cli/src/main.zig
Outdated
| .genesis_spec = undefined, | ||
| .validator_indices = undefined, | ||
| .local_priv_key = undefined, | ||
| .bootnodes = &[_][]const u8{}, |
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.
what does this fix?
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 a segfault caused by the defer-with-undefined antipattern, the fields were initialized as undefined, but defer deinit() always runs - even if initialization fails. When deinit() tried to free undefined pointers (0xaaaaaaaaaaaaaaaa), it crashed.
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.
each fields should be assigned in the next function, I don't think it is the cause, may at least not all fields need this empty value. can you point specific field, and why it is not assigned in the next function.
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.
You're right that only 3 fields technically need safe initialization, the deeper issue is the architectural pattern, setting up cleanup (defer) before initialization is complete. The proper fix would be refactoring buildStartOptions() to return the struct instead of mutating it, so defer is only set after full initialization. For now, I'll keep the current defensive fix since it's safe and works, but I'll note this for future refactoring.
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 you point the specific line of the problem code and how to reproduce this issue, current the change, some default values such as "" seems also not allocated in heap and can't be free
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.
let me check this code, how can I reproduce the segfault?
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.
didn't see the change we were talking in the chat?
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 did reply you on that then you pointed out a different pattern am yet to do that just merged latest changes from main, I will push on it shortly
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.
ok, i just see a github notification. Is it not ready to review again right now?
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.
Yes, but I will push shortly
87bda95 to
3c08acf
Compare
8835d4c to
d4ff9b1
Compare
…aps/zeam into add-test-genesis-two-nodes


closes #164