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

multi: use getTransaction grpc for confirmCoinbases. #334

Merged
merged 1 commit into from Jun 26, 2021

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented May 26, 2021

This reworks coinbase confirmation function to uses the getTransaction grpc to fetch transaction details and confirm them. The rescan begin height is now a coinbase maturity length of blocks from the lowest reported block with a tx having inaccurate confirmation information. Tests have been updated.

@dnldd dnldd force-pushed the add_confirm_coinbases_v2 branch 2 times, most recently from 0d1f21f to 0a1c123 Compare May 26, 2021 01:29
Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Added a few comments, mostly for myself to remember I need to re-review some of the points later.


The reason we originally used TxConfNotifications to track when coinbases become spendable was because of a race condition explained here in #222.

The old confirmCoinbases blocked until all of the coinbases were spendable. I don't think the new one is blocked, so it seems like it will be prone to the same race condition.

pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr_test.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
@dnldd
Copy link
Member Author

dnldd commented May 31, 2021

I don't think the issue described in #222 is going to be a problem here since gettxout was informing us of what the node knew about the provided tx output not factoring in the wallet's state concerning that output (the wallet could be updated late for example). The current solution being used in confirmCoinbasesV2 is calling the wallet's gettransaction grpc for the transactions of interest. If the wallet doesn't know about them yet it will have two more blocks to do so before a rescan is performed. The disparity we had before concerning the information the wallet and node had at a given time on a tx shouldn't be an issue now since we ask the wallet to confirm txs it will need to create the payout transaction directly, however we'll still have to confirm gettransaction works as expected and the expanded rescan range is adequate in mitigating the current tx confirmation issues being had with recieving tx confirmation notifications.

@dnldd dnldd force-pushed the add_confirm_coinbases_v2 branch from 0a1c123 to df8f0c3 Compare May 31, 2021 22:04
@jholdstock
Copy link
Member

Understood. Will deploy onto my test site and monitor for a few days.

@dnldd dnldd changed the title multi: add confirmCoinbasesV2. multi: use getTransaction grpc for confirmCoinbases. Jun 22, 2021
pool/hub.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
This reworks coinbase confirmation function to
uses the getTransaction grpc to fetch transaction
details and confirm them. The rescan begin height is
now a coinbase maturity length of blocks from the lowest
reported block with a tx having inaccurate confirmation
information. Tests have been updated.
@dnldd dnldd merged commit 0857388 into decred:master Jun 26, 2021
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

2 participants