[wip]: add get transaction rpc

[gnapoli23]

Fixes #7323

Adds get transaction rpc call to get info about a specific transaction related to the wallet.
This is a draft (I'm a newcomer), I would like to know if I'm on the right track or not.

[positiveblue]

Hello @gnapoli23, I'm glad you are taking a look into this 🎉

I see you are creating a new rpc endpoint + cmd just for this. I wonder if we could rather extend what we already have. It can be worth to discuss what possibilities we have here and their trade off before committing to one of them

There are two things to take into account:

a) The rpc interface and server implementation
b) cli command that calls that rpc endpoint

For a) we could could reuse the current GetTransactionsRequest used in listchaintxns. We could add a new field repeat string tx_hash_list=4. The GetTransactions methods should then be updated to ensure that:

• If tx_hash_list is empty we behave like always. Using the same logic that is used right now.
• If tx_hash_list is not empty: we need to call our bitcoin backend to get info about each tx using its hash. The good news is that there is an RPC endpoint for that: GetTransaction. If the other filters are set we should fetch them by hash but ensure that they are filtered out if do not match them.

For b) we use urfave so we can add a cli.StringSliceFlag so we can repeat the argument with multiple values and we would be able to set the list in the rpc.

lncli listchaintxns --tx_hash="hash_1" --tx_hash="hash_2" ...

If we follow this route, we would not only solve this issue, but give an rpc interface to fetch multiple txs in one call, with multiple filters and even be able to fetch txs that are not ours.

NOTE: The GetTransactionRequest is also used in the SubscribeTransactions method. We would have to update its logic too or at least check that tx_hash_list is empty.

NOTE 2: To use the GetTransaction endpoint the bitcoin backend needs to be compiled with wallet support, but if we are using it for LND I think it HAS to have it anyway.

NOTE 3: I guess --txindex is advised here.

[ErikEk]

@gnapoli23, @positiveblue This is my code so far (#7381). Please go ahead copy whatever you need. There are still things to be done. Fix the hash conversion, calculate the correct transaction amount, list our inputs and code cleanup ofc. Maybe add tests too.

[ErikEk]

@gnapoli23 Reach out if you have any questions and I might be able to help.

[gnapoli23]

@ErikEk Thanks sir for sharing your code and for your willingness to help, really appreciate it.
@positiveblue Thanks for the explanation and the starting guidance, I reverted the draft commit and will work as suggested.
Btw, I was thinking also another thing. Can this kind of setting be useful?

message GetTransactionsByRangeRequest {
    int32 start_height = 1;
    int32 end_height = 2;
}

message GetTransactionsByHashesRequest {
    repeated string tx_hash_list = 1;
}

message GetTransactionsRequest {

    oneof search_criteria {
        GetTransactionsByHashesRequest tx_height_range = 1;
        GetTransactionsByHashesRequest tx_hashes = 2;
    }

    string account = 3;

}

This is just something i was thinking during looking again at protobuf definition, thought it could be another way to separate the search criteria. If not needed, I will go straight as suggested! Thanks.

[ErikEk]

@ErikEk Thanks sir for sharing your code and for your willingness to help, really appreciate it. @positiveblue Thanks for the explanation and the starting guidance, I reverted the draft commit and will work as suggested. Btw, I was thinking also another thing. Can this kind of setting be useful?

message GetTransactionsByRangeRequest {
    int32 start_height = 1;
    int32 end_height = 2;
}

message GetTransactionsByHashesRequest {
    repeated string tx_hash_list = 1;
}

message GetTransactionsRequest {

    oneof search_criteria {
        GetTransactionsByHashesRequest tx_height_range = 1;
        GetTransactionsByHashesRequest tx_hashes = 2;
    }

    string account = 3;

}

This is just something i was thinking during looking again at protobuf definition, thought it could be another way to separate the search criteria. If not needed, I will go straight as suggested! Thanks.

@gnapoli23 That would break the api since we assume message index 1 and 2 to be start and end height. And I think GetTransactions shouldn't require any parameters to be set. Plus I can see the utility of both tx hashes and height interval parameters being available at the same time.

[gnapoli23]

@ErikEk yes I guess so.
@positiveblue I updated the code as suggested by adding tx_hash as a string slice in lncli listchaintxns cmd and relative refs to GetTransactions/ListTransactionDetails. In the btcwallet implementation I left a // TODO comment to update the logic: how should we handle it? Which parameter has highest priority? I guess the height limits, so I would like to understand if tx_hash_list should match somehow with them or should follow a separate logic.

[ErikEk]

@gnapoli23 How is going? Do you need help finishing this. I don't mind completing it if you have a busy schedule.

[gnapoli23]

@gnapoli23 How is going? Do you need help finishing this. I don't mind completing it if you have a busy schedule.

Hi @ErikEk and thanks for contacting me, I have been OOO in previous weeks so I couldn't work on it. I just rebased on top of latest commit, and I'll check where I was before being unavailable. I will try to move forward and if I run into any problems I will ping you. Thanks a lot for your support, really appreciate 🙏🏻

[ErikEk]

It has been three months now. I will continue the work in #7654.

[guggero]

Replaced by #7654.