(Fix and) Test that txn pay at least a minimum relay fee in functional tests #716

TheBlueMatt · 2020-09-18T22:35:36Z

This fixes some issues in the close-tx logic as well tests aggressively that we always include a proper fee in transactions.

codecov · 2020-09-18T23:24:36Z

Codecov Report

Merging #716 into main will increase coverage by 0.07%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #716      +/-   ##
==========================================
+ Coverage   91.92%   92.00%   +0.07%     
==========================================
  Files          37       36       -1     
  Lines       20098    20216     +118     
==========================================
+ Hits        18475    18599     +124     
+ Misses       1623     1617       -6

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.09% <ø> (+0.23%)`	⬆️
lightning/src/ln/channel.rs	`86.32% <82.14%> (+0.08%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`94.02% <100.00%> (-0.42%)`	⬇️
lightning/src/ln/reorg_tests.rs	`98.90% <0.00%> (-0.08%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.78% <0.00%> (-0.05%)`	⬇️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/channelmanager.rs	`85.64% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs
... and 8 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 937d1d8...9d206a3. Read the comment docs.

naumenkogs · 2020-09-21T10:06:01Z

lightning/src/ln/channel.rs

@@ -3031,9 +3054,14 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			},
 		};

+		let closing_tx_max_weight = self.get_closing_transaction_weight(
+			if let Some(oup) = closing_tx.output.get(0) { Some(&oup.script_pubkey) } else { None },


Why would this ever not have 0/1 outputs?

Close outputs can be dust, causing that channel party's balance to be burned to fees at close.

naumenkogs · 2020-09-21T10:06:53Z

lightning/src/ln/channel.rs

+		}
+		if (msg.fee_satoshis as u64) < min_feerate as u64 * closing_tx_max_weight / 1000 {
+			if let Some((last_feerate, _, _)) = self.last_sent_closing_fee {
+				if min_feerate >= last_feerate {


It should be >, not >=?

If its ==, that means the last time we proposed something to our counterparty it was already min_feerate and they came back with a counter-proposal. Proposing again would get us into a loop.

naumenkogs · 2020-09-21T10:09:03Z

I'm not an expert in weight/fee calculation, but I think this works as intended. Just couple questions.

lightning/src/ln/channel.rs

ariard · 2020-09-21T15:49:42Z

lightning/src/ln/channel.rs

 		let proposed_total_fee_satoshis = proposed_feerate as u64 * tx_weight / 1000;

 		let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(proposed_total_fee_satoshis, false);
 		let sig = self.holder_keys
 			.sign_closing_transaction(&closing_tx, &self.secp_ctx)
 			.ok();
+		assert!(closing_tx.get_weight() as u64 <= tx_weight);


Do we assert somewhere we never sign 0-outputs closing transactions ?

No, I don't think so. I'm not sure what we could do in a world where both sides decide their closing transaction is dust...it feels...more than a bit awkward but you can't stop the closing process and there's not a lot of value in broadcasting it...

ariard · 2020-09-21T15:54:56Z

lightning/src/ln/channel.rs

 		if let Some((_, last_fee, sig)) = self.last_sent_closing_fee {
 			if last_fee == msg.fee_satoshis {
 				self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
+				assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight);
+				debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2);


What the reason of this 2 of allowed diff ? Further as the pre-computed weight is done with the knowledge of number of outputs shouldn't assert be strictly equal to effective transaction weight ?

Signature sizes are variable, here we let it drop by 1 byte, though they could theoretically be even smaller.

nit: maybe could've used a comment on this point

lightning/src/ln/channel.rs

ariard · 2020-09-21T16:10:10Z

lightning/src/ln/channel.rs

-					if min_feerate >= last_feerate {
-						return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something lower ({}) than our Background feerate ({}).", last_feerate, min_feerate)));
-					}
+			min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);


Can we update min_feerate to Background feerate only if it's superior to min-relay-fee ?

The feerate API is very explicit that it must always return at least 253. We probably should check it at every callsite (via some wrapper), but this isn't any different for now.

ariard · 2020-09-21T16:14:17Z

lightning/src/ln/functional_test_utils.rs

+			for output in $tx.output.iter() {
+				total_value_out += output.value;
+			}
+			let min_fee = $tx.get_weight() as u64 / 4; // One sat per vbyte


Or instead compute the effective fee based on min feerate per kw and check if value_in - value_out is equal or superior. Rather not introduce reference to vbyte.

IIUC, bitcoin core requires 1 sat/vbyte, not 1/4 sat/weight, hence the 253 instead of 250. We could check it against 253 sat/weight, but the reality is the minimum is defined in vbytes.

ariard

See #716 (comment)

ariard · 2020-09-24T23:26:08Z

lightning/src/ln/channel.rs

+		1 +                                            // witness element count
+		4 +                                            // 4 element lengths (multisig dummy, 2 sigs, and witness script)
+		self.get_funding_redeemscript().len() as u64 + // funding witness script
+		2*(1 + 71);                                    // two signatures + sighash type flags


IIRC aren't Bitcoin standard (BIP66) ECDSA sigs at most 72? I think we can trigger new asserts as they check real-transaction-weight <= not-bigger-expected-weight ?

Hmm...you may be right, but tests aren't failing. Is it because low-s means we never sign something with an S that has the high bit set (so never need a 0-byte prefix in S)?

After an extended conversation on IRC today, we noted that secp's verify function will fail for high-S, and low-S' requirement is that the S field is <0x7F...., which always has the top bit unset, implying 71 bytes for the DER signature, so I think this is fine. I went ahead and added a comment noting this.

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/channel.rs

ariard · 2020-10-05T13:06:39Z

Code Review ACK 22e20ec pending on fixing comment : #716 (comment)

TheBlueMatt · 2020-10-05T15:30:49Z

@ariard also pointed out on IRC that weight is always rounded up to calculate vbytes for feerates - I updated the PR to match.

Though hopefully we never see a fee of 2.1 million BTC, either...

This resolves a number of bugs around how we calculate feerates on closing transactions: * We previously calculated the weight wrong both by always counting two outputs instead of counting the number of outputs that actually exist in the closing transaction and by not counting the witness redeemscript. * We use assertions to check the calculated weight matches what we actually build (with debug_assertions for variable-length sigs). * As an additional sanity check, we really should check that the transaction had at least min-relay-fee when we were the channel initator.

TheBlueMatt · 2020-10-05T15:32:55Z

Also rebased on upstream to pull CI fixes.

ariard · 2020-10-05T15:39:19Z

lightning/src/ln/functional_test_utils.rs

+				total_value_out += output.value;
+			}
+			let min_fee = ($tx.get_weight() as u64 + 3) / 4; // One sat per vbyte (ie per weight/4, rounded up)
+			// Input amount must be no larger than output amount + minimum relay fee


"Input amount must be larger than output amount + minimum relay fee" as it's okay to have a far larger fee than minimum relay ?

This also pays a fee on the transactions we generate in response to SpendableOutputDescriptors in tests. This fixes the known issues in lightningdevkit#630, though we should test for standardness in other ways as well.

ariard · 2020-10-05T16:11:25Z

Code Review ACK 3219926

valentinewallace

I have one question, but given that's resolved, LGTM 🌟 🌟

valentinewallace · 2020-10-17T22:28:47Z

lightning/src/ln/channel.rs

 		if let Some((_, last_fee, sig)) = self.last_sent_closing_fee {
 			if last_fee == msg.fee_satoshis {
 				self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
+				assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight);
+				debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2);


nit: maybe could've used a comment on this point

valentinewallace · 2020-10-19T18:24:02Z

lightning/src/ln/channel.rs

+		1 +                                            // witness element count
+		4 +                                            // 4 element lengths (2 sigs, multisig dummy, and witness script)
+		self.get_funding_redeemscript().len() as u64 + // funding witness script
+		2*(1 + 71);                                    // two signatures + sighash type flags


are each of the 1's: one 1 for the multisig dummy, one 1 for the sighash flag? (if that makes sense)

Multisig dummy is counted a few lines up (see comment), both signatures have sighash flags, so hence the 2 here.

naumenkogs reviewed Sep 21, 2020

View reviewed changes

ariard reviewed Sep 21, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-fee-tests branch from 5db2641 to 6bde56e Compare September 22, 2020 20:20

ariard reviewed Sep 24, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-fee-tests branch from 6bde56e to 22e20ec Compare September 26, 2020 02:10

TheBlueMatt added this to the 0.0.12 milestone Sep 27, 2020

ariard reviewed Oct 3, 2020

View reviewed changes

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

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

TheBlueMatt force-pushed the 2020-09-fee-tests branch from 22e20ec to e6228f7 Compare October 5, 2020 15:30

TheBlueMatt added 2 commits October 5, 2020 11:32

Fix max fee_satoshis constant to be 21 million, not 2.1 million.

6366fc7

Though hopefully we never see a fee of 2.1 million BTC, either...

TheBlueMatt force-pushed the 2020-09-fee-tests branch from e6228f7 to 9d206a3 Compare October 5, 2020 15:32

ariard reviewed Oct 5, 2020

View reviewed changes

Test that txn pay at least a minimum relay fee in functional tests

3219926

This also pays a fee on the transactions we generate in response to SpendableOutputDescriptors in tests. This fixes the known issues in lightningdevkit#630, though we should test for standardness in other ways as well.

TheBlueMatt force-pushed the 2020-09-fee-tests branch from 9d206a3 to 3219926 Compare October 5, 2020 15:54

TheBlueMatt merged commit 45a558b into lightningdevkit:main Oct 5, 2020

valentinewallace reviewed Oct 19, 2020

View reviewed changes

(Fix and) Test that txn pay at least a minimum relay fee in functional tests #716

(Fix and) Test that txn pay at least a minimum relay fee in functional tests #716

Uh oh!

Conversation

TheBlueMatt commented Sep 18, 2020

Uh oh!

codecov bot commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

naumenkogs commented Sep 21, 2020

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!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ariard commented Oct 5, 2020

Uh oh!

TheBlueMatt commented Oct 5, 2020

Uh oh!

TheBlueMatt commented Oct 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Oct 5, 2020

Uh oh!

valentinewallace 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 18, 2020 •

edited

Loading

TheBlueMatt Sep 22, 2020 •

edited

Loading

ariard left a comment •

edited

Loading