Onchaind replay different txs #1171

rustyrussell · 2018-03-05T09:53:11Z

This fixes #1114 with some other incidental cleanups along the way.

ZmnSCPxj

Some comments...

ZmnSCPxj · 2018-03-05T23:52:25Z

wallet/wallet.c

@@ -701,6 +705,9 @@ u32 wallet_first_blocknum(struct wallet *w, u32 first_possible)
 		else
 			first_utxo = UINT32_MAX;
 	}
+#else
+	first_utxo = db_get_intvar(w->db, "last_processed_block", UINT32_MAX);


Could this not potentially lose onchain funds if onchain funds were added long before any channels were created? Maybe safer to use first_possible here?

No, first_possible is the first theoretical possibility for the blockchain.

first_utxo (could be called "first_unseen_utxo") is simply the last processed_block (plus one, but that would make this statement more complex).

Okay, I will have to trust you on that, as the tx scanning is beyond my capability for now.

ZmnSCPxj · 2018-03-06T00:29:23Z

onchaind/onchain.c

+	 * comparison (ignoring signatures) works pretty well.
+	 *
+	 * FIXME: Better would be to compare outputs, but they weren't
+	 * saved to db correctly until now. (COMPAT_V052)


The comment suggests we should have a #ifdef COMPAT_V052 or similar in the vicinity?

It's in the FIXME as grep-fodder for COMPAT_V052. I'm not sure we'll actually do this, but at least we'll have the chance to do so when we decided to delete all those....

ZmnSCPxj · 2018-03-06T00:38:08Z

bitcoin/script.c

+{
+	u8 *script = tal_arr(ctx, u8, 0);
+
+	add_op(&script, OP_RETURN);


Does not standard OP_RETURN require an additional push after the OP_RETURN? Otherwise the tx may not get broadcast across the Bitcoin network to reach a miner?

bitcoind source doesn't seem to, but I added a test to be sure!

Where is test? I think regtest and testnet skip over IsStandardTx check?

The bitcoind source doesn't seem to indeed; it allows a plain OP_RETURN: https://github.com/bitcoin/bitcoin/blob/d889c036cd6f683116e6a27e404be2809d1deb76/src/script/standard.cpp#L88-L96

All the guides I found on the Internet indicate a single data push after OP_RETURN but the actual code seems to allow 0 to any number of data pushes, as long as the total length is below the limit.

Sorry, found some other issues, took me much longer to push than I hoped!

ZmnSCPxj · 2018-03-06T05:16:14Z

bitcoin/script.c

+{
+	u8 *script = tal_arr(ctx, u8, 0);
+
+	add_op(&script, OP_RETURN);


Where is test? I think regtest and testnet skip over IsStandardTx check?

The bitcoind source doesn't seem to indeed; it allows a plain OP_RETURN: https://github.com/bitcoin/bitcoin/blob/d889c036cd6f683116e6a27e404be2809d1deb76/src/script/standard.cpp#L88-L96

All the guides I found on the Internet indicate a single data push after OP_RETURN but the actual code seems to allow 0 to any number of data pushes, as long as the total length is below the limit.

rustyrussell · 2018-03-06T05:50:41Z

OK, fixed some CI-found errors, and added 4 new commits....

ZmnSCPxj

Looks OK to me, though CI seems to disagree...

rustyrussell · 2018-03-06T11:07:23Z

More test errors: I had to fix the wallet COMPAT patch, as we weren't setting the last_processed_block variable on all cases.

cdecker · 2018-03-06T21:54:58Z

onchaind/onchain.c

-		tal_resize(&tx->output, 0);
-	else
+	/* Result is trivial?  Spent to OP_RETURN to avoid leaving dust. */
+	if (tx->output[0].amount < dust_limit_satoshis + fee) {


This solves the to_us respend issue when the value is below the fee I guess. Wouldn't it be better to not create that to_us output at all? But that'd require signaling to the peer that we're ok omitting our output in the commit, which is a spec change.

This is onchain-only: if fees rise and we can't spent, it's best to remove the UTXO.

The alternative would be to drop the feerate and cross our fingers (or consider it closed?).

For some outputs we're not in a hurry, it's just tidying up: they could potentially use a lower fee rate.

rustyrussell · 2018-03-07T00:09:25Z

Rebased to fix conflicts.

cdecker · 2018-03-07T03:52:03Z

Looks like we have just one include out of order, otherwise we're good to go :-)

In general, it is true that accessors should take const and discard it, but chainparams is *always* const. Signed-off-by: Rusty Russell <[email protected]>

This lets us clearly mark transition features, in a way that they can be removed after 0.6 is released. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

Our testing also reveals a bug: we start lightningd and shut it down before fully processing the blockchain, so we don't set last_processed_block. Fix that by setting it immediately once we have a block: worst case it goes backwards a little. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

Use const, add TAKES to declaration. Signed-off-by: Rusty Russell <[email protected]>

And fix up resulting breakage. Signed-off-by: Rusty Russell <[email protected]>

This simplifies things, and means it's always in the database. Our previous approach to creating it on the fly had holes when it was created for onchaind, causing us to use another every time we restarted. Signed-off-by: Rusty Russell <[email protected]>

It would be better to give them unique values, but we don't fully support db migrate anyway and this is simple (though they will end up using the same key for multiple channel closes if created before this commit). Note that even if bip32_max_index is currently unset, it defaults to 0 so it will be found. Signed-off-by: Rusty Russell <[email protected]>

The root cause of ElementsProject#1114 was that the restarted onchaind created a different proposal to the one which had previously been mined: 2018-03-01T09:41:08.884Z lightningd(1): lightning_onchaind-020d3d5995a973c878e3f6e5f59da54078304c537f981d7dcef73367ecbea0e90e chan #1: STATUS_FAIL_INTERNAL_ERROR: THEIR_UNILATERAL/OUR_HTLC spent with weird witness 3 After the previous patches which fixed the output address difference, we could identify proposals by their outputs, but during the transition (onchaind started with old buggy version, restarted now) that wouldn't be right, so we match the inputs, discarding signatures which will be different. This works for all current cases. Closes: ElementsProject#1114 Signed-off-by: Rusty Russell <[email protected]>

…fees. This was revealed in ElementsProject#1114; onchaind isn't actually completely idempotent due to fee changes (and the now-fixed change in keys used). This triggers the bug by restarting with different fees, resulting in onchaind not recognizing its own proposal: 2018-03-05T09:38:15.550Z lightningd(23076): lightning_onchaind-022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59 chan #1: STATUS_FAIL_INTERNAL_ERROR: THEIR_UNILATERAL/OUR_HTLC spent with weird witness 3 Signed-off-by: Rusty Russell <[email protected]>

They're illegal. Instead do OP_RETURN so we don't pollute the UTXO. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

DONATING_TO_MINERS is pretty clear, I think. Signed-off-by: Rusty Russell <[email protected]>

With the following patch applied, we could clearly see onchaind try to broadcast the timeout tx one block too early: sendrawtx exit 26, gave error code: -26?error message:?non-final (code 64)? This is because of an out-by-one error in calculating the relative depth required, since the out->tx_blockheight is already 1 before the current block. Signed-off-by: Rusty Russell <[email protected]>

We say "in N blocks" but we actually mean "N blocks after this tx" which is actually N-1 or less. Change wording and tighten tests which misunderstood this. Also, the 'assert not l1.daemon.is_in_log('onchaind complete, forgetting peer')' are unlikely to work until the daemon has actually seen the block, so add sync_blockheight before all of those. These changes reveal some sloppy testing, which we fix. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Christian Decker <[email protected]>

cdecker · 2018-03-07T15:43:10Z

Rebased, fixed a minor conflict in the mocks of run-wallet and fixed header ordering.

@ZmnSCPxj if you could ack that there hasn't been a major change then I can merge this :-)

ZmnSCPxj · 2018-03-07T16:03:30Z

ACK 419d95a

rustyrussell requested review from jb55, cdecker and ZmnSCPxj March 5, 2018 09:53

ZmnSCPxj reviewed Mar 6, 2018

View reviewed changes

ZmnSCPxj approved these changes Mar 6, 2018

View reviewed changes

rustyrussell force-pushed the onchaind-replay-different-txs branch from 6dada6c to cce272f Compare March 6, 2018 05:48

ZmnSCPxj approved these changes Mar 6, 2018

View reviewed changes

rustyrussell force-pushed the onchaind-replay-different-txs branch from cce272f to aa2074d Compare March 6, 2018 11:06

rustyrussell force-pushed the onchaind-replay-different-txs branch from aa2074d to 9f7363e Compare March 6, 2018 20:29

cdecker reviewed Mar 6, 2018

View reviewed changes

rustyrussell force-pushed the onchaind-replay-different-txs branch from 9f7363e to 7b35410 Compare March 7, 2018 00:07

rustyrussell added 15 commits March 7, 2018 16:31

lightningd: don't discard const in get_chainparams().

5bb1c2f

In general, it is true that accessors should take const and discard it, but chainparams is *always* const. Signed-off-by: Rusty Russell <[email protected]>

Makefile: add COMPAT_V052 define.

419f902

This lets us clearly mark transition features, in a way that they can be removed after 0.6 is released. Signed-off-by: Rusty Russell <[email protected]>

Travis: do test build without compat defined.

d86a881

Signed-off-by: Rusty Russell <[email protected]>

pay: wrap missing route_channels handling in COMPAT_V052.

2a3d439

Signed-off-by: Rusty Russell <[email protected]>

txfilter: clean up prototypes.

5044f90

Use const, add TAKES to declaration. Signed-off-by: Rusty Russell <[email protected]>

wallet: add check-source to makefile.

958dcd1

And fix up resulting breakage. Signed-off-by: Rusty Russell <[email protected]>

onchaind: don't create zero-output txs if fees overwhelm us.

720b9b3

They're illegal. Instead do OP_RETURN so we don't pollute the UTXO. Signed-off-by: Rusty Russell <[email protected]>

test_lightningd.py: test onchaind when all turn into fees.

a472502

Signed-off-by: Rusty Russell <[email protected]>

onchaind: add a new state for where we're not producing an output.

e5f59fb

DONATING_TO_MINERS is pretty clear, I think. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell and others added 2 commits March 7, 2018 16:33

wallet: Fix header ordering

419d95a

Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the onchaind-replay-different-txs branch from 7b35410 to 419d95a Compare March 7, 2018 15:41

ZmnSCPxj approved these changes Mar 7, 2018

View reviewed changes

cdecker merged commit d363a68 into ElementsProject:master Mar 7, 2018

cdecker mentioned this pull request Mar 13, 2018

Crash while syncing #1212

Closed

ZmnSCPxj mentioned this pull request May 28, 2019

AWAITING_UNILATERAL bug #2684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Onchaind replay different txs #1171

Onchaind replay different txs #1171

rustyrussell commented Mar 5, 2018

ZmnSCPxj left a comment

ZmnSCPxj Mar 5, 2018

rustyrussell Mar 6, 2018

ZmnSCPxj Mar 6, 2018

ZmnSCPxj Mar 6, 2018

rustyrussell Mar 6, 2018

ZmnSCPxj Mar 6, 2018

rustyrussell Mar 6, 2018

ZmnSCPxj Mar 6, 2018

rustyrussell Mar 6, 2018

ZmnSCPxj Mar 6, 2018

rustyrussell commented Mar 6, 2018

ZmnSCPxj left a comment

rustyrussell commented Mar 6, 2018

cdecker Mar 6, 2018

rustyrussell Mar 6, 2018

rustyrussell commented Mar 7, 2018

cdecker commented Mar 7, 2018

cdecker commented Mar 7, 2018

ZmnSCPxj commented Mar 7, 2018

Onchaind replay different txs #1171

Onchaind replay different txs #1171

Conversation

rustyrussell commented Mar 5, 2018

ZmnSCPxj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Mar 6, 2018

ZmnSCPxj left a comment

Choose a reason for hiding this comment

rustyrussell commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Mar 7, 2018

cdecker commented Mar 7, 2018

cdecker commented Mar 7, 2018

ZmnSCPxj commented Mar 7, 2018