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

coin moves #3614

Merged
merged 42 commits into from May 12, 2020
Merged

coin moves #3614

merged 42 commits into from May 12, 2020

Conversation

niftynei
Copy link
Collaborator

@niftynei niftynei commented Apr 2, 2020

This PR adds a new notification type, coin_movement to c-ligthtning. By recording these notifications one may construct a canonical ledger for coin movements through a c-lightning node.

Cribbed from the PLUGINS.md update that this PR entails:

coin_movement

A notification for topic coin_movement is sent to record the
movement of coins. It is only triggered by finalized ledger updates,
i.e. only definitively resolved HTLCs or confirmed bitcoin transactions.

{
	"coin_movement": {
		"version":1,
		"node_id":"03a7103a2322b811f7369cbb27fb213d30bbc0b012082fed3cad7e4498da2dc56b",
		"movement_idx":0,
		"type":"chain_mvt",
		"account_id":"wallet",
		"txid":"0159693d8f3876b4def468b208712c630309381e9d106a9836fa0a9571a28722", // (`chain_mvt` type only, mandatory)
		"utxo_txid":"0159693d8f3876b4def468b208712c630309381e9d106a9836fa0a9571a28722", // (`chain_mvt` type only, optional)
		"vout":1, // (`chain_mvt` type only, optional)
		"payment_hash": "xxx", // (either type, optional on `chain_mvt`)
		"part_id": 0, // (`channel_mvt` type only, mandatory)
		"credit":"2000000000msat",
		"debit":"0msat",
		"tag":"deposit",
		"blockheight":102, // (`channel_mvt` type only. may be null)
		"timestamp":1585948198,
		"unit_of_account":"btc"
	}
}

version indicates which version of the coin movement data struct this
notification adheres to.

node_id specifies the node issuing the coin movement.

movement_idx is an increment-only counter for coin moves emitted by this node.

type marks the underlying mechanism which moved these coins. There are two
'types' of coin_movements:

  • channel_mvts, which occur as a result of htlcs being resolved and,
  • chain_mvts, which occur as a result of bitcoin txs being mined.

account_id is the name of this account. The node's wallet is named 'wallet',
all channel funds' account are the channel id.

txid is the transaction id of the bitcoin transaction that triggered this
ledger event. utxo_txid and vout identify the bitcoin output which triggered
this notification. (chain_mvt only) In most cases, the utxo_txid will be the
same as the txid, except for spend_track notficiations. Notifications tagged
chain_fees and journal_entry do not have a utxo_txid as they're not
represented in the utxo set.

payment_hash is the hash of the preimage used to move this payment. Only
present for HTLC mediated moves (both chain_mvt and channel_mvt)
A chain_mvt will have a payment_hash iff it's recording an htlc that was
fulfilled onchain.

part_id is an identifier for parts of a multi-part payment. useful for
aggregating payments for an invoice or to indicate why a payment hash appears
multiple times. channel_mvt only

credit and debit are millisatoshi denominated amounts of the fund movement. A
'credit' is funds deposited into an account; a debit is funds withdrawn.

tag is a movement descriptor. Current tags are as follows:

  • deposit: funds deposited
  • withdrawal: funds withdrawn
  • chain_fees: funds paid for onchain fees. chain_mvt only
  • penalty: funds paid or gained from a penalty tx. chain_mvt only
  • invoice: funds paid to or recieved from an invoice. channel_mvt only
  • routed: funds routed through this node. channel_mvt only
  • journal_entry: a balance reconciliation event, typically triggered
    by a penalty tx onchain. chain_mvt only
  • onchain_htlc: funds moved via an htlc onchain. chain_mvt only
  • pushed: funds pushed to peer. channel_mvt only.
  • spend_track: informational notification about a wallet utxo spend. chain_mvt only.

blockheight is the block the txid is included in. chain_mvt only. In the
case that an output is considered dust, c-lightning does not track its return to
our wallet. In those cases, the blockheight will be null, as they're recorded
before confirmation.

The timestamp is seconds since Unix epoch of the node's machine time
at the time lightningd broadcasts the notification.

unit_of_account is the 'currency' this coin movememnt is denominated in.

Resolves #3588

@niftynei niftynei added this to the 0.8.2 milestone Apr 2, 2020
@niftynei niftynei requested a review from rustyrussell Apr 2, 2020
@niftynei niftynei requested a review from cdecker as a code owner Apr 2, 2020
@niftynei niftynei force-pushed the nifty/money-movements branch 2 times, most recently from a6ad47c to 9fe8217 Compare Apr 4, 2020
@niftynei niftynei force-pushed the nifty/money-movements branch 2 times, most recently from 3b74b16 to 5a99ad9 Compare Apr 8, 2020
Copy link
Contributor

@rustyrussell rustyrussell left a comment

OK, I took a careful look since this is a brand new API. Though to be fair I didn't review all the calling paths in sufficient detail (there are many!).

So mainly minor (though, sorry, invasive) proposals. I really like this idea of a comprehensive API for coin moves, though, and it'd be great to get it in 0.8.2.

@@ -292,6 +292,9 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
hout->am_origin = am_origin;
if (am_origin)
hout->partid = partid;
else
hout->partid = 0;

Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

I disagree with this: partid should never be accessed if !am_origin. Your API should reflect this, too: don't output partid at all if there's no corresponding payment.

wallet/db.c Outdated
@@ -596,6 +596,9 @@ static struct migration dbmigrations[] = {
* Turn anything in transition into a WIRE_TEMPORARY_NODE_FAILURE. */
{SQL("ALTER TABLE channel_htlcs ADD localfailmsg BLOB;"), NULL},
{SQL("UPDATE channel_htlcs SET localfailmsg=decode('2002', 'hex') WHERE malformed_onion != 0 AND direction = 1;"), NULL},
/* For incoming HTLCs, we now keep track of whether or not we provided
* the preimage for it, or not. */
{SQL("ALTER TABLE channel_htlcs ADD we_filled INT;"), NULL},
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

I think INTEGER is preferred to INT in SQL, but I note we use both so maybe I'm wrong?

Copy link
Member

@cdecker cdecker Apr 9, 2020

Choose a reason for hiding this comment

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

sqlite3 is very lax with regards to its type-system, they're effectively aliases. In postgres this would be an s32, so we need to make sure that doesn't break.

Copy link
Collaborator Author

@niftynei niftynei May 8, 2020

Choose a reason for hiding this comment

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

this has been addressed. @rustyrussell was there another comment that was overlooked?

db_bind_int(stmt, 5, 1);
else
db_bind_null(stmt, 5);

Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

I prefer 0/1 rather than NULL/1? Make we_filled a bool * and then do:

/* Set for htlc_in */
if (we_filled)
   db_bind_int(stmt, 5, *we_filled);
else
  db_bind_null(stmt, 5);

Copy link
Collaborator Author

@niftynei niftynei May 8, 2020

Choose a reason for hiding this comment

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

this has been updated!

wallet/wallet.c Outdated
@@ -1867,6 +1874,8 @@ static bool wallet_stmt2htlc_in(struct channel *channel,
}
#endif

in->we_filled = !db_column_is_null(stmt, 13);
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

... then fix this.

return mvt_units[unit];
}

static u64 mvt_count = 0;
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

This surely belongs in a db somewhere?

Copy link
Collaborator Author

@niftynei niftynei Apr 9, 2020

Choose a reason for hiding this comment

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

updated in 1cbd8cf

wire/towire.c Outdated
if (tx->input_amounts[i])
towire_amount_sat(pptr, *tx->input_amounts[i]);
else
towire_amount_sat(pptr, AMOUNT_SAT(0));
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

You can have zero-valued inputs though. I think we should do all or nothing: either assert() that all input_amounts are set if tx->input_amounts is non-NULL, or don't include any if we don't have all of them.

@@ -81,6 +81,9 @@ static struct amount_msat our_msat;
/* Does option_static_remotekey apply to this commitment tx? */
bool option_static_remotekey;

/* We don't want to log any downstream coin moves if this is a known replay */
bool open_is_replay;
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

This should be "initial_tx_is_replay" perhaps? open is confusing here...

Copy link
Collaborator Author

@niftynei niftynei Apr 10, 2020

Choose a reason for hiding this comment

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

updated in 6c8d7ef to be pass through instead of a global (having a global didn't get me much of anything).

/* This could be written more concisely as
* count = ++ld->coin_moves_count;
* however I believe that's contra-code conventions */
ld->coin_moves_count++;
count = ld->coin_moves_count;
db_set_intvar(ld->wallet->db, "coin_moves_count",
count);
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

No, I have no problem with ++ prefix when necessary (it's just that C++ programmers have been trained to use it by default). I'd just do this all as a one-liner...

{
s64 count;
bool db_in_tx;
db_in_tx = db_in_transaction(ld->wallet->db);
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

I'm curious about what callchains do this outside a transaction? That has a weird smell to it...

Copy link
Collaborator Author

@niftynei niftynei Apr 14, 2020

Choose a reason for hiding this comment

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

Hm I can't seem to find them. I'm going to push the non-switched version up and see if Travis can find them for me :)

"coin_moves_count", -1);
db_commit_transaction(ld->wallet->db);
if (count == -1)
fatal("Something went wrong attmepting to fetch"
Copy link
Contributor

@rustyrussell rustyrussell Apr 9, 2020

Choose a reason for hiding this comment

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

typo, but maybe it's not? :)

@cdecker
Copy link
Member

@cdecker cdecker commented Apr 9, 2020

Didn't get too far with liquid-regtest and postgres wallet. This first issue is with the following configuration:

TEST_NETWORK=regtest
VALGRIND=1
PYTEST_PAR=10
TEST_DB_PROVIDER=postgres
DEVELOPER=1

It results in valgrind finding some uninitialized variables in the following tests:

  • test_fulfill_incoming_first
  • test_restart_many_payments
_______________ ERROR at teardown of test_fulfill_incoming_first _______________
[gw3] linux -- Python 3.6.8 /usr/bin/python3

request = <SubRequest 'teardown_checks' for <Function test_fulfill_incoming_first>>

    @pytest.fixture
    def teardown_checks(request):
        """A simple fixture to collect errors during teardown.

        We need to collect the errors and raise them as the very last step in the
        fixture tree, otherwise some fixtures may not be cleaned up
        correctly. Require this fixture in all other fixtures that need to either
        cleanup before reporting an error or want to add an error that is to be
        reported.

        """
        errors = TeardownErrors()
        yield errors

        if errors.has_errors():
            # Format a nice list of everything that went wrong and raise an exception
            request.node.has_errors = True
>           raise ValueError(str(errors))
E           ValueError:
E           Node errors:
E            - lightningd-2: reported valgrind errors
E            - lightningd-2: Node exited with return code 7
E           Global errors:

contrib/pyln-testing/pyln/testing/fixtures.py:171: ValueError
----------------------------- Captured stderr call -----------------------------
2020-04-09 11:39:54.034 EDT [1987] ERROR:  relation "version" does not exist at character 21
2020-04-09 11:39:54.034 EDT [1987] STATEMENT:  SELECT version FROM version LIMIT 1
2020-04-09 11:39:54.604 EDT [2090] ERROR:  relation "version" does not exist at character 21
2020-04-09 11:39:54.604 EDT [2090] STATEMENT:  SELECT version FROM version LIMIT 1
2020-04-09 11:39:54.623 EDT [2092] ERROR:  relation "version" does not exist at character 21
2020-04-09 11:39:54.623 EDT [2092] STATEMENT:  SELECT version FROM version LIMIT 1
--------------------------- Captured stdout teardown ---------------------------
------------------------------- Valgrind errors --------------------------------
Valgrind error file: valgrind-errors.6621
==6621== Use of uninitialised value of size 8
==6621==    at 0x608B86B: _itoa_word (_itoa.c:179)
==6621==    by 0x608EF0D: vfprintf (vfprintf.c:1642)
==6621==    by 0x60BB90F: vsnprintf (vsnprintf.c:114)
==6621==    by 0x1C2277: json_out_addv (json_out.c:239)
==6621==    by 0x16C4B7: json_add_member (json_stream.c:160)
==6621==    by 0x16B20E: json_add_u32 (json.c:603)
==6621==    by 0x136A06: json_mvt_id (notification.c:340)
==6621==    by 0x136AFE: coin_movement_notification_serialize (notification.c:357)
==6621==    by 0x136C32: notify_coin_mvt (notification.c:387)
==6621==    by 0x1226BA: notify_channel_mvt (coin_mvts.c:33)
==6621==    by 0x14F175: remove_htlc_out (peer_htlcs.c:1433)
==6621==    by 0x14F45D: update_out_htlc (peer_htlcs.c:1506)
==6621==
==6621== Conditional jump or move depends on uninitialised value(s)
==6621==    at 0x608B875: _itoa_word (_itoa.c:179)
==6621==    by 0x608EF0D: vfprintf (vfprintf.c:1642)
==6621==    by 0x60BB90F: vsnprintf (vsnprintf.c:114)
==6621==    by 0x1C2277: json_out_addv (json_out.c:239)
==6621==    by 0x16C4B7: json_add_member (json_stream.c:160)
==6621==    by 0x16B20E: json_add_u32 (json.c:603)
==6621==    by 0x136A06: json_mvt_id (notification.c:340)
==6621==    by 0x136AFE: coin_movement_notification_serialize (notification.c:357)
==6621==    by 0x136C32: notify_coin_mvt (notification.c:387)
==6621==    by 0x1226BA: notify_channel_mvt (coin_mvts.c:33)
==6621==    by 0x14F175: remove_htlc_out (peer_htlcs.c:1433)
==6621==    by 0x14F45D: update_out_htlc (peer_htlcs.c:1506)
==6621==
==6621== Conditional jump or move depends on uninitialised value(s)
==6621==    at 0x608F014: vfprintf (vfprintf.c:1642)
==6621==    by 0x60BB90F: vsnprintf (vsnprintf.c:114)
==6621==    by 0x1C2277: json_out_addv (json_out.c:239)
==6621==    by 0x16C4B7: json_add_member (json_stream.c:160)
==6621==    by 0x16B20E: json_add_u32 (json.c:603)
==6621==    by 0x136A06: json_mvt_id (notification.c:340)
==6621==    by 0x136AFE: coin_movement_notification_serialize (notification.c:357)
==6621==    by 0x136C32: notify_coin_mvt (notification.c:387)
==6621==    by 0x1226BA: notify_channel_mvt (coin_mvts.c:33)
==6621==    by 0x14F175: remove_htlc_out (peer_htlcs.c:1433)
==6621==    by 0x14F45D: update_out_htlc (peer_htlcs.c:1506)
==6621==    by 0x14F4A2: changed_htlc (peer_htlcs.c:1515)
==6621==
==6621== Conditional jump or move depends on uninitialised value(s)
==6621==    at 0x608FB4C: vfprintf (vfprintf.c:1642)
==6621==    by 0x60BB90F: vsnprintf (vsnprintf.c:114)
==6621==    by 0x1C2277: json_out_addv (json_out.c:239)
==6621==    by 0x16C4B7: json_add_member (json_stream.c:160)
==6621==    by 0x16B20E: json_add_u32 (json.c:603)
==6621==    by 0x136A06: json_mvt_id (notification.c:340)
==6621==    by 0x136AFE: coin_movement_notification_serialize (notification.c:357)
==6621==    by 0x136C32: notify_coin_mvt (notification.c:387)
==6621==    by 0x1226BA: notify_channel_mvt (coin_mvts.c:33)
==6621==    by 0x14F175: remove_htlc_out (peer_htlcs.c:1433)
==6621==    by 0x14F45D: update_out_htlc (peer_htlcs.c:1506)
==6621==    by 0x14F4A2: changed_htlc (peer_htlcs.c:1515)
==6621==
--------------------------------------------------------------------------------

In this configuration test_withdraw_misc also fails with an assertion error:

>       assert account_balance(l1, 'wallet') == 11974560000
E       assert 15987460000 == 11974560000
E         -15987460000
E         +11974560000

tests/test_misc.py:610: AssertionError

@cdecker
Copy link
Member

@cdecker cdecker commented Apr 9, 2020

The other configuration is the following:

TEST_NETWORK=liquid-regtest
VALGRIND=1
PYTEST_PAR=10
TEST_DB_PROVIDER=postgres
DEVELOPER=1

Surprisingly little errors despite the chain being different. It also produces the above valgrind errors, and it fails test_coin_movement_notices with the following:

       # Verify we recorded all the movements we expect
        check_coin_moves(l2, chanid_1, l1_l2_mvts)
>       check_coin_moves(l2, chanid_3, l2_l3_mvts)

tests/test_plugin.py:1276:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

n = <fixtures.LightningNode object at 0x7f414c622518>
account_id = '1ff53c604c6099eb2729d4c1fec57e2f5b61caa3b4d8d65d51db84eb06f49e82'
expected_moves = [{'credit': 1000000000, 'debit': 0, 'tag': 'deposit', 'type': 'chain_mvt'}, {'credit': 0, 'debit': 100000000, 'tag': '...'tag': 'chain_fees', 'type': 'chain_mvt'}, {'credit': 0, 'debit': 944570000, 'tag': 'withdrawal', 'type': 'chain_mvt'}]

    def check_coin_moves(n, account_id, expected_moves):
        moves = n.rpc.call('listcoinmoves_plugin')['coin_moves']
        node_id = n.info['id']
        acct_moves = [m for m in moves if m['account_id'] == account_id]
        assert len(acct_moves) == len(expected_moves)
        for mv, exp in list(zip(acct_moves, expected_moves)):
            assert mv['version'] == 1
            assert mv['node_id'] == node_id
            assert mv['type'] == exp['type']
            assert mv['credit'] == "{}msat".format(exp['credit'])
>           assert mv['debit'] == "{}msat".format(exp['debit'])
E           AssertionError

tests/utils.py:25: AssertionError

@niftynei niftynei force-pushed the nifty/money-movements branch 5 times, most recently from 9a19635 to 9a6f307 Compare Apr 16, 2020
@niftynei niftynei removed this from the 0.8.2 milestone Apr 16, 2020
@niftynei niftynei added this to the Next Release milestone Apr 16, 2020
@niftynei niftynei force-pushed the nifty/money-movements branch 6 times, most recently from 20dfc7b to 4b47525 Compare Apr 22, 2020
@niftynei
Copy link
Collaborator Author

@niftynei niftynei commented Apr 22, 2020

@cdecker tests seem to be a bit evened out now, if it's not too much trouble would you mind re-checking the liquid + alt-arch tests you ran?

@niftynei niftynei force-pushed the nifty/money-movements branch 3 times, most recently from a00f221 to 75e5a8e Compare May 2, 2020
niftynei added 2 commits May 8, 2020
Ignored outputs don't end up in the same 'resolved' pathway as other
tracked outputs do, so we mark them as moved when proposed/broadcast
instead of when resolved (since they'll never flow through as resolved)
@niftynei niftynei force-pushed the nifty/money-movements branch from 62e7599 to 0585717 Compare May 8, 2020
niftynei added 23 commits May 12, 2020
For cheats, we do a little bit of weird accounting. First we 'update'
our on-ledger balance to be the entirety of the channel's balance. Then,
as outputs get resolved, we record the fees and outputs as withdrawals
from this amount.

It's possible that they might successfully 'cheat', in which case we
record those as 'penalty' but debits (not credits).
We record htlcs when they're fulfilled as 'withdrawals' that are
onchain. This should make use of the payment_hash that we stashed.

Additionally, if an htlc spend comes through that's not ours, it's
probably them resolving our attempted cheat; we should allow it to
proceed without bombing, and just do our accounting as necessary. It'll
all come out in the wash.
Fixes the tx type annotation in a few places.
For every withdrawal transaction emitted, we record each of the outputs
plus the fees paid for this transaction.
If we don't save to disk, if the node restarts we'll lose them all
and the resulting balance check at the end will be incorrect.
Mostly we update existing tests to account for channel balances. In a
few places, new tests were needed as there wasn't an existing pathway
that tested the chain-fees for a few penalty cases
Check that we account for push_msat and wallet withdrawal/deposits
correctly
On node start we replay onchaind's transactions from the database/from
our loaded htlc table.  To keep things tidy, we shouldn't notify the
ledger about these, so we wrap pretty much everything in a flag that
tells us whether or not this is a replay.

There's a very small corner case where dust transactions will get missed
if the node crashes after the htlc has been added to the database but
before we've successfully notified onchaind about it.

Notably, most of the obtrusive updates to onchaind wrappings are due to
the fact that we record dust (ignored outputs) before we receive
confirmation of its confirmation.
Previously we were annotating every movement with the blockheight of
lightningd at notification time. Which is lossy in terms of info, and
won't be helpful for reorg reconciliation. Here we switch over to
logging chain moves iff they've been confirmed.

Next PR will fix this up for withdrawals, which are currently tagged
with a blockheight of zero, since we log on successful send.
This moves the notification for our coin spends from when it's
successfully submited to the mempool to when they're confirmed in a
block.

We also add an 'informational' notice tagged as `spend_track` which
can be used to track which transaction a wallet output was spent in.
Changelog-Added: Plugins: new notification type, `coin_movement`, which tracks all fund movements for a node
Should make it easier to track when coin moves in the plugin are
disjoint from what c-lightning says it's broadcast already.
It's possible for our peer to publish a commitment tx that has already
updated our balance for an htlc before we've completed removing it from
our commitment tx (aka before we've updated our balance). This used to
crash, now we just update our balance (and the channel balance logs!)
and keep going.

If they've removed anything from our balance, we'll end up counting it
as chain_fees below. Not ideal but fine... probably.
Wrap up more logic internally to the method call for htlcs. Also, don't
touch part id if we're not the 'origin'

Suggested-By: @rustyrussell
note that 'null' 'we_fulfilled's are going to be legacy from this
release forward.
Canonicalize the signature for the 'tag-type' of coin moves by unique
constructor/method calls.

Suggested-By: @rustyrussell
Updates the unit of account to be the chain_id, which is the BIP173 name
of the chain that the coins moved on.

Suggested-By: @rustyrussell
These guys take a while to run, so let's just skip them on the
valgrind/slow-machine combos :/
@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented May 12, 2020

Ack bc80300

@rustyrussell rustyrussell merged commit b881f73 into ElementsProject:master May 12, 2020
4 checks passed
fiatjaf pushed a commit to fiatjaf/mcldsp that referenced this issue May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants