Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6366fc7
Fix max fee_satoshis constant to be 21 million, not 2.1 million.
TheBlueMatt Sep 18, 2020
b43a7e3
Fix feerate calculation on closing transactions
TheBlueMatt Sep 18, 2020
3219926
Test that txn pay at least a minimum relay fee in functional tests
TheBlueMatt Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 46 additions & 15 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,8 +1054,30 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}

#[inline]
fn get_closing_transaction_weight(a_scriptpubkey: &Script, b_scriptpubkey: &Script) -> u64 {
(4 + 1 + 36 + 4 + 1 + 1 + 2*(8+1) + 4 + a_scriptpubkey.len() as u64 + b_scriptpubkey.len() as u64)*4 + 2 + 1 + 1 + 2*(1 + 72)
fn get_closing_transaction_weight(&self, a_scriptpubkey: Option<&Script>, b_scriptpubkey: Option<&Script>) -> u64 {
let mut ret =
(4 + // version
1 + // input count
36 + // prevout
1 + // script length (0)
4 + // sequence
1 + // output count
4 // lock time
)*4 + // * 4 for non-witness parts
2 + // witness marker and flag
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
Copy link

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

if let Some(spk) = a_scriptpubkey {
ret += ((8+1) + // output values and script length
spk.len() as u64) * 4; // scriptpubkey and witness multiplier
}
if let Some(spk) = b_scriptpubkey {
ret += ((8+1) + // output values and script length
spk.len() as u64) * 4; // scriptpubkey and witness multiplier
}
ret
}

#[inline]
Expand Down Expand Up @@ -2880,13 +2902,14 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
if self.feerate_per_kw > proposed_feerate {
proposed_feerate = self.feerate_per_kw;
}
let tx_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.counterparty_shutdown_scriptpubkey.as_ref().unwrap());
let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap()));
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);
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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...

if sig.is_none() { return None; }

self.last_sent_closing_fee = Some((proposed_feerate, total_fee_satoshis, sig.clone().unwrap()));
Expand Down Expand Up @@ -3007,7 +3030,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
if !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() {
return Err(ChannelError::Close("Remote end sent us a closing_signed while there were still pending HTLCs".to_owned()));
}
if msg.fee_satoshis > 21000000 * 10000000 { //this is required to stop potential overflow in build_closing_transaction
if msg.fee_satoshis > 21_000_000 * 1_0000_0000 { //this is required to stop potential overflow in build_closing_transaction
return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned()));
}

Expand All @@ -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 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this ever not have 0/1 outputs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

if let Some(oup) = closing_tx.output.get(1) { Some(&oup.script_pubkey) } else { None });
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);
Copy link

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

self.channel_state = ChannelState::ShutdownComplete as u32;
self.update_time_counter += 1;
return Ok((None, Some(closing_tx)));
Expand All @@ -3042,11 +3070,12 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {

macro_rules! propose_new_feerate {
($new_feerate: expr) => {
let closing_tx_max_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.counterparty_shutdown_scriptpubkey.as_ref().unwrap());
let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * closing_tx_max_weight / 1000, false);
let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap()));
let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * tx_weight / 1000, false);
let sig = self.holder_keys
.sign_closing_transaction(&closing_tx, &self.secp_ctx)
.map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?;
assert!(closing_tx.get_weight() as u64 <= tx_weight);
self.last_sent_closing_fee = Some(($new_feerate, used_total_fee, sig.clone()));
return Ok((Some(msgs::ClosingSigned {
channel_id: self.channel_id,
Expand All @@ -3056,10 +3085,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}
}

let proposed_sat_per_kw = msg.fee_satoshis * 1000 / closing_tx.get_weight() as u64;
let mut min_feerate = 253;
if self.channel_outbound {
let max_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
if (proposed_sat_per_kw as u32) > max_feerate {
if (msg.fee_satoshis as u64) > max_feerate as u64 * closing_tx_max_weight / 1000 {
if let Some((last_feerate, _, _)) = self.last_sent_closing_fee {
if max_feerate <= last_feerate {
return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something higher ({}) than our Normal feerate ({})", last_feerate, max_feerate)));
Expand All @@ -3068,21 +3097,23 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
propose_new_feerate!(max_feerate);
}
} else {
let min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
if (proposed_sat_per_kw as u32) < min_feerate {
if let Some((last_feerate, _, _)) = self.last_sent_closing_fee {
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);
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be >, not >=?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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)));
}
propose_new_feerate!(min_feerate);
}
propose_new_feerate!(min_feerate);
}

let sig = self.holder_keys
.sign_closing_transaction(&closing_tx, &self.secp_ctx)
.map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?;
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);

self.channel_state = ChannelState::ShutdownComplete as u32;
self.update_time_counter += 1;
Expand Down
16 changes: 14 additions & 2 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,14 +505,26 @@ pub fn create_announced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a
macro_rules! check_spends {
($tx: expr, $($spends_txn: expr),*) => {
{
$tx.verify(|out_point| {
let get_output = |out_point: &bitcoin::blockdata::transaction::OutPoint| {
$(
if out_point.txid == $spends_txn.txid() {
return $spends_txn.output.get(out_point.vout as usize).cloned()
}
)*
None
}).unwrap();
};
let mut total_value_in = 0;
for input in $tx.input.iter() {
total_value_in += get_output(&input.previous_output).unwrap().value;
}
let mut total_value_out = 0;
for output in $tx.output.iter() {
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 - output amount = fee, so check that out + min_fee is smaller than input
assert!(total_value_out + min_fee <= total_value_in);
$tx.verify(get_output).unwrap();
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4708,6 +4708,7 @@ macro_rules! check_spendable_outputs {
input: vec![input],
output: vec![outp],
};
spend_tx.output[0].value -= (spend_tx.get_weight() + 2 + 1 + 73 + 35 + 3) as u64 / 4; // (Max weight + 3 (to round up)) / 4
let secp_ctx = Secp256k1::new();
let keys = $keysinterface.derive_channel_keys($chan_value, key_derivation_params.0, key_derivation_params.1);
let remotepubkey = keys.pubkeys().payment_point;
Expand Down Expand Up @@ -4742,6 +4743,7 @@ macro_rules! check_spendable_outputs {

let delayed_payment_pubkey = PublicKey::from_secret_key(&secp_ctx, &delayed_payment_key);
let witness_script = chan_utils::get_revokeable_redeemscript(revocation_pubkey, *to_self_delay, &delayed_payment_pubkey);
spend_tx.output[0].value -= (spend_tx.get_weight() + 2 + 1 + 73 + 1 + witness_script.len() + 1 + 3) as u64 / 4; // (Max weight + 3 (to round up)) / 4
let sighash = Message::from_slice(&bip143::SigHashCache::new(&spend_tx).signature_hash(0, &witness_script, output.value, SigHashType::All)[..]).unwrap();
let local_delayedsig = secp_ctx.sign(&sighash, &delayed_payment_key);
spend_tx.input[0].witness.push(local_delayedsig.serialize_der().to_vec());
Expand Down Expand Up @@ -4769,6 +4771,7 @@ macro_rules! check_spendable_outputs {
input: vec![input],
output: vec![outp.clone()],
};
spend_tx.output[0].value -= (spend_tx.get_weight() + 2 + 1 + 73 + 35 + 3) as u64 / 4; // (Max weight + 3 (to round up)) / 4
let secret = {
match ExtendedPrivKey::new_master(Network::Testnet, &$node.node_seed) {
Ok(master_key) => {
Expand Down