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

client/dcr: Identify unknown txs #2748

Merged
merged 4 commits into from
Jun 13, 2024
Merged

client/dcr: Identify unknown txs #2748

merged 4 commits into from
Jun 13, 2024

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Apr 26, 2024

This diff updates the DCR wallet to identify any unknown transaction and add it to the transaction history. Redeems, refunds, and bond refund transactions are updated to use an external address instead of an internal one. Otherwise, the RPC wallet's listsinceblock command would not return them.

Closes #2744

This diff updates the DCR wallet to identify any unknown transaction and
add it to the transaction history. Redeems, refunds, and bond refund
transactions are updated to use an external address instead of an internal
one. Otherwise, the RPC wallet's listsinceblock command would not return
them.
@martonp martonp marked this pull request as ready for review May 21, 2024 09:33
@JoeGruffins
Copy link
Member

Testing well. I notice that the amounts we use from the database are whole numbers while I guess the amounts we id have the tx fees removed. Here I made some swaps and then restored from seed and made another, you can see the difference:

image

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved

if tx.Send && allOutputsPayUs(msgTx) && len(msgTx.TxIn) == 1 {
return &asset.WalletTransaction{
Type: asset.Split,
Copy link
Member

Choose a reason for hiding this comment

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

Could be a SelfSend I guess, but split is more likely.

Comment on lines 6542 to 6547
go func() {
dcr.tipMtx.RLock()
tip := dcr.currentTip
dcr.tipMtx.RUnlock()
dcr.syncTxHistory(ctx, uint64(tip.height))
}()
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be asynchronous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it can only be a longer running function on the initial call in Connect. After your rescanning PR gets merged though and this can get reset to 0, it could become a longer running function whenever the rescan completes.

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
@martonp
Copy link
Contributor Author

martonp commented May 25, 2024

Testing well. I notice that the amounts we use from the database are whole numbers while I guess the amounts we id have the tx fees removed. Here I made some swaps and then restored from seed and made another, you can see the difference:

We don't have access to the counterparty's swap transaction, so we cannot know the fee. I guess we could implement it for full nodes though.

@JoeGruffins
Copy link
Member

We don't have access to the counterparty's swap transaction, so we cannot know the fee. I guess we could implement it for full nodes though.

If we have the redemption or refund transaction isn't the fee in - out?

@martonp
Copy link
Contributor Author

martonp commented May 28, 2024

If we have the redemption or refund transaction isn't the fee in - out?

Yes, but the SPV wallet does not have access to the counterparty's swap transaction, so how will we get the input?

@JoeGruffins
Copy link
Member

I thought we would have the input witness data as part of the raw transaction. Let me make sure...

@JoeGruffins
Copy link
Member

I'm pretty sure we can get the tx fee if we have the raw tx of the transaction. I just tried this with a native wallet and it worked:

	amtIn := int64(0)
	for _, in := range msgTx.TxIn {
		amtIn += in.ValueIn
	}

	amtOut := int64(0)
	for _, out := range msgTx.TxOut {
		amtOut += out.Value
	}

	fee := amtIn-amtOut

@martonp
Copy link
Contributor Author

martonp commented May 30, 2024

I'm pretty sure we can get the tx fee if we have the raw tx of the transaction. I just tried this with a native wallet and it worked:

It works, thanks.

Comment on lines +6171 to +6174
_, err = txHistoryDB.GetTx(txHash.String())
if err == nil {
continue
}
Copy link
Member

@JoeGruffins JoeGruffins May 31, 2024

Choose a reason for hiding this comment

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

I was adding this to zec and noticed here that the ListTransactions result looks like this:

Result:
{
 "transactions": [{                 (array of object) JSON array of objects containing verbose details of the each transaction
  "account": "value",               (string)          DEPRECATED -- Unset
  "address": "value",               (string)          Payment address for a transaction output
  "amount": n.nnn,                  (numeric)         The value of the transaction output valued in decred
  "blockhash": "value",             (string)          The hash of the block this transaction is mined in, or the empty string if unmined
  "blockindex": n,                  (numeric)         Unset
  "blocktime": n,                   (numeric)         The Unix time of the block header this transaction is mined in, or 0 if unmined
  "category": "value",              (string)          The kind of transaction: "send" for sent transactions, "immature" for immature coinbase outputs, "generate" for mature coinbase outputs, or "recv" for all other received outputs.  Note: A single output may be included multiple times under different categories
  "confirmations": n,               (numeric)         The number of block confirmations of the transaction
  "fee": n.nnn,                     (numeric)         The total input value minus the total output value for sent transactions
  "generated": true|false,          (boolean)         Whether the transaction output is a coinbase output
  "involveswatchonly": true|false,  (boolean)         Unset
  "time": n,                        (numeric)         The earliest Unix time this transaction was known to exist
  "timereceived": n,                (numeric)         The earliest Unix time this transaction was known to exist
  "txid": "value",                  (string)          The hash of the transaction
  "txtype": "value",                (string)          The type of tx (regular tx, stake tx)
  "vout": n,                        (numeric)         The transaction output index
  "walletconflicts": ["value",...], (array of string) Unset
  "comment": "value",               (string)          Unset
  "otheraccount": "value",          (string)          Unset
 },...],                                              
 "lastblock": "value",              (string)          Hash of the latest-synced block to be used in later calls to listsinceblock
}    

This is one vout of a transaction, what if a tx had two outputs paying to the wallet? Would that txid then appear twice? In that case the second vout would be passed over.

Copy link
Member

@JoeGruffins JoeGruffins May 31, 2024

Choose a reason for hiding this comment

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

I guess it's fine with idUnknownTx which takes the txid and pulls in everything. Nevermind.

Comment on lines 6034 to 6037
if err != nil {
dcr.log.Errorf("Error decoding txid %s: %v", tx.TxID, err)
return
dcr.log.Errorf("ExtractAddrs error: %w", err)
return false
}
Copy link
Member

Choose a reason for hiding this comment

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

Theres no error to check here.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ExtractAddrs error: %w -> ExtractAddrs error: %v

@buck54321 buck54321 merged commit 12a90a7 into decred:master Jun 13, 2024
5 checks passed
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.

dcr: wallet sees incoming tx and updates balance, but not tx history
4 participants