Skip to content

Conversation

@ffranr
Copy link
Contributor

@ffranr ffranr commented Oct 13, 2022

Pull Request Checklist

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

@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch from 9a56042 to 37cb234 Compare October 13, 2022 21:55
@ffranr
Copy link
Contributor Author

ffranr commented Oct 13, 2022

@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.)

@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch from 37cb234 to 4aa83b9 Compare October 13, 2022 22:07
Copy link
Contributor

@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.

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

Copy link
Contributor

@positiveblue positiveblue left a comment

Choose a reason for hiding this comment

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

looks really nice @ffranr 🙏

@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch from 4aa83b9 to 37ef2a5 Compare October 18, 2022 16:37
@ffranr
Copy link
Contributor Author

ffranr commented Oct 18, 2022

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 ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch 2 times, most recently from bc91915 to 4a07bfd Compare October 18, 2022 20:07
@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch 4 times, most recently from 11fd04c to 6a21dd1 Compare October 24, 2022 14:48
@ffranr
Copy link
Contributor Author

ffranr commented Oct 25, 2022

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.

@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch 2 times, most recently from 25ac20d to 4516636 Compare November 7, 2022 16:15
@ffranr ffranr marked this pull request as ready for review November 7, 2022 16:15
Copy link
Contributor

@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.

Left a few comments and tried to answer your questions.

@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch from 4516636 to 31d9912 Compare November 10, 2022 16:59
@ffranr
Copy link
Contributor Author

ffranr commented Nov 10, 2022

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.

@ffranr ffranr requested a review from guggero November 10, 2022 17:03
Copy link
Contributor

@positiveblue positiveblue left a comment

Choose a reason for hiding this comment

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

👌

Copy link
Contributor

@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.

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 ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch from dbd2381 to 1e600d5 Compare November 11, 2022 16:04
@ffranr
Copy link
Contributor Author

ffranr commented Nov 11, 2022

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:

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

@ffranr ffranr requested a review from guggero November 14, 2022 15:08
Copy link
Contributor

@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.

tACK, LGTM 🎉

This commit adds a new RPC endpoint which returns a per-account list of account modification fees.
@ffranr ffranr force-pushed the 386-expose-historical-account-modification-fees-via-rpc branch from 1e600d5 to cd63205 Compare November 15, 2022 12:54
@ffranr
Copy link
Contributor Author

ffranr commented Nov 15, 2022

@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?

@ffranr ffranr requested a review from guggero November 15, 2022 12:59
@guggero
Copy link
Contributor

guggero commented Nov 15, 2022

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

@guggero guggero merged commit e65f78d into lightninglabs:master Nov 15, 2022
@ffranr
Copy link
Contributor Author

ffranr commented Nov 15, 2022

Thanks for the explanation!

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.

4 participants