multi: fix chain notif. blocking by pmt processor. #324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jholdstock
reviewed
Apr 26, 2021
dnldd
force-pushed
the
fix_chain_notif_blocking
branch
from
d86e97e
to
862d50d
Compare
jholdstock
reviewed
Apr 28, 2021
dnldd
force-pushed
the
fix_chain_notif_blocking
branch
from
862d50d
to
3e298c1
Compare
This incorporates wallet rescanning into the payment process to restore tx conf functionality when the wallet gets into an error state. Associated tests have been added.
dnldd
force-pushed
the
fix_chain_notif_blocking
branch
from
3e298c1
to
cd7a037
Compare
This dedicates a lifecycle process for payment processing and updates the necessary configs. Payment processing is now triggered via signals from the chainstate to the payment manager. Associated tests have been added.
dnldd
force-pushed
the
fix_chain_notif_blocking
branch
from
cd7a037
to
3260806
Compare
|
Tried deploying the latest, along with my additional logging, and I'm seeing this: |
This updates the dividend payment process to get the lowest failing tx conf height to use for the start of a wallet rescan to ensure all failing tx confs are covered.
dnldd
force-pushed
the
fix_chain_notif_blocking
branch
from
3260806
to
f734f31
Compare
jholdstock
reviewed
May 5, 2021
This fixes an issue where the tx hashes sent in the notification request were duplicates of each other because of taking slice of the iterator variable. The stopAfter param was also updated to send over the correct parameter (conf count) instead of a block height.
jholdstock
requested changes
May 7, 2021
There was a problem hiding this comment.
Completed a thorough review, comments attached.
As we have discussed on Matrix, I am really not a fan of this change because we are adding a large amount of code to work around a bug in dcrwallet, which should really be fixed there. In order to ensure we come back and remove this code later, I would like to propose two things:
- We add a good, lengthy comment to the code, to explain the dcrwallet bug and why the rescan is necessary.
- We immediately open a new issue on the dcrpool repo to remove this code as soon as Transaction not spendable dcrwallet#1740 is fixed. It should link to this PR and to the dcrwallet issue for reference.
jholdstock
requested changes
May 12, 2021
dnldd
force-pushed
the
fix_chain_notif_blocking
branch
from
c5caa49
to
e673794
Compare
jholdstock
approved these changes
May 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes chain notification messages being blocked by payment processing calls when tx confirmations are not accurately being reported.
The fix includes:
ensuring a payment processing call concludes before starting another one via the processing flag.
calling the payment process in a goroutine to avoid blocking the chain state process.
keeping track of tx confirmation failures and starting a wallet rescan if the failures exceed threshold.
Associated tests have been updated.