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

Don't lose unconfirmed transactions. #1586

Merged
merged 1 commit into from Jun 18, 2019

Conversation

Projects
None yet
2 participants
@nopara73
Copy link
Collaborator

commented Jun 18, 2019

Closes #1580
Closes #1541
Closes #1514
Closes #1558
Closes #1536

It's a draft, because I want to either make a nice PR or brake this down into multiple parts.

This fixes the "spent" issue with unconfirmed transactions. The issue wasn't that we serialized them improperly, but we deserialized and then serialized them before we would've initialized them properly.

@nopara73 nopara73 marked this pull request as ready for review Jun 18, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 18, 2019

I changed my mind. I started to do the MempoolStore in the same way as the IndexStore, but that's a larger work I don't want to mix into here. So the changes:

  • I added a SendCount that makes sure the WalletService won't stop until a transaction send is in progress. For this I had to remove the disposing and add a StopAsync instead.
  • I use the TransactionProcessingLock more consistently, not only when mempool tx arrives.
  • I don't let the transactions to be serialized until the unconfirmed transactions are deserialized. (UnconfirmedTransactionsInitialized)
  • I make sure to serialize the transactions more often.
{
if (!_disposedValue)
while (Interlocked.Read(ref SendCount) != 0) // Make sure to wait for send to finish.

This comment has been minimized.

Copy link
@lontivero

lontivero Jun 18, 2019

Collaborator

I think a ManualResetEvent could be better than waiting actively in a while loop. Anyway, this will work well of course.

@lontivero
Copy link
Collaborator

left a comment

This PR looks good to me.

What worries me is the other branch, the one used to send the INV message because I think the same problem applies there.

@nopara73 nopara73 merged commit 4aca140 into zkSNACKs:master Jun 18, 2019

3 of 4 checks passed

Wasabi.Linux #20190618.8 failed
Details
CodeFactor No issues found.
Details
Wasabi.Osx #20190618.8 succeeded
Details
Wasabi.Windows #20190618.8 succeeded
Details

@hkjn hkjn referenced this pull request Jun 27, 2019

Closed

Miscalculated balance #1637

@nopara73 nopara73 deleted the nopara73:unconf branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.