Skip to content

Use separate mutex for locked outputs #245

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

Merged
merged 5 commits into from
Jun 5, 2025
Merged

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented May 28, 2025

This PR reverts a change we made that introduced a deadlock in the locking behaviour. Instead of keeping the tip in memory, we return it from the store alongside the unspent elements. It also improves the overall locking by covering the fetching of unspent element under the lock as well.

@peterjan peterjan self-assigned this May 28, 2025
@github-project-automation github-project-automation bot moved this to In Progress in Sia May 28, 2025
@peterjan peterjan marked this pull request as ready for review May 28, 2025 12:32
Base automatically changed from pj/fix-race to master May 28, 2025 15:39
@n8maninger
Copy link
Member

n8maninger commented May 28, 2025

This feels like somewhere is calling a wallet method that locks the mutex within the open db transaction rather than an issue with the internal locking. The locking is fairly intentional to prevent invalid proofs. I'd rather not change it unless we're confident it's not breaking something else.

@ChrisSchinnerl
Copy link
Member

This feels like somewhere is calling a wallet method that locks the mutex within the open db transaction rather than an issue with the internal locking. The locking is fairly intentional to prevent invalid proofs. I'd rather not change it unless we're confident it's not breaking something else.

The obvious candidate is UpdateChainState. Because usually the SingleAddressWallet first acquires mu and then opens a database transaction. e.g. in FundV2Transaction it first locks the mutex and then locks the outputs in the store while holding the mutex. In UpdateChainState the transaction is already open and then it acquires mu.

So all it takes is for one goroutine to try and open a database transaction while holding the wallet lock and another one acquiring the wallet lock from within an open database connection. Which could in theory happen now if FundV2Transaction and UpdateChainState are called rapidly in parallel.

@peterjan peterjan force-pushed the pj/locked-outputs branch 3 times, most recently from 641887d to 3319301 Compare June 3, 2025 07:55
@peterjan
Copy link
Member Author

peterjan commented Jun 3, 2025

Renter tests are 🟢 - verified here https://github.com/SiaFoundation/renterd/pull/1896/checks

ChrisSchinnerl
ChrisSchinnerl previously approved these changes Jun 3, 2025
@n8maninger n8maninger force-pushed the pj/locked-outputs branch from 4020d67 to 545526f Compare June 5, 2025 21:31
@n8maninger n8maninger merged commit df8379c into master Jun 5, 2025
9 checks passed
@n8maninger n8maninger deleted the pj/locked-outputs branch June 5, 2025 21:31
@github-project-automation github-project-automation bot moved this from In Progress to Done in Sia Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants