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

json-rpc: Annotate transactions of interest and add the `listtransactions` RPC method #2696

Merged
merged 16 commits into from Jun 8, 2019

Conversation

@cdecker
Copy link
Member

commented May 31, 2019

This PR extends the capabilities of our internal transaction tracking by annotating transactions of interest with a transaction type and a channel_id. This allows us to group transactions by the channel they affected, trace the on-chain footprint of a channel (improving our debuggability) and showing on-chain transaction history in wallet UIs.

  • The channel_id in the transactions table is not a foreign key into the channels table since we delete entries from that table, which would reset that grouping key. I found it better to keep them set, resulting in them being more of a grouping key rather than a real reference to a channel.
  • Transactions are inserted and then annotated with their type aposteriori since we add transactions in the main daemon based on tx_watches or txo_watches but only subdaemons (onchaind in particular) know their type. It's further complicated by the fact that some watch callbacks actually require the transaction to be in the transactions table before being called. The two alternatives I have tried were:
    • Have the tx_watch and txo_watch entries be marked with type and channel_id so that we can insert them with the correct values right away. This failed since for onchaind we automatically track descendants of a tracked transaction, and we'd have to ask onchaind for its type anyway.
    • Have the callbacks for the watches insert the transaction and annotate them at the same time. This also fails for onchaind, but in some places where we call wallet_transaction_add directly followed by wallet_transaction_annotate this worked out fine. I found the overhead small enough to warrant not combining the two into a single call.
  • I had a teardown test during testing that'd check that no TX_UNKNOWN transactions are left in the DB in the end, but decided against including it since it produced some false positives for tests where the node gets killed (mainly cheat attempts) and it was triggering on remote sweep transactions as well, which we just unwatch right away again. If desired I can include that teardown test again, but it'll need instrumentation to avoid false positives.
  • I need to rework the way we confirm transactions a bit, since recording a tx_watch for each of the transactions is a bit messy, and I'd like to merge the transactions and channeltxs tables in a future PR.

Open questions:

  • There are no migrations attempting to infer the type of existing transactions in the table yet. I'm not sure we want to spend extra cycles on that since we are now tracking more transactions that will not be present anyway (withdrawals were not tracked at all), and it is bound to reverse-engineer in an automated fashion what type a transaction had. What do the others think? Is keeping them TX_UNKNOWN good enough or how much work should we invest here?
  • Like mentioned above I'd like to revamp the way we track confirmations. That'd be for another PR, or would you prefer we include it here?

@cdecker cdecker added the json-rpc label May 31, 2019

@cdecker cdecker added this to the 0.7.1 milestone May 31, 2019

@cdecker cdecker requested a review from rustyrussell May 31, 2019

@cdecker cdecker self-assigned this May 31, 2019

@cdecker cdecker requested a review from niftynei May 31, 2019

@cdecker cdecker added the wallet label May 31, 2019

@@ -270,6 +289,15 @@ struct channeltx {
u32 depth;
};

struct wallet_transaction {

This comment has been minimized.

Copy link
@darosior

darosior Jun 1, 2019

Collaborator

Isn't having two "wallet transaction" structures a little bit ambiguous ?

struct wallet_tx {

}

static const struct json_command listtransactions_command = {
"listtransactions", json_listtransactions,

This comment has been minimized.

Copy link
@darosior

darosior Jun 1, 2019

Collaborator

Maybe add a command category if #2658 gets merged ?

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

This PR is probably sufficient, though I have yet to review. I'm itching to get 0.7.1 out.

My main concern is that Bring-Your-Own-Fees means we will combine/chain txs. This means someone needs to be able to re-sign or re-create txs when one gets replaced, eg. unilateral close race. Even more ambitious, we might want to RBF our own txs if we get another thing we want to spend.

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2019

My main concern is that Bring-Your-Own-Fees means we will combine/chain txs. This means someone needs to be able to re-sign or re-create txs when one gets replaced, eg. unilateral close race. Even more ambitious, we might want to RBF our own txs if we get another thing we want to spend.

What we do here is basically just track transactions we sent, and transactions concerning our channels that are sent by the counterparty. Tracking in this case means adding a type, associating them with a channel and tracking confirmation status. If we have things like splicing or combine sweeps, we'll just add those types to the transactions, which is already supported. For RBF the confirmation status becomes more interesting since we want to filter by confirmation status there.

There is one limiting factor in the current PR, namely that we have a many-to-one relationship from transaction to channel, so for splicing with changing channel_id or sweep across multiple channels (unlikely due to CLTV incompatibilies) we lose expressiveness. Is that something we should address now by creating a transaction_channel associative table that make this a many-to-many relationship just for safety?

@rustyrussell
Copy link
Contributor

left a comment

OK, reading this it's clear to me that annotating transactions is not actually what we want. We want to annotate outputs, and imply transactions from that.

At the moment, it's hard to care because all our transactions are single-purpose. But even so it's not clear: for a closing tx which has two outputs, we still need to annotate which one we own. We should probably head directly to per-output, which (while it might be a degree of churn) could well simplify our handling now and in future.

That doesn't make this wrong, but it does suggest we make sure our APIs are going to work; listtransactions might still be a better name, but it's really listoutputs?

TX_WALLET_WITHDRAWAL = 4,
TX_CHANNEL_FUNDING = 8,
TX_CHANNEL_CLOSE = 16,
TX_CHANNEL_UNILATERAL = 32,

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

We don't distinguish their unilateral from ours?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

Ah, I see, THEIRS is a bit, the rest are distinct. Maybe we should just make those three cases distinct?

TX_CHANNEL_OUR_UNILATERAL / TX_CHANNEL_THEIR_UNILATERAL, TX_CHANNEL_OUR_HTLC_SUCCESS ... TX_CHANNEL_OUR_HTLC_TIMEOUT...?

That gets us closer to the onchain types, too.

This comment has been minimized.

Copy link
@cdecker

cdecker Jun 3, 2019

Author Member

Not really, a close transaction is also a deposit to us if we had funds in them, but annotate at different places, creating all possible combinations is likely not what we want.

But I agree that TX_THEIRS is a bit strange since it indicates both who originated the transaction as well as filtering out transactions we don't have a stake in (e.g., their sweeps).

TX_CHANNEL_CHEAT = 1024,
};
/* Any combination of the above wallet_tx_types */
typedef unsigned short txtypes;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

In C (unlike C++!) enums can have any integer value for exactly this reason. So this typedef is unnecessary; enum wallet_tx_type can be used directly.

This comment has been minimized.

Copy link
@cdecker

cdecker Jun 3, 2019

Author Member

Hm, thought I'd make it explicit, not that people start using switch statements over the enum and then wonder why they don't catch all the variants. Will remove.

@@ -48,6 +48,7 @@ onchain_init_reply,5101
# onchaind->master: Send out a tx.
onchain_broadcast_tx,5003
onchain_broadcast_tx,,tx,struct bitcoin_tx
onchain_broadcast_tx,,type,u16

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

Use enum here, and define towire_wallet_txtype and fromwire_ somewhere?

This means moving wallet_txtypes out to common; maybe just call it 'txtype' then?

return TX_CHANNEL_UNILATERAL;
case THEIR_HTLC_FULFILL_TO_US:
case OUR_HTLC_SUCCESS_TX:
return TX_CHANNEL_HTLC_SUCCESS;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

I had to re-read the code to see the difference between these two tx types! THEIR_HTLC_FULFILL_TO_US is their unilateral close (simple case, no timeouts) and OUR_HTLC_SUCCESS_TX is our own unilateral close (using the sig they gave us, with timeout).

Does it make sense to distinguish these at the wallet level? Maybe, because the second has a timeout. Similarly OUR_HTLC_TIMEOUT_TO_US and OUR_HTLC_TIMEOUT_TX.

@@ -23,6 +23,7 @@
#include <onchaind/onchain_types.h>
#include <stdio.h>
#include <unistd.h>
#include <wallet/wallet.h>

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

Definitely belongs in common...

@@ -1093,6 +1093,7 @@ static void output_spent(struct tracked_output ***outs,
status_trace("Notified about tx %s output %u spend, but we don't care",
type_to_string(tmpctx, struct bitcoin_txid, &txid),
tx->wtx->inputs[input_num].index);

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

ENODIFF?

{TX_CHANNEL_HTLC_SUCCESS, "channel_htlc_success"},
{TX_CHANNEL_HTLC_TIMEOUT, "channel_htlc_timeout"},
{TX_CHANNEL_PENALTY, "channel_penalty"},
{TX_CHANNEL_CHEAT, "channel_unilateral_cheat"},

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 3, 2019

Contributor

Another reason to make the enums distinct, put them in a common header file, and autogenerate this?

This comment has been minimized.

Copy link
@cdecker

cdecker Jun 3, 2019

Author Member

I coded them manually as I wanted them to be nicely readable since they end up in the JSON output. These basically drop the TX_ prefix, and have a LESS SHOUTY lowercase 😉

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

OK, reading this it's clear to me that annotating transactions is not actually what we want. We want to annotate outputs, and imply transactions from that.

Interesting idea, I like it. The reason I annotated transactions and not outputs is because the original issue I tried to address was having a way to list our on-chain transactions, and centralizing all of our full transaction tracking in the existing transactions table sounded like a good idea.

Tracking outputs requires us to have a separate table transaction_outputs with the following format:

CREATE TABLE transaction_outputs (
    transaction_id BLOB REFERENCES transactions(id) ON DELETE CASCADE,
    outnum INTEGER,
    type INTEGER,
    channel_id  INTEGER
);

In addition we also need to annotate inputs in some cases (such as withdrawals and channel closes), requiring yet another table. Then representing that in the JSON result requires additional logic (JOINs duplicate the shared entries so we iterate over entries until the transactions.id changes, and a double join with inputs and outputs just gets really annoying).

I'm happy to give a shot though, if the extra effort is warranted :-)

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

OK, after offline discussion with @cdecker we're going to defer a per-input & output tracking for post 0.7.1: for now this is an obvious win. @cdecker please apply this after minor fixups suggested above.

Ack 2ddae30

@cdecker cdecker force-pushed the cdecker:listtransactions branch 2 times, most recently from e6006bc to 45dab27 Jun 7, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Ack 4b262e8

cdecker added some commits May 23, 2019

db: Add channel_id and type to the transactions table
Mainly used to differentiate channel-related transactions from on-chain wallet
transactions. Will be used to filter `listtransaction` results and bundle
transactions that belong to the same channel.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wallet: Add function to annotate transactions aposteriori
Since we add the transactions while processing the blockchain, and before we
have enough context to annotate them correctly, i.e., in the txwatches, we add
them first and then annotate them aposteriori.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
chaintopology: Annotate transactions as deposits if we owned outputs
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wallet: Store and annotate withdrawals in the DB
We weren't storing them so far, which is sub-optimal :-)

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wallet: Annotate funding transaction in the database
Signed-off-by: Christian Decker <decker.christian@gmail.com>
onchaind: Store and annotate close transaction when we drop to chain
Signed-off-by: Christian Decker <decker.christian@gmail.com>
onchaind: Store and annotate transactions we broadcast ourselves
Signed-off-by: Christian Decker <decker.christian@gmail.com>
onchaind: Allow onchaind to annotate transactions we watch
This is important for things we automatically watched because it spends a
watch txo, but only onchaind knows the details about what the TX really is and
how it should be handled.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
onchaind: Have onchaind annotate htlc-{succes,failure} and penalties
Signed-off-by: Christian Decker <decker.christian@gmail.com>
onchaind: Annotate their sweep transactions
Signed-off-by: Christian Decker <decker.christian@gmail.com>
openingd: Annotate our own funding transaction
Signed-off-by: Christian Decker <decker.christian@gmail.com>
channel: Along with the last_tx also remember its type
This takes the guesswork out of `drop_to_chain` and allows us to annotate the
last_tx consistently.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
onchaind: Have onchaind annotate unilateral, cheat and mutual closes
onchaind knows best, no need to guess outside.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wallet: Add function to retrieve transactions from the wallet
Signed-off-by: Christian Decker <decker.christian@gmail.com>
json-rpc: Add `listtransactions` RPC method
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wallet: Rip out the txtypes type in favor of enum wallet_tx_type
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <decker.christian@gmail.com>

@rustyrussell rustyrussell force-pushed the cdecker:listtransactions branch from 4b262e8 to 6f25317 Jun 8, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

autosquashed...

Ack 6f25317

@rustyrussell rustyrussell merged commit b6b548a into ElementsProject:master Jun 8, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.