client/asset: correct available and locked balance calculations#548
Conversation
itswisdomagain
force-pushed
the
balance-report-correction
branch
from
a27994b to
a424c1c
Compare
itswisdomagain
force-pushed
the
balance-report-correction
branch
from
67dc9f0 to
69dcbb3
Compare
itswisdomagain
changed the title
| dcr.fundingMtx.Lock() | ||
| defer dcr.fundingMtx.Unlock() | ||
| for _, outPoint := range lockedOutpoints { | ||
| opID := outpointID(&outPoint.Hash, outPoint.Index) | ||
| utxo, found := dcr.fundingCoins[opID] | ||
| if found { | ||
| sum += utxo.op.value | ||
| continue | ||
| } | ||
| txOut, err := dcr.node.GetTxOut(&outPoint.Hash, outPoint.Index, true) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if txOut == nil { | ||
| // Must be spent now? | ||
| dcr.log.Debugf("ignoring output from listlockunspent that wasn't found with gettxout. %s", opID) | ||
| continue |
There was a problem hiding this comment.
There was a dcrwallet fix that fixed this issue, making this gettxout check no longer necessary, correct? Can you please call that out in github comments and a commit message to justify change? Just note that git commit messages may refer to commit hashes, but not github links.
There was a problem hiding this comment.
Right. Went back to review the related discussion in #471 and #453 and maybe I should bring back the gettxout check, especially because of your last comment on 471.
It seems dcrwallet's listlockunspent still returns coins that are already spent, although that should have been fixed. If that's the case, calling gettxout for each coin returned from listlockunspent helps to prevent including a spent output in the locked balance calculation.
Worth noting again though, especially as per this comment on 453, gettxout may not identify spent outputs if the spending tx is still in mempool, so even with gettxout, we might still have cases where listlockunspent returns a spent output and gettxout does not call this out. If gettxout does return nil for such output, we'd know it's spent and ignore it.
There was a problem hiding this comment.
I no longer see the issue in #471. Has been a while actually.
@buck54321 may need to weight in here as the new logSplitFunds probably enters into this.
There was a problem hiding this comment.
I think it's fixed via decred/dcrwallet#1753, but I'm nervous about a regression. Let's go with it as-is here and see if we run into any trouble
Also, remove the now-obsolete `confs` arg from the `dcr.fund` method. Can be restored later if necessary, but the fundConf requirement was retired in commit 5d54e9d, allowing 0-conf utxos to be selected in funding transactions.
dcrwallet's listlockedunspent was previously found to sometimes return spent outputs, which causes the calculated locked balance amount to be higher than it actually is. This has been fixed upstream in 57a9016623fbf5033a7dffd748b63d44218529ba, ensuring that listlockedunspent only returns unspent outputs. Thus, the locked balance calculation in dcr.lockedAtoms() now fully relies on the results of the listlockedunspent cmd to determine locked unspent outputs, instead of cross-checking the returned results against the gettxout rpc.
itswisdomagain
force-pushed
the
balance-report-correction
branch
from
69dcbb3 to
defe372
Compare
3 participants
Recent testing with the simnet trade harness uncovered this bug with balance reporting.
dcr wallet's
listlockedunspentwas previously found to sometimes return spent outputs, which causes the calculated locked balance amount to be higher than it actually is (see #453). This has been fixed in decred/dcrwallet#1753, ensuring thatlistlockedunspentonly returns unspent outputs. Thus, the locked balance calculation indcr.lockedAtoms()now fully relies on the results of thelistlockedunspentcmd to determine locked unspent outputs, instead of cross-checking the returned results against thedcr.fundingCoinsmap and thegettxoutrpc.The
confsarg is also removed from thedcr.fundmethod as it was pretty much made obsolete when the fundConf requirement was eliminated in #499. The method's doc is updated to indicate that utxos with 1+ confs are given preference when selecting new tx inputs, and unconfirmed utxos are included in the selection if confirmed utxos are insufficient.