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

wallet: add GetTransaction returning data for any transaction #779

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bjarnemagnussen
Copy link
Contributor

This PR adds a method to the wallet that allows to get transaction data on any transaction given its ID. If the transaction exists in the transaction store, it will be returned with the data from the database, otherwise data from the backend is returned as a TransactionSummary.

This method can be useful in situations where one is interested in a specific transaction only, instead of e.g. all transactions from the GetTransactions method. It also adds new functionality as it can return data from the backend of a transaction not in the internal wallet.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the added feature.

I wonder though how useful this is, if you need to rely on the txindex to be enabled for this to work? The above GetTransactions works without index, is just much slower (for obvious reasons). Would that be used as a fall back?

cc @Roasbeef

wallet/wallet.go Outdated
bool, error) {

var res GetTransactionResult
chainClient := w.chainClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to hold the w.chainClientLock mutex here.

wallet/wallet.go Outdated
j := len(blockHashBytes) - i - 1
reversed[j] = n
}
blockHash, err = chainhash.NewHash(reversed)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can use chainhash.NewHashFromStr directly instead of reversing it yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I cannot believe I overlooked this method...

if err != nil {
return nil, false, err
}
case *chain.NeutrinoClient:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to include the Neutrino client here, we bailed out earlier for that chain client type.

wallet/wallet.go Outdated
return false, nil
}

return w.TxStore.RangeTransactions(txmgrNs, bestHeight,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: multi-line method call formatting.

wallet/wallet.go Outdated
// GetTransaction returns data of any transaction given its id. A mined
// transaction is returned in a Block structure which records properties about
// the block. A bool is also returned to denote if the transaction previously
// existed in the database or not.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mention that the transaction index needs to be turned on for this to work?

@bjarnemagnussen
Copy link
Contributor Author

@guggero Thanks a lot for taking your time to give me the feedback! I have added a commit with changes.

The idea with this function is to a) provide a more performant lookup of a single relevant transaction given its id, but also b) as a convenience function if the transaction index is set on the backend to allow polling for an arbitrary transaction.

Regarding the transaction index I have made a little code change to continue fetching a transaction from the database, even if the transaction index is not set on the backend. Only if the transaction cannot be found in the database and the backend does not have the transaction index will the function call fail.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Can you please squash the commits?
Other than a few minor nits, this looks good to me 🎉

wallet/wallet.go Outdated
// In case the transaction is unconfirmed it could due to race condition
// confirm during the time we receive it from the backend and check it
// in the database.
// We therefore store the best block height and will use it for quering the
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/quering/querying (and also line length)

wallet/wallet.go Outdated
return nil, false, err
}

var res GetTransactionResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: move to just above where this is actually used? So before the switch?

wallet/wallet.go Outdated
Comment on lines 2484 to 2491
// TODO: probably should make RangeTransactions not
// reuse the details backing array memory.
dets := make([]wtxmgr.TxDetails, len(details))
copy(dets, details)
details = dets

for i := range details {
dbTx := makeTxSummary(dbtx, w, &details[i])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, could this just be:

for i, txDetail := range details {
        txDetail := txDetail
        dbTx := makeTxSummary(dbtx, w, &txDetail)

to avoid copying the whole slice?

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 was a bit confused about this TODO comment. Note though, that I used the code from the GetTransactions method above here. But I have made your code suggestion in the squashed commit.

@bjarnemagnussen
Copy link
Contributor Author

Thanks @guggero! I have incorporated your suggestions in the squashed commit.

wallet/wallet.go Outdated
// existed in the database or not.
// The backend must have a transaction index to poll a confirmed transaction
// that does not exist in the internal wallet.
func (w *Wallet) GetTransaction(txHash *chainhash.Hash) (*GetTransactionResult,
Copy link
Member

Choose a reason for hiding this comment

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

Unclear w.r.t the purpose of this method: it seems to rely on the RPC calls, and will return a reply even if the wallet isn't found in the wallet.

Where would this be used that https://pkg.go.dev/github.com/btcsuite/btcwallet/wtxmgr#Store.TxDetails wouldn't be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be useful for applications that use btcwallet as dependency. E.g. in LND a remotely force closed transaction is not added to the wallet/store and therefore won't show up via GetTransactions. It could therefore be convenient to have a function that gets the transaction from the wallet/store if it exists and falls back to the backend otherwise.

wallet/wallet.go Outdated Show resolved Hide resolved
@guggero
Copy link
Collaborator

guggero commented Dec 6, 2023

This can now rebased after #890 is in. The only remaining diff would be that if we don't have the transaction in the wallet that we would go and ask the chain backend for it.

@mrfelton mrfelton force-pushed the upstream/feat/add-gettransaction branch 4 times, most recently from bb1f86b to 55913d0 Compare May 9, 2024 15:50
@guggero guggero self-requested a review May 10, 2024 07:25
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Main comment is about making sure this doesn't panic for Neutrino clients. Not sure we can give them a useful response though, as it would require re-scanning the chain for the transaction. So not sure what to do about that. What would this extended call be used for in your use case in the first place?

wallet/wallet.go Outdated
var bestHeight int32
var blockHash *chainhash.Hash

var summary *TransactionSummary
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can unify all these into a single var ( ... ) block.

Choose a reason for hiding this comment

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

Updated in updated commit

wallet/wallet.go Outdated
summary = &TransactionSummary{
Hash: &txHash,
Transaction: txRaw,
MyInputs: make([]TransactionSummaryInput, 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can just leave those nil instead of creating an empty slice IMO.

Choose a reason for hiding this comment

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

updated in latest commit

if err != nil {
return nil, err
}
case *chain.BitcoindClient:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about Neutrino? Those clients would currently panic on the txResult.Hex access below.

Copy link

@mrfelton mrfelton May 15, 2024

Choose a reason for hiding this comment

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

I've added this Neutrino check back in, although I originally removed it due to your prior comment!

wallet/wallet.go Outdated

// Calculate the number of confirmations.
if res.Height != -1 {
res.Confirmations = bestHeight - res.Height + 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is correct. bestHeight and res.Height are both the exact same value at this point.

Choose a reason for hiding this comment

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

Why would they have the same value? bestHeight is the current sync height, res.Height is the height at which the transaction was found.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See line 2639.

Copy link

@mrfelton mrfelton May 15, 2024

Choose a reason for hiding this comment

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

lol. I've updated this to fetch the chain tip and then compare the heights to get the number of confirmations.

@mrfelton mrfelton force-pushed the upstream/feat/add-gettransaction branch from 55913d0 to d7cb37f Compare May 15, 2024 15:11
@mrfelton
Copy link

mrfelton commented May 15, 2024

Thanks for the update. Main comment is about making sure this doesn't panic for Neutrino clients. Not sure we can give them a useful response though, as it would require re-scanning the chain for the transaction. So not sure what to do about that. What would this extended call be used for in your use case in the first place?

The use case is as @bjarnemagnussen mentioned in #779 (comment). but more specifically, to enhance LND's GetTransaction RPC to be able to fetch transactions that are not known to LND (this is a customisation we have had applied to lnd for a while)

@mrfelton mrfelton force-pushed the upstream/feat/add-gettransaction branch 2 times, most recently from 8f64458 to 0ad5f81 Compare May 15, 2024 15:46
@mrfelton mrfelton force-pushed the upstream/feat/add-gettransaction branch from 0ad5f81 to 9e52443 Compare May 15, 2024 15:46
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

but more specifically, to enhance LND's GetTransaction RPC to be able to fetch transactions that are not known to LND

Why not just use getrawtransaction then? GetTransaction (an API method of the wallet) is intended only to fetch transactions that are relevant to the wallet.

This PR takes that API method and adds in additional network calls (and for neutrino new p2p messages being sent), which significantly expands the scope of the method.

@mrfelton
Copy link

mrfelton commented Jul 3, 2024

That's certainly another valid approach, and is actually more akin to what we are doing now (we are no longer activly using this enhancement).

GetTransaction (an API method of the wallet) is intended only to fetch transactions that are relevant to the wallet.

IIRC, the transactions we were interested in were at one point in some way relevant to the wallet (e.g. a remote force close transaction, or transactions related to RBF, abandoned transactions from failed channel opens) Part of the rationale for making this accessible through LND rather than calling through to Bitcoind directly was for simplicity. LND generally provides everything needed to operate on top of it, and there are multiple instances where LND proxies through to Bitcoind to make something from the underlaying available. We did not want to open up direct connections to our Bitcoind instances and complicate things with needing to manage the multiple connections.

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.

5 participants