Anchor-outputs (1/3): Refactoring chain backend to extract PackageTemplate #642

ariard · 2020-06-19T07:48:20Z

This is 1/3 of anchor ouput implementation

I invite reviewers to only Concept ACK/Approach ACK until I open the complete serie of patchset to see if implementation makes sense.

Rational

Anchor ouptut proposal is a spec move towards introducing dynamic fee-setting for LN transactions. Actually, feerate is selected by channel opener through update_fee and must be adapted to follow network-wise mempool increases/decreases. This model is broken with regards to quick fee spikes, adversial counterparty exploiting mempool policy rules or update_fee in itself introducing funny attack vectors (Flood & Loot among others).

Anchor output modifies commitment transaction format by introducing a special per-party output for adjusting feerate through CPFP. Note, without base layer changes this is still insecure and can only be relied for favorable mempool congestion in case of intentional unilateral closure.

Implementing anchor output consists in both onchain changes to support CPFP logic and offchain ones to dynamically add this new anchor output. This CPFP logic and the further onchain backend will likely get more complex in the future both by security and fee efficiency (like "aggressive" aggregation, cross-commitment CPFP, don't-go-onchain-if-your-HTLC-is-non-economic, don't-bump-more-than-HTLC-in-danger, ...). So this PR envision this by removing code complexity from OnchainTxHandler to some new interface OnchainRequest/PackageTemplate mimicking "packets".

Motivations behind OnchainRequest/PackageTemplate is to provide a generic interface for OnchainTxHandler without this component being aware of actual transaction type processed. It's nice to introduce later pre-signed justice transactions for watchtowers or anchor output garbage-collecting transactions.

Interface holds high-level methods like package_weight or package_amounts. Concretely it kills the monster OnchainTxHandler::generate_claim_tx to get it ready for CPFP support. Current interface is an enum, we may want to switch to trait object but it comes with trade-offs in Rust (thoughts ?)

PR scope

This PR SHOULD NOT introduce any behavior change.

Overview of PR:

move InputDescriptors in new onchain_utils
replace InputDescriptors by PackageTemplate, switching from per-input claim to holistic package make it easier to compute weight, which is less footgunish
replace ClaimTxBumpMaterial/ClaimRequest by a unique structure, easing aggregatiion
extract fee computing utilitties from `OnchainTxHandler

lightning/src/ln/channel.rs

lightning/src/ln/onchain_utils.rs

arik-so · 2020-06-19T16:59:43Z

lightning/src/ln/onchain_utils.rs

+			},
+		}
+	}
+	pub(crate) fn package_split(&mut self, outp: &BitcoinOutPoint) -> Option<PackageTemplate> {


This method needs comments, because package_split is not descriptive.

lightning/src/ln/onchain_utils.rs

arik-so · 2020-06-19T17:01:27Z

lightning/src/ln/onchain_utils.rs

+			}
+		}
+	}
+	pub(crate) fn package_merge(&mut self, mut template: PackageTemplate) {


Same as package_split.

arik-so · 2020-06-19T17:13:45Z

lightning/src/ln/onchaintx.rs

 			// Don't claim a outpoint twice that would be bad for privacy and may uselessly lock a CPFP input for a while
-			if let Some(_) = self.claimable_outpoints.get(&req.outpoint) { log_trace!(logger, "Bouncing off outpoint {}:{}, already registered its claiming request", req.outpoint.txid, req.outpoint.vout); } else {
+			if let Some(_) = self.claimable_outpoints.get(req.content.outpoints()[0]) { log_trace!(logger, "Bouncing off outpoint {}:{}, already registered its claiming request", req.content.outpoints()[0].txid, req.content.outpoints()[0].vout); } else {


you keep accessing the first outpoint, so I'd just put it in a variable for easier legibility

devrandom

Here are my initial thoughts on this PR

lightning/src/ln/onchain_utils.rs

devrandom · 2020-11-08T12:42:38Z

lightning/src/ln/onchain_utils.rs

+/// of the claim tx has its feerate increased. For the latter case, access to the whole package
+/// sizea and pre-committed fee is required to compute an efficient bump.
+#[derive(Clone, PartialEq)]
+pub(crate) enum PackageTemplate {


There's a lot of matching going on in the implementation code below. Maybe it will be cleaner to have this be a hierarchy of types rather than an enum?

Good point, I think we can aggregate MalleableJusticeTx/CounterpartyHTLCTx on a lot of cases. Though I'm holding this on the discussion to switch to dynamic trait objects instead of an enum pseudo-interface.

Dynamic traits kinda seems like cheating here, can we not convert the trait variants to enum variants and place structs that each implement some PackageTemplateVariant trait for the inner structs instead? That would keep runtime cost the same but allow similar code cleanup.

lightning/src/ln/onchain_utils.rs

devrandom · 2020-11-08T12:45:13Z

lightning/src/ln/onchain_utils.rs

+		};
+		bumped_tx.get_weight() + witnesses_weight
+	}
+	pub(crate) fn package_finalize<L: Deref, ChanSigner: ChannelKeys>(&self, onchain_handler: &mut OnchainTxHandler<ChanSigner>, value: u64, destination_script: Script, logger: &L) -> Option<Transaction>


this creates a circular dependency between this class and OnChainTxHandler. Perhaps extract an interface from OnChainTxHandler to break this cycle? actually this seems to be a pre-existing issue.

Yes in the future I think we can just pass key_signer as we're just interested with signer for construction and signing. Maybe we can remove entirely signer from OnchainTxHandler and just give it to PackageTemplate constructor ?

Note, you still have get_fully_signed_htlc_tx/get_fully_signed_holder_tx blocking to remove the passing of OnchainTxHandler, it's a leftover from the past which is my bad. OnchainTxHandler shouldn't have any commitment transaction state directly exposed and it should be wrapped inside package. I started to do this move while writing this PR but it was already a big refactor...

yeah, sounds like a good topic for a separate PR

lightning/src/ln/onchain_utils.rs

devrandom · 2020-11-08T12:55:20Z

lightning/src/ln/onchain_utils.rs

+		}
+	// ...else just increase the previous feerate by 25% (because that's a nice number)
+	} else {
+		let fee = previous_feerate * (predicted_weight as u64) / 750;


isn't this 1000/750 = 33% or did I miss something?

I think you're right :

(feerate=1000) * (weight=100) / (weight_denominator=1000) = 100

(feerate=1000) * (weight=100) / (weight_denominator=750) = 133

So good catch! I need to update tests so not marking at resolved yet. If you have a better idea on the % heuristic lmk it's quite arbitrary rn :)

well, to be obvious, this has to be high enough so it tracks feerate spikes within the expiration time, especially when there's an attack on the mempool. if I understand the code correctly, a bump happens on every block, right?

so, the desired bump % is dependent on the expiration time in blocks and the ratio of the spike to original feerate. one issue is if the original feerate was very low, let's say because the network was quiet. then perhaps we can get a very high spike ratio.

for example, with a 100x spike height and 144 block expiration, the bump should be around 3.2%. we could handle much higher spikes with a 10% increase - up to a (1.1 ^ 144) = 900000 feerate increase over 144 blocks.

we should have this in a const with some documentation on the const justifying the value based on assumptions on maximum spike height and minimum expiration time.

Bump happens accordingly to OnchainTxHandler::get_height_timer, if expiration is coming in next 3 blocks, bumps at every block.

What do you qualify as spike height here ? The original height at which the transaction is broadcast for first time ? Ideally you want the fee-bump to be exponential, save on fees if timelock is far away, be ready to burn as funds at stake when the expiration is really near.

Agree on documenting our assumptions.

apologies, missed this reply. I should have said "spike factor" - which I meant as the ratio of the feerate when we start to try to publish and the feerate at the peak of the spike.

I agree that the bumps should be exponential (and they are in your code, right?).

But my assumption about the bump frequency was incorrect. Looking at get_height_timer, if we have 144 blocks, we bump 9 times every 15 blocks, then twice every 3 blocks then another 3 times, for a total of 14 or so. I might be slightly off. so 1.25 ^ 14 = 22.

I'm not sure that's actually enough to be safe, since we do stand to lose the funds in the worst case. I think a factor of 100 would be better.

ariard · 2020-11-10T02:25:21Z

Thanks for review @devrandom, updated with at least the minor fixed.

I think before to go further with this PR review there is a discussion between using enum as a pseudo-interface or moving to trait objects. The downside with trait objects (experimented a bit for offchain generic output in #619) is you need a Box and a memory over-head. It might be okay for onchain stuff which isn't a hot path.

Note, I see this PR as a real blocker to work further on third-party watchtower support, future PackageTemplate should be DelegatedJusticeTx/HolderDLCTx. Once we resolve the code design point above, I would be glad to land this PR as soon as we can.

devrandom · 2020-11-10T15:58:46Z

Regarding efficiency - I usually don't try to optimize this early in a project's lifetime. This especially applies to crypto code, where sign/encrypt/verify operations are usually the bulk of the CPU load.

ariard · 2021-03-09T00:11:24Z

Updated at de93efc.

Currently, this is the main area where I'm curious of feedback. If we prefer trait objects, I'll need to re-work this PR, so first I would like to settle on trait objecs or enum as a abstract package interface.

I think before to go further with this PR review there is a discussion between using enum as a pseudo-interface or moving to trait objects. The downside with trait objects (experimented a bit for offchain generic output in #619) is you need a Box and a memory over-head. It might be okay for onchain stuff which isn't a hot path.

TheBlueMatt

Overall looks like a good refactor. Have a couple of API questions.

lightning/src/ln/onchain_utils.rs

TheBlueMatt · 2021-03-19T15:56:52Z

lightning/src/ln/onchain_utils.rs

+	}
+}
+
+/// An enum to describe a claim content which is generated by ChannelMonitor and


Can you expand the doc comments and add docs on each variant describing these in t a bit more detail? Does this datastructure restrict the possible things we can claim in one transaction (ie can we claim CounterpartyHTLCTx outputs in the same transaction as MalleableJusticeTx outputs, or do we always punish revoked counterpaty HTLC transactions via a MalleableJusticeTx PackageTemplate)? More docs would help here.

TheBlueMatt · 2021-03-19T16:03:03Z

lightning/src/ln/onchain_utils.rs

+/// of the claim tx has its feerate increased. For the latter case, access to the whole package
+/// sizea and pre-committed fee is required to compute an efficient bump.
+#[derive(Clone, PartialEq)]
+pub(crate) enum PackageTemplate {


Dynamic traits kinda seems like cheating here, can we not convert the trait variants to enum variants and place structs that each implement some PackageTemplateVariant trait for the inner structs instead? That would keep runtime cost the same but allow similar code cleanup.

lightning/src/ln/onchain_utils.rs

TheBlueMatt · 2021-03-19T18:15:56Z

lightning/src/ln/onchain_utils.rs

+	pub(crate) height_timer: Option<u32>,
+	// Block height before which claiming is exclusive to one party,
+	// after reaching it, claiming may be contentious.
+	pub(crate) absolute_timelock: u32,


Same here, shouldn't this be a function on PackageTemplate?

Yeah, I'll should study if I can merge OnchainRequest with PackageTemplate, now that I've isolated outpoints and their solving materials in PackageSolvingData. Maybe, the original reason is to facilitate aggregation/fragmentation at block connect/disconnect.

TheBlueMatt · 2021-03-19T19:15:56Z

Regarding efficiency - I usually don't try to optimize this early in a project's lifetime. This especially applies to crypto code, where sign/encrypt/verify operations are usually the bulk of the CPU load.

This I agree with for CPU operations, but memory allocation operations are more tricky. My personal experience on Core made me very, very scared of too much allocation, its very easy to have a program that ends up with 3x RSS as is required from memory fragmentation and other overhead, and not really have an easy way to fix it - trying to reduce memory allocations after the fact can be a very challenging rebase, doubly so because there aren't any good tools for debugging or profiling these issues.

devrandom · 2021-03-23T15:28:20Z

lightning/src/ln/onchain_utils.rs

+/// An enum to describe a claim content which is generated by ChannelMonitor and
+/// used by OnchainTxHandler to regenerate feerate-bump transactions to settle claims.
+///
+/// Template may be either malleable (a justice tx, a counterparty HTLC tx) or lockdown (a holder htlc


Why is "counterparty HTLC tx" malleable, but "holder HTLC tx" not? is the term "HTLC tx" used differently in the two cases?

Holder HTLC tx only become malleable with anchor changes introduced in other branches of this patchset :)
Though they're only malleable under SIGHASH_SINGLE. Claiming transactions on counterparty HTLC outputs have always been malleable beyond witness satisfaction (i.e settup nLocktime correctly) ?

ariard · 2021-04-19T00:05:39Z

Okay pushed a refactored version of the refactor at 8b27973. It gets rides of code duplication by encapsulating outputs special solving data in a new PackageSolvingData. Still needs heavily documentation but will do once we agree on onchain_utils's structs.

Also, another point I did like with trait objects compare to enum was the ability to easily support other package/outputs types without this new logic being in-tree. Though, we can do it with enum to, e.g add CounterpartyPTLCOutput in PackageSolvingData that we can gate on #[cfg(feature = "ptlc")]

ariard · 2021-04-22T00:32:07Z

Rebased at 238dd96. Working on API documentation and test coverage.

TheBlueMatt

Note that a number of tests fail, and a few appear to hang.

TheBlueMatt · 2021-05-04T14:30:13Z

lightning/src/chain/channelmonitor.rs

@@ -1565,10 +1420,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		// *we* sign a holder commitment transaction, not when e.g. a watchtower broadcasts one of our
 		// holder commitment transactions.
 		if self.broadcasted_holder_revokable_script.is_some() {
-			let (claim_reqs, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx);
+			let (claim_reqs, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, /*XXX(ariard) Option<height> */ 0);


I assume these weren't meant to slip in :)

Removed, they're markers for the remaining patchset. For height values passed they don't matter as we can't yet bump them.

TheBlueMatt · 2021-05-04T14:51:28Z

lightning/src/chain/onchaintx.rs

@@ -286,9 +150,9 @@ pub struct OnchainTxHandler<ChannelSigner: Sign> {
 	// us and is immutable until all outpoint of the claimable set are post-anti-reorg-delay solved.
 	// Entry is cache of elements need to generate a bumped claiming transaction (see ClaimTxBumpMaterial)
 	#[cfg(test)] // Used in functional_test to verify sanitization
-	pub pending_claim_requests: HashMap<Txid, ClaimTxBumpMaterial>,
+	pub pending_claim_requests: HashMap<Txid, PackageTemplate>,


rustc doesn't like this - even though the whole module is only pub(crate) the fact that this includes a pub(crate) thing makes it sad. We can just make this pub(crate) instead.

TheBlueMatt · 2021-05-04T14:55:50Z

lightning/src/chain/onchain_utils.rs

+	counterparty_delayed_payment_base_key: PublicKey,
+	counterparty_htlc_base_key: PublicKey,
+	per_commitment_key: SecretKey,
+	input_descriptor: InputDescriptors,


Its a little confusing that a RevokedOutput could have type OfferedHTLC/ReceivedHTLC. Can we split out the type here or assert that the type is a Revoked... one in build (it looks like the non-revoked types are really just used for get_witnesses_weight).

So I dried-up InputDescriptors and instead I'm using new named constants. It turns out InputDescriptors and the associated helper get_witnesses_weight don't serve a real purpose. At detection, spend weights can be marked as static, we won't touch them anymore until transaction generation.

TheBlueMatt · 2021-05-04T15:00:06Z

lightning/src/chain/onchain_utils.rs

+		let weight = match self {
+			PackageSolvingData::RevokedOutput(ref outp) => { get_witnesses_weight(&[outp.input_descriptor]) },
+			PackageSolvingData::CounterpartyHTLCOutput(ref outp) => { get_witnesses_weight(if outp.preimage.is_some() { &[InputDescriptors::OfferedHTLC] } else { &[InputDescriptors::ReceivedHTLC] }) },
+			PackageSolvingData::HolderHTLCOutput(..) => { 0 },


Can you add a comment describing why its ok that these are 0 here, or what the calling preconditions are for this function?

Added a comment, values should change with the remaining anchor patchset, introducing fee-bumping of holder outputs spending packages.

codecov · 2021-05-11T16:24:36Z

Codecov Report

Merging #642 (499d84c) into master (f8450a7) will decrease coverage by 0.02%.
The diff coverage is 85.87%.

❗ Current head 499d84c differs from pull request most recent head 0065308. Consider uploading reports for the commit 0065308 to get more accurate results

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   90.42%   90.40%   -0.03%     
==========================================
  Files          59       59              
  Lines       30173    30299     +126     
==========================================
+ Hits        27285    27392     +107     
- Misses       2888     2907      +19

Impacted Files	Coverage Δ
lightning/src/util/enforcing_trait_impls.rs	`90.38% <ø> (ø)`
lightning/src/routing/router.rs	`96.05% <50.00%> (-0.12%)`	⬇️
lightning/src/util/ser_macros.rs	`91.11% <62.06%> (-5.63%)`	⬇️
lightning/src/ln/channel.rs	`88.24% <81.81%> (-0.06%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`94.90% <88.88%> (-0.17%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.19% <95.83%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.02% <97.43%> (-0.05%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.21% <100.00%> (+0.05%)`	⬆️
lightning/src/chain/keysinterface.rs	`94.96% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/msgs.rs	`88.20% <100.00%> (-0.02%)`	⬇️
... and 6 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 f8450a7...0065308. Read the comment docs.

TheBlueMatt

This is great, but I think we can simplify the logic a bunch more and pull more out of channelmonitor. It may be that some of it makes more sense in a followup, but probably it could go in here.

TheBlueMatt · 2021-05-12T19:13:09Z

lightning/src/ln/onchain_utils.rs

 #[derive(PartialEq, Clone, Copy)]
 pub(crate) enum InputDescriptors {
 	RevokedOfferedHTLC,
 	RevokedReceivedHTLC,
 	OfferedHTLC,
 	ReceivedHTLC,
-	RevokedOutput, // either a revoked to_local output on commitment tx, a revoked HTLC-Timeout output or a revoked HTLC-Success output
+	RevokedOutput, // either a revoked to_holder output on commitment tx, a revoked HTLC-Timeout output or a revoked HTLC-Success output


In the previous commit (while moving this block) you change it from to_holder to to_local, and then change it back here.

Right, corrected in "Add onchain_utils"

TheBlueMatt · 2021-05-12T19:16:08Z

lightning/src/ln/onchain_utils.rs

+}
+
+impl Writeable for RevokedOutput {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {


nit here and below: can we implement this with impl_writeable!()?

Yep good idea, done.

TheBlueMatt · 2021-05-12T19:16:40Z

lightning/src/ln/onchain_utils.rs

+
+impl Readable for RevokedOutput {
+	fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+		let per_commitment_point = Readable::read(reader)?;


style nit: If we're reading this in-order we can also put the Readable::read calls inside the struct creation below.

TheBlueMatt · 2021-05-12T19:18:27Z

lightning/src/ln/onchain_utils.rs

+		let weight = match self {
+			PackageSolvingData::RevokedOutput(ref outp) => { get_witnesses_weight(&[outp.input_descriptor]) },
+			PackageSolvingData::CounterpartyHTLCOutput(ref outp) => { get_witnesses_weight(if outp.preimage.is_some() { &[InputDescriptors::OfferedHTLC] } else { &[InputDescriptors::ReceivedHTLC]}) },
+			// Note: Currently, weights of holder outputs spending witnesses aren't used


If this is unused can we at least have a debug_assert!(false) instead of returning a bogus value?

Amount is exercised in generate_claim_tx so added a debug_assert(amt == 0) otherwise added for weight.

TheBlueMatt · 2021-05-12T19:34:42Z

lightning/src/chain/channelmonitor.rs

@@ -1571,7 +1571,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			// First, process non-htlc outputs (to_holder & to_counterparty)
 			for (idx, outp) in tx.output.iter().enumerate() {
 				if outp.script_pubkey == revokeable_p2wsh {
-					let revk_outp = RevokedOutput::build(per_commitment_point, per_commitment_key, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, InputDescriptors::RevokedOutput, outp.value, None, self.counterparty_tx_cache.on_counterparty_tx_csv);
+					let revk_outp = RevokedOutput::build(per_commitment_point, per_commitment_key, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, WEIGHT_REVOKED_OUTPUT, outp.value, None, self.counterparty_tx_cache.on_counterparty_tx_csv);


This feels like even more of a layer violation than the original InputDescriptors enum. I think my original complaint about it was more about how its confusing to have two separate enums (and that you can have a non-Revoked InputDescriptor inside of a RevokedOutput which is obviously nonsensical). Instead, can we make RevokedOutput at least contain an enum so that the htlc field isn't an option and we don't need to store a weight passed in from another layer and can calculate it locally?

Addressed with new RevokedHTLCOutput and corresponding constructor

TheBlueMatt · 2021-05-12T19:40:09Z

lightning/src/chain/channelmonitor.rs

-					let witness_data = InputMaterial::Revoked { per_commitment_point, counterparty_delayed_payment_base_key: self.counterparty_tx_cache.counterparty_delayed_payment_base_key, counterparty_htlc_base_key: self.counterparty_tx_cache.counterparty_htlc_base_key, per_commitment_key, input_descriptor: InputDescriptors::RevokedOutput, amount: outp.value, htlc: None, on_counterparty_tx_csv: self.counterparty_tx_cache.on_counterparty_tx_csv};
-					claimable_outpoints.push(ClaimRequest { absolute_timelock: height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32, aggregable: true, outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 }, witness_data});
+					let revk_outp = RevokedOutput::build(per_commitment_point, per_commitment_key, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, WEIGHT_REVOKED_OUTPUT, outp.value, None, self.counterparty_tx_cache.on_counterparty_tx_csv);
+					let justice_package = PackageTemplate::build_package(commitment_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable, height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32, true, 0, None, height);


Similarly - can we simplify this further and push more logic down into onchain_utils? eg even having channelmonitor aware of what types of outputs are and are not Malleable seems like split logic - ideally ChannelMonitor can only know about the transactions its looking at and then tell onchain_utils or onchaintx which of a given enum an output is and let it handle the rest (maybe with different constructors depending on the types out outputs being spent instead of one big constructor?).

Good call, this one is super straightforward to address. Instead of splitting constructor, I did embed malleability assignment based on output type. channelmonitor is now blind w.r.t to package malleability

ariard · 2021-05-15T23:14:22Z

Rebased on top of #923, with comments fixed at 59a27af

TheBlueMatt · 2021-05-16T21:04:58Z

Concept ACK, this looks really good. I'll give it a more thorough review ASAP.

jkczyz

Concept ACK. Feel free to ignore some of the more nitty comments for now. Interested more in constructing packages and serialization.

lightning/src/chain/channelmonitor.rs

jkczyz · 2021-05-17T06:57:55Z

lightning/src/ln/onchain_utils.rs

+/// A package is defined as one or more transactions claiming onchain outputs in reaction
+/// to confirmation of a channel transaction. Those packages might be aggregated to save on
+/// fees, if satisfaction of outputs's witnessScript let's us do so.


Is there a distinction between a "package" and a "package template"?

Yes a package is just a chain of transactions. A package template a chain of transactions with a superset of constraints such as "should be confirmed in chain before height X" or "must not be aggregated".

I concede that currently chain of transactions are one-sized which might make it confusing but in the rest of the patchset, commitment transactions are attached a CPFP, such becoming two-sized.

lightning/src/ln/onchain_utils.rs

jkczyz · 2021-05-17T17:57:02Z

lightning/src/ln/onchain_utils.rs

+		if self.malleability == PackageMalleability::Untractable || merge_from.malleability == PackageMalleability::Untractable {
+			panic!("Merging template on untractable packages");
+		}
+		if !self.aggregation || !merge_from.aggregation {
+			panic!("Merging non aggregatable packages");
+		}


Can these be enforced by the type system? PackageTemplate feels like a builder.

Can you precise further your thinking here ? Ofc I can encode deeper aggregation/malleability at the output-level (i.e PackageSolvingData) and if aggregation logic is correct it should ensure similarity of aggregation/malleability among package members. Though is it a big win in term of API clarity ?

Further it will inflate serialized package size as you will redundantly encode properties ?

Can you precise further your thinking here ? Ofc I can encode deeper aggregation/malleability at the output-level (i.e PackageSolvingData) and if aggregation logic is correct it should ensure similarity of aggregation/malleability among package members. Though is it a big win in term of API clarity ?

I'm thinking something similar to InvoiceBuilder where PackageTemplate would be constructed piecemeal (i.e., chaining method calls for each field) and where these properties are enforced by the type system by conditionally implementing methods (e.g., a PackageTemplate marked by a type parameter as Malleable could only be merged with another such PackageTemplate). Similarly with aggregation.

TBH, I don't see a straightforward way to do such conditional methods implementation and considering the API should change a bit with remaining PRs of the patchset (e.g some output types becoming malleable, CPFP attachment, ...), I would wait for them to land before to solidify the API now ? I think that's a good direction, just maybe too early.

lightning/src/ln/onchain_utils.rs

TheBlueMatt

I think I've basically gone through the whole thing in detail now. A few methods could be squashed to clean it up even more but it looks good.

lightning/src/chain/onchaintx.rs

TheBlueMatt · 2021-05-17T01:38:26Z

lightning/src/chain/onchaintx.rs

@@ -305,7 +169,7 @@ pub struct OnchainTxHandler<ChannelSigner: Sign> {

 	latest_height: u32,

-	secp_ctx: Secp256k1<secp256k1::All>,
+	pub(super) secp_ctx: Secp256k1<secp256k1::All>,


This doesn't seem to be used at the end (and is a layer violation IMO).

It's still used in onchain_utils, actually I could initiate PackageTemplate with a Signer/secp256k1::All, though we don't have anymore Signer access in ChannelMonitor.

If we make OnchainTxhandler its own interface in the future, you may be able to pass a differing Signer than the ChannelMonitor though does this flexibility is really worthy and do we want to keep signing operations in OnchainTxHandler?

Huh, I thought I compile-checked it. Its fine to leave as-is if its actually used, I just didn't see where.

Have a look in PackageSolvingData::finalize_input ?

lightning/src/chain/onchain_utils.rs

TheBlueMatt · 2021-05-18T18:41:33Z

lightning/src/chain/channelmonitor.rs

+						continue;
+					}
+				} else { None }, htlc.amount_msat);
+				let htlc_package = PackageTemplate::build_package(holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), height, false, 0, None, height);


I'm a bit confused by the timelock semantics. Previously we were always setting absolute_timelock to none here, but now we set it to the current height (though it should be current height + 1 if we're trying to do the "always timelock transactions" thing). Should we be moving towards absolute_timelock really meaning "the real timelock when this transaction can hit the chain"? I do think that would make it much more useful.

In that case, does the absolute_timelock here not need to be HTLC timelock + height (for HTLC-Timeout transactions)? May be worth adding some debug assertions that the transactions generated have a timelock that matches the absolute_timelock in the PackageTemplate.

I think I would agree on defining absolute_timelock as "the real hit when counterparty transaction can hit the chain" ?

Right, timelock should not be height but in fact htlc.cltv_expiry. Though current value of height is just a tweak to avoid generating non-bumped HTLC-transactions for nothing, which would break some tests. Note, it should be changed in the rest of the patchset iirc.

Right, I think the other few calls already use that definition, just not this one, though it may also make sense to always do max(HTLC timeout, current height + 1)?

IIRC, if HTLC timeout is already behind the chain tip, we're going to bump it at every block according to get_block_height_timer, so doing max(HTLC timeout, current height + 1) isn't going to change anything.

jkczyz

Mostly nits on chain_utils. Can take a closer look at later commits tonight.

lightning/src/ln/onchain_utils.rs

lightning/src/chain/onchain_utils.rs

jkczyz

Code mostly looks good. Comments are primarily around doc confusion and code organization.

lightning/src/ln/onchain_utils.rs

jkczyz · 2021-05-19T06:44:44Z

lightning/src/ln/onchain_utils.rs

@@ -821,3 +822,104 @@ pub(crate) fn get_witnesses_weight(inputs: &[InputDescriptors]) -> usize {
 	}
 	tx_weight
 }
+
+/// Attempt to propose a fee based on consumed outpoints's values and predicted weight of the claiming


It's unclear what outpoints are being consumed from this context. Seems to be talking about a call site. Likewise throughout.

Yes, reword a bit, let me know if it's clearer.

lightning/src/ln/onchain_utils.rs

lightning/src/chain/mod.rs

jkczyz · 2021-05-19T07:13:03Z

lightning/src/chain/onchaintx.rs

@@ -347,7 +333,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {

 		// Compute new height timer to decide when we need to regenerate a new bumped version of the claim tx (if we
 		// didn't receive confirmation of it before, or not enough reorg-safe depth on top of it).
-		let new_timer = Some(Self::get_height_timer(height, cached_request.timelock()));
+		let new_timer = Some(onchain_utils::get_height_timer(height, cached_request.timelock()));


Why move this to onchain_utils if it is only used here? I think we should avoid "utils" modules as they tend to collect unrelated code.

Right, made it part of the package interface as it's belonging to it. We don't have an other data struct with a height timer through the rest of chain/

lightning/src/chain/package.rs

lightning/src/ln/package.rs

jkczyz · 2021-05-21T08:01:39Z

lightning/src/ln/package.rs

+#[derive(PartialEq)]
+enum OutputCategory {
+	Revoked,
+	CounterpartyOfferedHTLC,
+	CounterpartyReceivedHTLC,
+	HolderHTLC,
+	HolderFunding,
+}


Looking at how this is used, it doesn't really seem necessary. You can instead have a method on PackageSolvingData that takes another PackageSolvingData and returns whether they are compatible. No need to introduce the term "category" which isn't defined. This would also eliminate the redundancy between OutputCategory and PackageSolvingData.

BTW, such comparison can be accomplished for complex enum types like PackageSolvingData using std::mem::discriminant.

https://doc.rust-lang.org/std/mem/fn.discriminant.html

Okay finally dropped OutputCategory/OutputType! And leverage usage of std::mem::discriminant. There is still a bunch of special logic in output_type_eq to handle the forced-equality for revoked outputs.

lightning/src/chain/package.rs

jkczyz · 2021-05-24T16:38:50Z

lightning/src/chain/package.rs

+					PackageSolvingData::RevokedHTLCOutput(..) => { return true },
+					PackageSolvingData::RevokedOutput(..) => { return true },


If these variants only differ by one field but are considered "equal", would it be simpler to make that particular field an enum and collapse these two variants into one variant rather than repeat the six other fields across each?

I was anticipating future implementation of disaggregation-after-timelock to solve this transaction pinning : lightning/bolts#803. If this mitigation is implemented they won't be considered "equal" anymore.

Package.rs aims to gather interfaces to communicate between onchain channel transactions parser (ChannelMonitor) and outputs claiming logic (OnchainTxHandler). These interfaces are data structures, generated per-case by ChannelMonitor and consumed blindly by OnchainTxHandler.

PackageTemplate aims to replace InputMaterial, introducing a clean interface to manipulate a wide range of claim types without OnchainTxHandler aware of special content of each. This is used in next commits.

Duplicated code in onchain.rs is removed in next commits.

This commit replaces InputMaterial in both ChannelMonitor/ OnchainTxHandler. This doesn't change behavior.

jkczyz

Looks good! Thanks for addressing everything.

TheBlueMatt

Looks good. Anything else can come in a followup.

arik-so reviewed Jun 19, 2020

View reviewed changes

devrandom reviewed Nov 8, 2020

View reviewed changes

TheBlueMatt reviewed Mar 19, 2021

View reviewed changes

devrandom reviewed Mar 23, 2021

View reviewed changes

TheBlueMatt added this to the 0.0.15 milestone May 4, 2021

TheBlueMatt reviewed May 4, 2021

View reviewed changes

jkczyz assigned jkczyz and unassigned jkczyz May 4, 2021

jkczyz self-requested a review May 4, 2021 23:41

valentinewallace mentioned this pull request May 12, 2021

Panic due to funding transaction rejected with "non-BIP68-final" lightningdevkit/ldk-sample#9

Closed

TheBlueMatt reviewed May 12, 2021

View reviewed changes

jkczyz reviewed May 17, 2021

View reviewed changes

TheBlueMatt approved these changes May 17, 2021

View reviewed changes

TheBlueMatt reviewed May 18, 2021

View reviewed changes

jkczyz reviewed May 19, 2021

View reviewed changes

jkczyz reviewed May 21, 2021

View reviewed changes

TheBlueMatt reviewed May 21, 2021

View reviewed changes

lightning/src/chain/package.rs Show resolved Hide resolved

TheBlueMatt reviewed May 21, 2021

View reviewed changes

lightning/src/chain/package.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 21, 2021

View reviewed changes

lightning/src/chain/package.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 24, 2021

View reviewed changes

lightning/src/chain/package.rs Outdated Show resolved Hide resolved

jkczyz reviewed May 24, 2021

View reviewed changes

Antoine Riard added 7 commits May 25, 2021 19:54

Introduce PackageTemplae, a replacement of InputMaterial

c9bd7b3

PackageTemplate aims to replace InputMaterial, introducing a clean interface to manipulate a wide range of claim types without OnchainTxHandler aware of special content of each. This is used in next commits.

Duplicate fee computation utilities out of OnchainTxHandler

2d451b8

Duplicated code in onchain.rs is removed in next commits.

Integrate PackageTemplate

5ccb075

This commit replaces InputMaterial in both ChannelMonitor/ OnchainTxHandler. This doesn't change behavior.

Move onchain* to chain/

4363f96

Move get_height_timer out of OnchainTxHandler

bb0e33f

Move compute_output_value as part of package member functions

881c0f5

TheBlueMatt mentioned this pull request May 26, 2021

Migrate some inner structs to TLVs #928

Merged

Add package template unit tests

c40ebf1

jkczyz approved these changes May 26, 2021

View reviewed changes

TheBlueMatt approved these changes May 26, 2021

View reviewed changes

TheBlueMatt merged commit f074343 into lightningdevkit:master May 26, 2021

		PackageSolvingData::RevokedHTLCOutput(..) => { return true },
		PackageSolvingData::RevokedOutput(..) => { return true },

Anchor-outputs (1/3): Refactoring chain backend to extract PackageTemplate #642

Anchor-outputs (1/3): Refactoring chain backend to extract PackageTemplate #642

Uh oh!

Conversation

ariard commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rational

PR scope

Uh oh!

Uh oh!

Uh oh!

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!

devrandom left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devrandom Nov 8, 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!

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!

ariard commented Nov 10, 2020

Uh oh!

devrandom commented Nov 10, 2020

Uh oh!

ariard commented Mar 9, 2021

Uh oh!

TheBlueMatt left a comment

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!

Choose a reason for hiding this comment

Uh oh!

ariard Apr 18, 2021 • 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!

TheBlueMatt commented Mar 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Jun 19, 2020 •

edited

Loading

devrandom Nov 8, 2020 •

edited

Loading

ariard Apr 18, 2021 •

edited

Loading

ariard commented Apr 19, 2021 •

edited

Loading

codecov bot commented May 11, 2021 •

edited

Loading