Expose historical account action fees via RPC

[ffranr]

Pull Request Checklist

• LndServices minimum version has been updated if new lnd apis/fields are
  used.

[ffranr]

@guggero does my solution look as if it's going in the right direction?

(By the way, I haven't completed the logic yet and the formatting isn't correct.)

[guggero]

Approach looks good! Allowed myself to add some early comments.

[positiveblue]

looks really nice @ffranr 🙏

[ffranr]

These are the lnd transactions I'm trying to process: https://gist.github.com/ffranr/73b0624a96a93893a66ba73c5161b3b5

AccountCreation is at "block_height": 120. But there's a AccountModification for the same account at "block_height": 0. My methodology does not recognize this earlier (?) AccountModification.

Ordering over the time_stamp field seems to be correct. But I thought that time_stamp might be unreliable and that block_height should be checked first. Am I correct?

@positiveblue mentioned the following in chat:

this looks like: for some modification we are not setting the block height at all OR we update it after it gets "confrimed"

I think he's right. After regtest mine 12, block_height values are no longer zero.

In this instance, I will only sort on time_stamp going forward.

[ffranr]

I'm still stuck with a strange withdraw transaction. I would expect at least one is_our_address to be true. And I thought at least one output_type should be taproot.

Command:

pool_alice accounts withdraw --trader_key $TRADER_KEY --amt 50000 --sat_per_vbyte 5 --addr bcrt1p2agvu57xt3p6wut5xql59qpazhx2j02dswlprm2tv0ts2xp4xpvsed8wg8

addr is set to a random address.

{
            "tx_hash": "755b00c399d3bf8eec0fe68156af9b90d443436bbb9f282078aeae855a2a0205",
            "amount": "0",
            "num_confirmations": 0,
            "block_hash": "",
            "block_height": 0,
            "time_stamp": "1666710175",
            "total_fees": "0",
            "dest_addresses": [
                "bcrt1p2agvu57xt3p6wut5xql59qpazhx2j02dswlprm2tv0ts2xp4xpvsed8wg8",
                "bcrt1pr9tnxkd5txungm4tl4vjcl0mad3mu8cskyw96axnzla4j0qrwevsq5t4sg"
            ],
            "output_details": [
                {
                    "output_type": "SCRIPT_TYPE_PUBKEY_HASH",
                    "address": "bcrt1p2agvu57xt3p6wut5xql59qpazhx2j02dswlprm2tv0ts2xp4xpvsed8wg8",
                    "pk_script": "51205750ce53c65c43a77174303f42803d15cca93d4d83be11ed4b63d70518353059",
                    "output_index": "0",
                    "amount": "50000",
                    "is_our_address": false
                },
                {
                    "output_type": "SCRIPT_TYPE_PUBKEY_HASH",
                    "address": "bcrt1pr9tnxkd5txungm4tl4vjcl0mad3mu8cskyw96axnzla4j0qrwevsq5t4sg",
                    "pk_script": "512019573359b459b9346eabfd592c7dfbeb63be1f10b11c5d74d317fb593c037659",
                    "output_index": "1",
                    "amount": "142300",
                    "is_our_address": false
                }
            ],
            "raw_tx_hex": "0200000000010158b13fd365bee3ce6673efaae79bafdd59f7dba31df42b1353503c44a1b672ec0000000000000000000250c30000000000002251205750ce53c65c43a77174303f42803d15cca93d4d83be11ed4b63d70518353059dc2b02000000000022512019573359b459b9346eabfd592c7dfbeb63be1f10b11c5d74d317fb593c037659014047a40ba6132b9ab6da159aa7fd7bb814ee9f58751d5e50db07d92f8b3a7929f853c49f8488d0518a8404bd62ec164b7f724e554fa9c030839e1d8a4e7570da0100000000",
            "label": " poold -- AccountModification(acct_key=038709293544c27d37c632f15bc5a2a2d0ab6d0fc46e6fbc28ec899015d26c3f9c, expiry=false, deposit=false, is_close=false)",
            "previous_outpoints": [
                {
                    "outpoint": "ec72b6a1443c5053132bf41da3dbf759ddaf9be7aaef7366cee3be65d33fb158:0",
                    "is_our_output": false
                }
            ]
        },

lnd/lncli versions seems to be correct:

user@lightninglabs:~/pool$ reg_alice version
{
    "lncli": {
        "commit": "v0.15.3-beta",
        "commit_hash": "b4e7131bdb47531ad2f00ce345ddcdb58935bba5",
        "version": "0.15.3-beta",
        "app_major": 0,
        "app_minor": 15,
        "app_patch": 3,
        "app_pre_release": "beta",
        "build_tags": [
            "autopilotrpc",
            "signrpc",
            "walletrpc",
            "chainrpc",
            "invoicesrpc",
            "watchtowerrpc",
            "neutrinorpc",
            "monitoring",
            "peersrpc",
            "kvdb_postgres",
            "kvdb_etcd"
        ],
        "go_version": "go1.18.2"
    },
    "lnd": {
        "commit": "v0.15.3-beta",
        "commit_hash": "b4e7131bdb47531ad2f00ce345ddcdb58935bba5",
        "version": "0.15.3-beta",
        "app_major": 0,
        "app_minor": 15,
        "app_patch": 3,
        "app_pre_release": "beta",
        "build_tags": [
            "autopilotrpc",
            "signrpc",
            "walletrpc",
            "chainrpc",
            "invoicesrpc",
            "watchtowerrpc",
            "neutrinorpc",
            "monitoring",
            "peersrpc",
            "kvdb_postgres",
            "kvdb_etcd"
        ],
        "go_version": "go1.18.2"
    }
}

And I'm using the latest commit (+ my changes) for pool and subasta.

[guggero]

Left a few comments and tried to answer your questions.

[ffranr]

Thanks for the review @guggero and for answering my questions! I believe my latest update has addressed your comments, please let me know if I've missed anything.

[positiveblue]

👌

[guggero]

I tested this out locally and found a few things that didn't work. Would be nice if you could do a quick manual test run before requesting review, please.
Other than that it looks very good, can get this merged soon I think.

[ffranr]

Thank you for your latest review @guggero ! You're right I should always try out the code before I request a review. I'll make sure that I do.

I've tried out the latest commit with account creation and it went as expected.

Here's an example deposit label:

 "label": "poold -- {\"account\":{\"key\":\"034b2564b04f24e243ce54366b1b23eb9bf98cd97b2ad13e0b7fa5b093c30171e2\",\"action\":\"deposit\",\"expiry_height\":4439,\"output_index\":0,\"expiry_spend\":false,\"tx_fee\":1105,\"balance_diff\":200000}}",

And here's an example account creation label:

"label": "poold -- {\"account\":{\"key\":\"034b2564b04f24e243ce54366b1b23eb9bf98cd97b2ad13e0b7fa5b093c30171e2\",\"action\":\"create\",\"expiry_height\":4439,\"output_index\":0,\"expiry_spend\":false,\"tx_fee\":null,\"balance_diff\":200000}}",

Note \"tx_fee\":null for the account creation transaction because of this TODO:

pool/account/manager.go

Line 639 in 1e600d5

// TODO(ffranr): Tx inputs are required to calculate the

[guggero]

tACK, LGTM 🎉

[ffranr]

@guggero Thanks for the review. I've added txid as you suggested. I've tested by checking the RPC return after creating a pool account.

You mentioned a "byte reverse order problem", what is that?

[guggero]

You mentioned a "byte reverse order problem", what is that?

By convention the ID of a transaction (a.k.a. TXID) is the little endian representation of its SHA256 hash. Which means the hex notation seen in most block explorers is actually the reverse of the hex representation of the hash. So in lnd we normally use the raw hash when a gRPC field is of type bytes (which corresponds to []byte) but use the human-readable (e.g. reverse order) notation when showing it as a hex string (gRPC type of string). In some instances we even re-format things in the CLI just to have nice copy/pasteable TXIDs in the console output: https://github.com/lightninglabs/pool/blob/master/cmd/pool/account.go#L47

[ffranr]

Thanks for the explanation!