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.

Sign up for GitHub

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

Jump to bottom

Bugfix: race condition of two threads using same RandomX VM #1021

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Bugfix: race condition of two threads using same RandomX VM #1021

merged 1 commit into from
Feb 2, 2023

Conversation

us77ipis
Copy link
Contributor

@us77ipis us77ipis commented Nov 4, 2022

Race condition between

veil/src/pow.cpp

Line 462 in 4107c99

randomx_calculate_hash(GetMyMachineValidating(), &hash_blob, sizeof uint256(), hash);
and

veil/src/chain.cpp

Line 304 in 4107c99

randomx_calculate_hash(GetMyMachineValidating(), &hash_blob, sizeof uint256(), hash);

which use the same RandomX VM but the first one correctly acquires the lock cs_randomx_validator before, while the second doesn't acquire the lock.

This bug caused the daemon / wallet to randomly crash.

Zannick
Zannick approved these changes Nov 11, 2022
View reviewed changes
Copy link
Collaborator

@Zannick Zannick left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK 140b2fe

@seanPhill seanPhill added the QA: Pending QA is waiting a response/confirmation from developers label Dec 21, 2022
@seanPhill seanPhill added QA: In Progress This is being tested by the QA team. KEEP CALM and RELAX. and removed QA: Pending QA is waiting a response/confirmation from developers labels Jan 31, 2023
@seanPhill
Copy link
Collaborator

I have been running this for some weeks, but haven't done a lot of RandomX mining.
I am stepping up this mining on testnet.

@seanPhill seanPhill added QA: Passed This has passed QA testing and can be merged to master and removed QA: In Progress This is being tested by the QA team. KEEP CALM and RELAX. labels Feb 2, 2023
@seanPhill
Copy link
Collaborator

I ran this on testnet and mined a lot of RandomX without any problems. Debug log entries look fine.

@seanPhill seanPhill merged commit df7b820 into Veil-Project:master Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zannick Zannick Zannick approved these changes

Assignees
No one assigned
Labels
QA: Passed This has passed QA testing and can be merged to master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@us77ipis @seanPhill @Zannick