Upgrade dcrwallet to master branch (future version v4) #48
Conversation
|
still getting same error after purchasing a new ticket i have also confirmed i have the right branch |
|
Can you see this statement logged: |
no, not anywhere in the logs for now |
It implies that this is a new error. Triggered by different issues. I will look into it. |
|
The whole vsp implementation seems to have a bug. Its initiates the first VSP fee payment which is successful, it then goes to initiate more payment requests at intervals which all fail because the first one was successful. |
dmigwi
changed the title
|
The title has been changed to Upgrade dcrwallet to version v4 because that is the main fix that resolves the ticket issues. |
dmigwi
force-pushed
the
reconcile-payment-error
branch
from
c31b2c7
to
860029e
Compare
|
Cryptopower currently requires go 1.19 to build, but this PR require go1.20, means the go mod would need to be updated |
Update the workflow cache version
|
The ticket buying error seems to have been finally resolved: |
dmigwi
force-pushed
the
reconcile-payment-error
branch
5 times, most recently
from
87e9639
to
527a113
Compare
dmigwi
force-pushed
the
reconcile-payment-error
branch
from
527a113
to
000de69
Compare
|
i'm unable to vote on treasury spending 
|
| // Default policy for fee payments unless another is provided by the | ||
| // caller. | ||
| Policy *Policy |
There was a problem hiding this comment.
Do callers provide policy anywhere? Looks like all methods have been modified to no longer pass a policy.
There was a problem hiding this comment.
Upstream uses a default policy for all ticket purchases. Because here we ought to purchase tickets from any account, a default policy can't work here and thus the work around.
| // SetAccountInfo set the account purchase information | ||
| func (c *Client) SetAccountInfo(feeAcc, changeAcc int32) { | ||
| c.policy.FeeAcct = uint32(feeAcc) | ||
| c.policy.ChangeAcct = uint32(changeAcc) | ||
| } |
There was a problem hiding this comment.
Could separate processes try to set this info, leading to unexpected results? Why don't callers provide the policy to use, at the point the policy is needed?
There was a problem hiding this comment.
By separate processes, do you two mean instances where two source accounts are being used simultaneously? Its always been one account at a go.
Also this account information is set before the ticket buying loop starts, so we don't have any scenario of multiple synchronous writes to those variables would happen.
|
@dreacot and @itswisdomagain much of the code introduced here is copied word for word from here: https://github.com/decred/dcrwallet/tree/master/internal/vsp The reason why its copied, is because the code is implemented in an |
dmigwi
changed the title
| if ok && currentChoice == strings.ToLower(choiceID) { | ||
| // Do not set the same choice again | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Might it be necessary to continue even if it's the same value? Below, the specified ticket or all unvoted tickets are updated with the vsp to use this choice. I'm wondering if it's possible that a ticket in that list doesn't already have the correct vote choice set with the vsp.
There was a problem hiding this comment.
The purpose of this is to prevent unnecessary API calls. It does this by preventing the existing choice being set again.
I don't think setting the same choice again is necessary.
| for agendaID, choiceID := range choices { | ||
| if agendaID == d.Vote.Id { | ||
| votingPreference = choiceID | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This look correct?
votingPreference = choices[d.Vote.Id]
There was a problem hiding this comment.
This is double query; during the map for loop, choices[d.Vote.Id] already exists as choiceID.
| @@ -181,7 +177,7 @@ func (asset *Asset) SetVoteChoice(agendaID, choiceID, hash, passphrase string) e | |||
| firstErr = err | |||
There was a problem hiding this comment.
One of the new dcrwallet commits since v3.0.1 abolished the concept of firstErr and returns any error encountered immediately. We should adopt that. Can possibly create a separate issue but generally, when upgrading the dcrwallet dep, I often do a comparison of the code changes between the last version in our go.mod and the new version we're introducing. The comparison for this version bump is https://github.com/decred/dcrwallet/compare/v3.0.1..a87fa843495e (file changes) and decred/dcrwallet@v3.0.1...a87fa84 (commits).
There was a problem hiding this comment.
This could be done app wide so a separate issue can best address this.
| VSPFeePaymentProcess: func(ctx context.Context, ticketHash *chainhash.Hash, feeTx *wire.MsgTx) error { | ||
| return vspClient.Process(ctx, ticketHash, feeTx, asset.GetvspPolicy(account)) | ||
| return vspClient.Process(ctx, ticketHash, feeTx) | ||
| }, |
There was a problem hiding this comment.
VSPFeePaymentProcess: vspClient.Process
There was a problem hiding this comment.
VSPFeePaymentProcess requires a different method signature than supported by vspClient.Process.
| log.Info("Setting the ticket(s) purchasing account info") | ||
| vspClient.SetAccountInfo(account, account) |
There was a problem hiding this comment.
Upstream dcrwallet doesn't modify the fee and change accounts after the vsp is created with a policy that defines those values. All subsequent calls to wallet.PurchaseTickets continue to use the same accounts initially set in the policy, even if the PurchaseTicketsRequest uses a different source account.
Modifying that policy everytime this PurchaseTickets method is called is potentially problematic. There's nothing to ensure that this method isn't called twice in quick succession, with the callers passing different accounts in each call, and ending up getting unexpected outcomes.
I understand the desire to use the same purchasing account as fee and change account but perhaps go with dcrwallet's style for now and never change those account values once they've been set at the time of creating the vspClient.
Or come up with a safer way to make this value dynamic.
I remember I'd discussed this issue with the upstream repo maintainer before, even opened a PR to resolve it then.
decred/dcrwallet#2129
There was a problem hiding this comment.
Our implementation here reuses the VSP clients therefore we cannot set default fee and change accounts when creating a client. In dcrwallet, they use default policy accounts but that doesn't apply to use because a user can manage many wallets with many accounts and any of the accounts can be selected as the source of funds.
There's nothing to ensure that this method isn't called twice in quick succession theoretically this is possible but how vspClient.SetAccountInfo( is used in the code, that case scenario is impossible to replicate. I feel adding those extra safeguards will only make the method more complicated and error prone.
..this account information is set before the ticket buying loop starts, so we don't have any scenario of multiple synchronous writes to those variables would happen...
That account information is set only once together with other ticket information and it doesn't happen more than one per given ticket purchase trigger. Only the following methods set the account information:
func (asset *Asset) PurchaseTickets(account, numTickets int32, vspHost, passphrase string, vspPubKey []byte) ([]*chainhash.Hash, error) {func (asset *Asset) runTicketBuyer(ctx context.Context, passphrase string, cfg *TicketBuyerConfig) error {
None of them can execute in multiple quick successions and if it does, this is a bug where that is triggered.
|
@dmigwi seem like this pr not need, please close it |
|
PR superseded by this #544 |
Closes #47
Only reconcile VSP fees if an error occurred when processing it initially