Broadcast transactions only after their timelock is up #932

TheBlueMatt · 2021-05-27T19:08:53Z

Built on #928, this avoids broadcasting transactions too early resulting in us leaning on users to do rebroadcasting. Instead, we wait until timelocks are up to broadcast.

codecov · 2021-05-27T19:51:58Z

Codecov Report

Merging #932 (d51d606) into main (df829a8) will increase coverage by 0.08%.
The diff coverage is 95.79%.

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
+ Coverage   90.45%   90.54%   +0.08%     
==========================================
  Files          60       60              
  Lines       30595    30698     +103     
==========================================
+ Hits        27675    27795     +120     
+ Misses       2920     2903      -17

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`83.15% <ø> (ø)`
lightning/src/util/test_utils.rs	`82.54% <40.00%> (-0.60%)`	⬇️
lightning/src/chain/package.rs	`92.51% <86.20%> (+0.10%)`	⬆️
lightning/src/chain/channelmonitor.rs	`91.04% <94.11%> (+0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.18% <97.03%> (+0.37%)`	⬆️
lightning-background-processor/src/lib.rs	`94.73% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`94.14% <100.00%> (+0.46%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.78% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`94.81% <100.00%> (-0.09%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.62% <100.00%> (+<0.01%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df829a8...d51d606. Read the comment docs.

lightning/src/chain/channelmonitor.rs

lightning/src/chain/onchaintx.rs

ariard · 2021-05-27T22:48:16Z

lightning/src/chain/onchaintx.rs

+					log_debug!(logger, "  Outpoint {}", outpoint);
+				}
+				self.locktimed_packages.entry(req.package_timelock()).or_insert(Vec::new()).push(req);
+			} else if let Some((new_timer, new_feerate, tx)) = self.generate_claim_tx(height, &req, &*fee_estimator, &*logger) {


Can we broadcast delayed-transactions scenario with the following reorg-scenario ?

delayed-transactions are cached in locktimed_packages

height expiration is reached, we generate and broadcast delayed transactions

chain is reorg-out, delayed transactions are evicted from mempools

before syncing to new tip, we receive a preimage offchain and enter into update_claims_view

I don't think we can regenerate and broadcast a non-propagating delayed transaction as regeneration timer are height-based. In the future, if we switch regeneration timer to mempool feeration variations, we might broadcast such delayed transactions.

Otherwise, I can't think of other edgy reorg cases.

In that case don't we still generate the relevant commitment + timeout/claim transactions when the block comes back in when we sync to the new tip? Either way we should bump after a few blocks if we haven't confirmed by then.

Clearly, not a big deal. It's a just a edge-case about new feature added by this PR in case of a hypothetical future change. If we switch or add the option of mempool-based timing we'll have to split our hairs on few others things around OnchainTxHandler.

(Thought I still think mempool fluctuations-based timing might be more more interesting for fee savings than dumb rebroadcast after blocks X, if you're not near timelocks expiration)

ariard · 2021-05-27T22:51:09Z

lightning/src/chain/package.rs

@@ -576,13 +600,22 @@ impl PackageTemplate {
 		}
 		self.height_timer = cmp::min(self.height_timer, merge_from.height_timer);
 	}
-	pub(crate) fn package_amount(&self) -> u64 {
+	/// Gets the amount of all outptus being spent by this package, only valid for malleable


nit: *outputs an "only valid for now (legacy channel types)"

Hmm, not sure what you're suggesting? I'll let you update the docs for anchor in the anchor PRs.

lightning/src/chain/package.rs

TheBlueMatt · 2021-05-28T00:13:26Z

Rebased after merge of #928 and addressed comments.

ariard

Code Review ACK 2f755b1

I went through all the fixes commit, globally satisfied with new comments. I had a look on test changes they look good overall.

jkczyz

Need to take a closer look at the tests in the later commits.

jkczyz · 2021-05-28T00:21:45Z

lightning/src/chain/package.rs

+			PackageSolvingData::HolderHTLCOutput(..) => { debug_assert!(false); 0 },
+			PackageSolvingData::HolderFundingOutput(..) => { debug_assert!(false); 0 },


Would unreachable! be appropriate?

Actually, yea. I was avoiding doing it to keep the panic in debug, but I think you're right, if we ever actually get there, its probably indication that we're gonna generate an invalid spend transaction and/or burn funds to fee, which is not ok.

lightning/src/chain/channelmonitor.rs

jkczyz · 2021-05-28T00:34:57Z

lightning/src/ln/functional_tests.rs

@@ -1499,16 +1499,17 @@ fn test_duplicate_htlc_different_direction_onchain() {

 	// Check we only broadcast 1 timeout tx
 	let claim_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
-	let htlc_pair = if claim_txn[0].output[0].value == 800_000 / 1000 { (claim_txn[0].clone(), claim_txn[1].clone()) } else { (claim_txn[1].clone(), claim_txn[0].clone()) };


Was this unnecessary?

Entirely, the 800_000 wasn't even right anymore - now that we subtract fees no output was ever exactly the 800k msat

lightning/src/ln/functional_tests.rs

lightning/src/chain/package.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/chanmon_update_fail_tests.rs

lightning/src/util/test_utils.rs

jkczyz · 2021-05-28T07:27:42Z

lightning/src/ln/functional_tests.rs

@@ -8689,6 +8700,9 @@ fn test_concurrent_monitor_claim() {
 		watchtower
 	};
 	let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+	// Make the tx_broadcaster aware of enough blocks that it doesn't think we're violating
+	// transaction lock time requirements here.
+	chanmon_cfgs[0].tx_broadcaster.blocks.lock().unwrap().resize((CHAN_CONFIRM_DEPTH + 1 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS) as usize, (header, 0));


Is there a need to use a Vec? Would storing the last block header and height suffice?

The Vec here is the same one that the block_disconnected stuff uses, so, yes.

Ah, I didn't realized they were shared. Makes sense.

jkczyz · 2021-05-28T07:42:34Z

lightning/src/chain/channelmonitor.rs

+					// Note that we add + 1 as transactions are broadcastable when they can be
+					// confirmed in the next block, not the current height.


I'm not sure if I can parse this. If the current height is X then any transaction we broadcast won't confirm until X + 1 at the earliest by definition.

Right, its just noting that we can always broadcast when the locktime is X + 1. I can drop if you think its obvious but Antoine suggested it.

Fun fact of the day - Bitcoin Core had a bug for many years where it actually wouldn't accept-for-relay transactions until they were confirm-able in the current block, not X+1.

If the current block is X and the earliest the transaction can confirm is X + 1 then broadcasting at X should be fine, right? Because the next block is X + 1. I guess I'm partly wondering if the + 1 is needed. Maybe I'm misunderstanding cltv_expiry.

Right, if the current height is X, then the next block which will be mined is X + 1. Thus, we can always broadcast something with a CLTV expiry of X + 1 as it can go into the next block (but could not appear in the current best block).

Ah, that makes sense. I think dropping "not the current height" would be beneficial as including it can lead to confusion.

jkczyz

Just a couple nits, but otherwise this is still outstanding: ff8ca02#r641682315

lightning/src/ln/reorg_tests.rs

lightning/src/ln/functional_tests.rs

TheBlueMatt · 2021-05-28T21:35:38Z

Hmm, your link isn't working for me now, but I did try to respond at ff8ca02#r641820784

jkczyz · 2021-05-28T21:46:36Z

lightning/src/chain/channelmonitor.rs

+					// Note that we add + 1 as transactions are broadcastable when they can be
+					// confirmed in the next block, not the current height.


Ah, that makes sense. I think dropping "not the current height" would be beneficial as including it can lead to confusion.

ariard

Code Review ACK a59b879

Changes since last time are addressing Jeff's comments

This somewhat cleans up the public API of PackageSolvingData to make it harder to get an invalid amount and use it, adding further debug assertion to check it at test-time.

This allows us to interrogate a PackageTemplate for the CLTV timelock of the resulting transaction.

This cleans up some of the transaction format verification and docs to make it easier when we delay CLTV-locked transactions to update the tests.

This simplifies logic somewhat and avoids duplicating the storage of the current height in OnchainTxHandler.

This stores transaction templates temporarily until their locktime is reached, avoiding broadcasting (or RBF bumping) transactions prior to their locktime. For those broadcasting transactions (potentially indirectly) via Bitcoin Core RPC, this ensures no automated rebroadcast of transactions on the client side is required to get transactions confirmed.

TheBlueMatt · 2021-05-28T23:59:14Z

Tweaked the comment as suggested at #932 (comment), fixed one trivial error from new fixes, and squashed. Will merge after CI:

$ git diff-tree -U1 a59b8798 d51d606ba
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 3df0111f5..26187ce34 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -1914,3 +1914,3 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 					// Note that we add + 1 as transactions are broadcastable when they can be
-					// confirmed in the next block, not the current height.
+					// confirmed in the next block.
 					continue;
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 30b9d0145..eb4f6054b 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -5843,3 +5843,3 @@ fn test_key_derivation_params() {
 	// Ensure all nodes are at the same height
-	let node_max_height = nodes.iter().map(|node| node.blocks.borrow().len()).max().unwrap() as u32;
+	let node_max_height = nodes.iter().map(|node| node.blocks.lock().unwrap().len()).max().unwrap() as u32;
 	connect_blocks(&nodes[0], node_max_height - nodes[0].best_block_info().1);
$

TheBlueMatt changed the title ~~2021 05 broadcast locktime delay~~ Broadcast transactions only after their timelock is up May 27, 2021

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch 2 times, most recently from 7fa5a2c to d267f7b Compare May 27, 2021 19:26

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch from d267f7b to 6202aaa Compare May 27, 2021 20:30

ariard reviewed May 27, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch from 6202aaa to 2f755b1 Compare May 28, 2021 00:13

ariard reviewed May 28, 2021

View reviewed changes

jkczyz reviewed May 28, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch 4 times, most recently from 4c6b0ac to ab7f150 Compare May 28, 2021 16:43

jkczyz reviewed May 28, 2021

View reviewed changes

lightning/src/ln/reorg_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch from ab7f150 to a59b879 Compare May 28, 2021 21:36

jkczyz approved these changes May 28, 2021

View reviewed changes

ariard reviewed May 28, 2021

View reviewed changes

TheBlueMatt added 7 commits May 28, 2021 23:56

Add assertions to ensure we don't use an invalid package_amount

f492a19

This somewhat cleans up the public API of PackageSolvingData to make it harder to get an invalid amount and use it, adding further debug assertion to check it at test-time.

Simplify HolderHTLCOutput constructor and track CLTV expiry

56491d9

This allows us to interrogate a PackageTemplate for the CLTV timelock of the resulting transaction.

Simplify tx checks in functional tests to make later commits simpler

7d6938a

This cleans up some of the transaction format verification and docs to make it easier when we delay CLTV-locked transactions to update the tests.

Expose the timelock of transaction packages

9bd87e9

Always pass height to OnchainTxHandler::update_claims_view

79ffc5e

This simplifies logic somewhat and avoids duplicating the storage of the current height in OnchainTxHandler.

Track the blocks a node has connected in the TestBroadcaster

90e984e

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch from a59b879 to a87a02f Compare May 28, 2021 23:57

TheBlueMatt added 3 commits May 28, 2021 23:58

Skip transactions which are locktime'd when broadcasting in test

00bbc51

Dont broadcast HTLC-Timeouts when closing a channel until locktime

f61676e

panic if locktime is violated when broadcasting in tests

d51d606

TheBlueMatt force-pushed the 2021-05-broadcast-locktime-delay branch from a87a02f to d51d606 Compare May 28, 2021 23:58

TheBlueMatt merged commit 8cc9410 into lightningdevkit:main May 29, 2021

		PackageSolvingData::HolderHTLCOutput(..) => { debug_assert!(false); 0 },
		PackageSolvingData::HolderFundingOutput(..) => { debug_assert!(false); 0 },

		// Note that we add + 1 as transactions are broadcastable when they can be
		// confirmed in the next block, not the current height.

Broadcast transactions only after their timelock is up #932

Broadcast transactions only after their timelock is up #932

Uh oh!

Conversation

TheBlueMatt commented May 27, 2021

Uh oh!

codecov bot commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented May 28, 2021

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented May 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 28, 2021

Uh oh!

Uh oh!

codecov bot commented May 27, 2021 •

edited

Loading