Add the "planning" module #481

afilini · 2022-10-25T15:03:35Z

This PR modifies the satisfaction logic so that instead of building a full witness it builds "witness templates", essentially containing placeholders instead of the actual signatures/preimages.

Once you have a "witness template" you can then analyze it (figure out the precise weight) and interactively (or in single shot) try to construct an actual witness by replacing the placeholders with actual values.

We introduce the AssetProvider trait which somewhat mirrors the Satisfier trait, but only returns whether something is available or not. For convenience the AssetProvider trait is automatically implemented for every Satisfier through a wrapping structure.

Finally, we also introduce the Assets structure which is a builder-like struct that implements the AssetProvider trait and can be used to easily tell miniscript which keys and preimages are available, or how long one is willing to wait for timelocks to expire. Using this, one can easily figure out, before creating a transaction, how much the witness is going to weight and what nLockTime/nSequence should be set in the transaction assuming those assets will be available later for signing.

The commits are co-authored by @danielabrozzoni since we've both worked together on this on and off for the past couple of weeks.

apoelstra · 2022-10-26T12:49:26Z

Heads up that in the future we may drop PackedLockTime rust-bitcoin/rust-bitcoin#1330 ... I think you should create a different newtype which implements Ord and calls unreachable!() rather than doing the wrong thing in the case that the types don't match.

src/descriptor/mod.rs

apoelstra · 2022-10-26T16:38:52Z

src/miniscript/satisfy.rs

+    /// Public key and its size
+    Pubkey(Pk, usize),
+    /// Public key hash and its size
+    PubkeyHash(hash160::Hash, usize),


In dc0b280:

This comment should be updated to clarify that it's the size of the hashed pubkey, not the size of the hash

As of 4849893 this is resolved.

apoelstra · 2022-10-26T16:59:31Z

Done reviewing 0f90fb9. This looks amazing! My comments are mostly nits.

afilini · 2022-10-26T18:41:39Z

I remember you suggested on IRC that we also add some kind of API to mark entire branches as "available", instead of marking the individual items. I think this would also be useful in another case, which isn't (and can't, I guess) be covered by this "Assets" approach: setting a "minimum" locktime, i.e. asking miniscript to find a way to spend that includes a timelock of at least a given value. This could be useful if you know you have a branch with a timelock and one without and you want to force the use of the timelock, even if it's not the cheaper way to spend.

My idea for this feature is to figure out a way to provide a custom closure to the satisfy_helper function: right now it takes a "minimum" fn which finds the cheapest solution of a thresh. My idea is that you could swap this with your own custom function that can analyze the sub-templates and pick the ones you want.

But considering this PR is already pretty large (and I still need a bit of time to experiment with that) I figured I could submit that with a follow-up PR.

src/plan.rs

apoelstra · 2022-11-17T13:51:10Z

src/descriptor/key.rs

@@ -485,6 +485,30 @@ impl DescriptorPublicKey {
        DefiniteDescriptorKey::new(definite)
            .expect("The key should not contain any wildcards at this point")
    }
+
+    /// Whether this key matches a [`DefiniteDescriptorKey`]


In e16e7dd:

This matches function should be private and have a short wrapper

pub fn matches<D: AsRef<DescriptorPublicKey>>(&self, key: D) { self.real_matches(key.as_ref()) }

And then in your matches method (now real_matches) take a &DescriptorPublicKey directly rather than writing &definite_key.0.

This depends on #492 BTW

I also think that ths matches name should be different because I continue to think that this "truncate the path and check for a match" logic is confusing. Maybe it should be called matches_prefix or something?

I'd be willing to move this to a followup PR because I think this is dominating discussion here, and this PR is actually much much larger than this method :). I'm also musing about whether DefiniteDescriptorKey should have say, an Option<usize> which indicates which index it came from (if any) to assist with what you're trying to do.

Sorry I made a mistake in the rebase and forgot to squash commits: I've renamed it to is_parent because I realized there was already another matches function which implements a similar logic but looks at a bip32::KeySource instead of an actual key.

I don't really like the new name either, but I couldn't think of a different term for a key that is a derivation step before another one.

I can definitely wait for #492, I'm thinking about a few more API improvements that I was planning to add in a separate PR, but if we want to hold this for a few more days I could manage to squeeze them in as well.

The new name is better but I'd still like to revisit this :) BTW I think the code should also have a check that the final (truncated) step in the derivation path is actually a wildcard.

Not clear to me if this is resolved, still to be resolved, or to be left for another PR?

I think this is resolved in #592

ba6c8f2 in #592 removes the is_parent (previously called matches) method in favor of is_key_direct_child_of. Since the new method uses DerivationPath I can't check if the last step is a wildcard, and this makes the method definition a bit weirder, but I had to make it that way in order to support KeySource in Asset.

apoelstra

ACK 632a365

sanket1729 · 2022-11-17T14:45:38Z

I am also interested in reviewing this. Give me a couple more days.

apoelstra

ACK c3ba4d3

LLFourn

This is a really nice design. It fits in great with the existing satisifer API well (with one caveat about the merkle root for tap key spends).

I think the biggest improvement I'd like to see to the general idea is to make the Plan self-contained. I think the plan should contain everything you need to analyze and execute spending from that input i.e. you don't need to keep around the definite descriptor once you've settled on a plan. I think the only thing that is missing in terms of data is the merkle root for tap key spends. In terms of functionality the plan would need to be able to populate an psbt::Input with all the fields necessary to complete the witness for the plan. That way you can use the plan instead of just blindly setting all the fields with update_input_with_descriptor for all possible spending paths. I think this could come in a follow up PR.

I was wondering if it were possible to get rid of the two types of witness templates and unify them. Rather than trying to describe the approach I just attempted to implement it myself:

LLFourn@7e0df6a

The basic strategy is to have a Option in each placeholder variant that can be satisfied where you can put the satisfaction data (i.e. signature). Once the signature (or whatever) is there then it will no longer show up when you ask the template what is missing. Once all these Options have been set to Some(_) you can finally get the witness. This allows you avoid the PartialSatisfaction type. M I think it might also allow you to get rid of the WitnessTemplate type entirely in the public API. ore importantly it lets you satisfy the witness template without using a Satisfier impl easily. It probably needs some more work to fully explore this API change. Let me know what you think and whether I should open up a PR.

LLFourn · 2022-11-18T11:25:13Z

src/miniscript/satisfy.rs

+    /// ECDSA signature given the pubkey hash
+    EcdsaSigHash(hash160::Hash),
+    /// Schnorr signature
+    SchnorrSig(Pk, Option<TapLeafHash>),


This tells you the Pk and TapLeafHah if you are signing a leaf but you are signing using internal key it doesn't tell you the merkle root. Wouldn't it make sense to have an enum here with the kind of SchnorrSig you are producing and the artifact you need in each case.

Another related point is that putting the artifacts in here means they are not being held as invariant. An invalid state is representable here where in the list of Placeholders the value of Option<TapLeafHash> is not the same for all signature items in the list. It feels like this belongs in a context type. It might even be possible to merge EcdsaSig and SchnorrSig into the same Signature variant (and the Sighash thing too) then where the type of signature can be gotten from the context.

nit: The SigHash in EcdsaSigHash is a bit confusing since "sighash" (and even "ecdsa sighash") is an existing but unrelated concept. Perhaps rename this to EcsaPubkeyHashSig.

I think I agree with adding the merkle root and using types to ensure a consistent state, but it's not clear to me what you have in mind when you suggest moving the TapLeafHash into a context type.

Do you think we should add the context as part of the Placeholder type itself? Like, Placeholder<Pk, SigCtx>. I think the problem in that case is that we'd need different SigCtx types for different TapLeafHash values, if that makes sense. And I can't think of a way to do it in Rust, maybe with const generics? But they are probably too new for miniscript's MSRV.

The other alternative would be to store the context alongside the vec of Placeholder, in the WitnessTemplate, and then figure out a way to easily access it when you are iterating the list of required signatures.

The other alternative would be to store the context alongside the vec of Placeholder, in the WitnessTemplate, and then figure out a way to easily access it when you are iterating the list of required signatures.

Yeah I was thinking something more like this. A general context object which has what kind of signatures you are producing (ecdsa, tap key, tap script) and any dependencies needed to sign (merkle root, tap leaf hash). That would clean things up a bit i think.

LLFourn · 2022-11-18T11:53:44Z

src/miniscript/satisfy.rs

+    stack: Vec<I>,
+}
+
+impl<I> AsRef<[I]> for WitnessTemplate<I> {


My recently developed understanding of AsRef tells me that it should be used if we intend for functions that take AsRef of [Placeholder<Pk>] or something like that. This would allow us to pass in a WitnessTemplate<Placeholder> or a &[Placeholder<Pl>] etc. But I don't think that's what we want here? I think there should just be a accessor for this like .placeholders(&self) -> &[I].

LLFourn · 2022-11-19T04:36:00Z

src/util.rs

+            Placeholder::PubkeyHash(_, size) => *size,
+            Placeholder::Pubkey(_, size) => *size,
+            Placeholder::EcdsaSigPk(_) | Placeholder::EcdsaSigHash(_) => 73,
+            Placeholder::SchnorrSig(_, _) | Placeholder::SchnorrSigHash(_, _) => 66,


idea: What if "asset provider" told you about the sighash it was willing to use on this input and it was stored with the SchnorrSig. Then we could get the right answer here (64) most of the time.

Yes, I think it can be done. We'll then store the sighash in the RequiredSig enum so that you know how to produce the signature so that it matches the expected size.

Just to clarify: we use 66 here because it counts the OP_PUSH byte as well, but with sighash_default it can drop to 65.

Hmm why include the varint for the size here (I assume that's what you mean by OP_PUSH)? the element size itself seems more useful because you can easily go from elem_size -> varint_len + elem_size but the other way around is harder (impossible?). Also it doesn't look like for TapControlBlock and TapScript you are including the varint_len.

LLFourn · 2022-11-19T04:53:46Z

src/plan.rs

+    }
+
+    /// Lookup the tap key spend sig
+    fn lookup_tap_key_spend_sig(&self, _: &Pk) -> bool {


Ahh now I can see why you didn't provide the merkle root. The satisifier API doesn't give you it. I strongly think it should. In general would allow you to implement Satisfier for a private key + info about tx`. Dunno how big a change this would be and whether you can fit it in here.

LLFourn · 2022-11-19T04:55:33Z

src/descriptor/segwitv0.rs

@@ -393,6 +422,37 @@ impl<Pk: MiniscriptKey + ToPublicKey> Wpkh<Pk> {
    }
 }

+impl Wpkh<DefiniteDescriptorKey> {
+    /// Returns a plan if the provided assets are sufficient to produce a non-malleable satisfaction
+    pub fn get_plan<P>(&self, provider: &P) -> Option<Plan>


I think plan would be preferable to get_plan. The method is making a plan so that's a better verb than "get".

Agreed. I think this naming is an improvement. This PR is heavy in new data structures/changes. Can also defer nits to the future, as @afilini thinks.

As of 4849893 this is resolved.

sanket1729

First off, thanks a lot. Asset Planning is one of the things that we are lacking. It really sucked to use the max_satisfaction APIs for change estimation.

I did a review of the first two commits. Have some questions before I proceed to more review.

pub struct Assets {
    raw_keys: HashSet<DescriptorPublicKey>, // The keys that we cannot sign with, but used for pkh -> pk mapping
    avail_keys: HashSet<DescriptorPublicKey>, // The keys that we can sign with
    sighash_type: Option<PsbtSigHashType>, // The sighash e that we want to use while signing
    .. // All other fields for hashes stay the same.
    }

We can also skip the raw_keys part as that is still in discussion. We can then the methods in AssetProvider to better match the API. I believe the above matches the most realistic user scenario: "Given keys, hash locks, and timelocks, construct a template before actually signing". We can then apply satisfiers on the placeholder witness to get the complete witness.
Just to make sure we are on the same page, I think the workflow using planner might look like

Construct Assets and obtain a plan. Retry with other Assets if Plan creation fails. We could have multiple assets hot_assets, countersparty_assets, or cold_assets. We can change our plan based on the availability of different assets/counter parties.
Obtain the satisfaction weight for the given plan and compute the change output using the fees.
Create an unsigned psbt with required nSequence/nLockTime.
Update using update_utxo_with_descriptor. We can change this function to also take in a plan argument. Or embed the descriptor in the plan. Not really sure.
Using the witness template obtained from the plan to get the Template::required_signatures.
Sign with the required keys and add the required preimages. If we face any error here, perhaps the counterparty did not provide signatures. go back to step 1.
Finalize psbt.

Here, we do not require signatures in assets, and that is probably was @LLFourn is suggesting.

sanket1729 · 2022-11-20T21:05:22Z

src/miniscript/satisfy.rs

@@ -706,6 +706,12 @@ pub struct Satisfaction {
    /// Whether or not this (dis)satisfaction has a signature somewhere
    /// in it
    pub has_sig: bool,
+    // We use PackedLockTime here as we need to compare timelocks using Ord. This is safe,
+    // as miniscript checks for us beforehand that the timelocks are of the same type.


Miniscript does not ensure this. It is possible to have miniscripts like and(older(TIME_LOCK), older(HEIGHT_LOCK)). Those miniscript are not sane, but we should still support satisfaction for those.

The correct thing to do would be to have Satisfaction::Impossible whenever we combine two timelocks of different types.

Thanks for pointing this out, I wasn't aware of this!

Oof, I read this comment and totally missed that it wasn't true!

Looking back at this, the comment is clearly wrong but I still think the code is fine, or at least it is not introducing any new issues (more about this below): that field is populated either by Terminal::After or by combining satisfactions using cmp::max. Terminal::After uses Satisfier::check_after, which should return true only for one of the two units. Although this doesn't seem to be explicitly documented, it is implied by the way the current satisfier is implemented:

rust-miniscript/src/miniscript/satisfy.rs

Lines 1001 to 1004 in a7bfff4

Terminal::After(t) => Satisfaction {

stack: if stfr.check_after(t.into()) {

Witness::empty()

} else if root_has_sig {

Like with the current implementation, in this PR we only ever create new satisfactions containing a timelock when the unit is "compatible" with the satisfier, and then we just combine those by getting the max value. Which should be safe.

Now, some could argue that it should be possible to create a satisfier that accepts both timelock units. For example (After::Blocks(x), After::Seconds(y)) is a satisfier that will return true on check_after queries for all blocks timelocks lower than x and all seconds timelocks lower than y. The rationale for this would be that a user may be willing to wait either a specific block height or a timestamp, so it may provide a satisfier that accepts both. This could cause miniscript to mix timelocks, even with the current implementation, and there are two possible fixes:

Make it clear that Satisfier::check_after should never return true for both timelock units and change the macro that implements tuple satisfiers to somehow enforce this (see current implementation below). Obviously we don't have control over custom implementations, so it would still be possible to hit this bug with a custom satisfier

rust-miniscript/src/miniscript/satisfy.rs

Lines 507 to 515 in a7bfff4

fn check_after(&self, n: LockTime) -> bool {

let &($(ref $ty,)*) = self;

$(

if $ty.check_after(n) {

return true;

}

)*

false

}

Implement checks when merging timelocks: this PR would really help implementing this, because it introduces explicit timelocks in the Satisfaction struct which make it easy to compare them when merging. With the current satisfier once a timelock is "accepted" the corresponding Satisfaction doesn't contain any information about its value, which makes it impossible to compare with another Satisfaction. However, this introduces a different problem: it breaks the guarantee that miniscript finds the cheapest/optimal solution during satisfaction, at least with the current "greedy" algorithm. Essentially, at some point while traversing the tree we could have to make a choice between satisfaction A that includes a block-based timelock and B that includes a time-based timelocks. Choosing the cheapest one at this level, doesn't guarantee we will find the optimal solution overall, because it may cause us to discard another part of the tree because the timelocks are incompatible. So I think introducing this "compatibility check" is inherently incompatible with the current greedy algorithm (I'm sure there's a specific term for this kind of problems but I don't remember it :) )

I think I would personally go with the first option, to avoid having to make major changes to the satisfaction algorithm, essentially improving the documentation and changing the macro to at least ensure you can't hit the bug without explicitly implementing a misbehaving satisfier.

@afilini, thanks for the detailed response. I agree with your assessment of going with the first solution in interest of complexity involved in the second solution. I can think more about it and work on a follow-up. There is no need to block this particular PR based on insane miniscript possibilities.

Just a small note, we already give up a lot of guarantees in the greedy algorithm when it comes to certain edge cases. For example, we choose the cheapest solution regardless of the number of stack elements. It is possible that we reach an ImpossibleSatisfaction when we exceed more than 100 stack elements in Segwitv0 miniscript. Choosing a more expensive alternative that might have less stack elements could have been better.

While parsing miniscripts using regular APIs, FormStr, parse, compile we guarantee that these manuscripts cannot be parsed. Users have to explicitly opt into these by calling from_str_ext(ExtParams) , parse_ext... The satisfaction algorithm might not produce a solution for these miniscripts even if it exists. More on these https://github.com/rust-bitcoin/rust-miniscript/blob/master/doc/resource_limitations.md

Looks like this is resolved but the comment is still in 4849893 albeit with the PackedLockTime changed to AbsLockTime - better to remove the comment,right? @sanket1729 can you confirm please.

Removed in f3a8566 in #592

sanket1729 · 2022-11-20T22:46:55Z

src/plan.rs

+
+/// Trait describing a present/missing lookup table for constructing witness templates
+///
+/// This trait mirrors the [`Satisfier`] trait with the difference that instad of returning the


/s/instad/instead

sanket1729 · 2022-11-20T22:52:29Z

src/plan.rs

+        false
+    }
+
+    /// Given a raw `Pkh`, lookup corresponding `Pk`. If present, return its lenght.


Here and below: s/lenght/length .

src/plan.rs

sanket1729 · 2022-11-21T08:08:06Z

src/plan.rs

+    keys: HashMap<hash160::Hash, DescriptorPublicKey>,
+    tap_key_spend_sig: Option<bitcoin::SchnorrSig>,
+    ecdsa_signatures: HashMap<DescriptorPublicKey, (bitcoin::EcdsaSig, usize)>,
+    schnorr_signatures: HashMap<(DescriptorPublicKey, TapLeafHash), (bitcoin::SchnorrSig, usize)>,


Why do we need the signatures here? Will post a detailed reply in main conversation.

The idea is that in multi-party protocols you can use signatures made by others as "assets" themselves. In that case you can't "cheat" by saying you have the key, because if you only have one signature you can either take it or it's not useful to you at all. When you say you have a key the planning code assumes you can sign anything you want with that key, which is a bit different.

Thinking about it now, I was planning to implement IntoAssets for a psbt::Input but I think I ended up forgetting about it, so I'll try to add that as well.

I thought the purpose of planning was to decide which spend to use before signing. In which case, why do we need signatures? I think having signatures in assets would be confusing, curious what @apoelstra thinks about this.

In that case you can't "cheat" by saying you have the key

You would still need to assume that the other party will sign with a key because you calculated the change output based on it. So, they can still refuse to sign and cheat.

Perhaps, we have some misunderstanding about what Plan is supposed to do.

I think @afilini is thinking of the scenario of where you get a request to sign a TX input and a few signatures under other keys. The TX is already finalized at this point. To figure out what you're meant to be contributing for each input you construct a plan from the descriptor + existing signatures and see what's left to do. Of course, assuming you're using PBSTs you could just blindly follow what's missing from the PSBTs but this allows some analysis before doing so e.g. construct a plan with only the signatures + what you have locally and see if it's possible for you alone to finish the PSBT before actually engaging signing devices.

I think it would be a good idea to get rid of the signatures themselves from this struct though. If they've already signed then those keys can be treated as an asset. Just map those signatures to their asset keys but there's no need to actually represent them like that in the "assets" I think.

Oh right after writing this I realized that it's possible the signatures are only valid for one tapleaf but not the other. What if we generalized this to conditioning public keys that can only sign on particular tap leaves? E.g. maybe I have a hardware device that will only sign on this tapleaf one normally but might sign on this tap leaf (or do tap key) if some condition is met. Then existing signatures would map to keys that can only sign on a particular leaf (or as tap key).

What if we generalized this to conditioning public keys that can only sign on particular tap leaves?

Yes, I agree that we should do this. Otherwise I don't really see the point of supporting signatures in lieu of public keys.

As of 4849893 this is resolved.

afilini · 2022-11-21T14:09:01Z

Thanks everyone for the review! I think I agree with most of what you pointed out, so I'll start working in that direction and see what I can come up with.

I think one of the limitations of the current AssetProvider trait is that it must provide essentially a "subset" of the information that the Satisfier does. Because we want to provide a default AssetProvider implementation for every type that also implements Satisfier.

So this complicates things like adding the merkle root in the plan: as Lloyd pointed out it would require changing the Satisfier trait itself and I was trying to avoid that. But if @sanket1729 and @apoelstra are ok with it, I can definitely do it.

@sanket1729 re: your point number 4 in the flow

Update using update_utxo_with_descriptor. We can change this function to also take in a plan argument. Or embed the descriptor in the plan. Not really sure.

Talking with @LLFourn we were thinking about changing the plan API so that the plan itself could be used to update the PSBT. This would allow you to:

Avoid keeping the descriptor around even after you've created a plan
Only add to the PSBT the relevant metadata (which is probably what you had in mind when you suggested to take the plan as an argument)

I really like this idea but I haven't tried implementing it yet, so there could be some things missing in the plan that won't allow me to (fully) update the PSBT. Worst case as you suggested we can embed the full descriptor in the plan, although it seems a bit "hacky" to me.

apoelstra · 2022-11-21T15:28:23Z

I am definitely ok with changing the Satisfier trait to make this PR better.

sanket1729 · 2022-11-21T20:51:54Z

@LLFourn , sorry I also don't follow why having a merkle_root is useful in AssetProvider. It is already a deterministic function of the descriptor. If we carry the descriptor in plan, we don't need any additional information. Plan

What I think of AssetProvider is providing information on what things could be available. These things should in principle only be information about keys, hash locks, and timelocks without any signatures/pre-images.

I think what @afilini and you are mentioning is more along the lines of SigningProvider, information that you might need for signing. I think we can already use Psbts for it.

I think before @afilini puts more time into this PR, we should settle on what Plan should do.

In terms of functionality the plan would need to be able to populate an psbt::Input with all the fields necessary to complete the witness for the plan.

Why put witnesses/Merkle root in Plan instead of adding them to psbt directly? I feel we are duplicating a lot of functionality that is already well-defined in psbts. The only psbts don't tell you is how to construct the initial psbt and which spend paths to use. IMO, this should be the only thing Plan does.

LLFourn · 2022-11-22T03:53:59Z

@LLFourn , sorry I also don't follow why having a merkle_root is useful in AssetProvider. It is already a deterministic function of the descriptor. If we carry the descriptor in plan, we don't need any additional information. Plan

What I think of AssetProvider is providing information on what things could be available. These things should in principle only be information about keys, hash locks, and timelocks without any signatures/pre-images.

Agreed 100%. I was not suggesting the merkle root be in the AssetProvider. Including the desctiptor in the plan would be sufficient but I agree with @afilini that iit seems hacky and error prone.

I think what @afilini and you are mentioning is more along the lines of SigningProvider, information that you might need for signing. I think we can already use Psbts for it.

I think before @afilini puts more time into this PR, we should settle on what Plan should do.

In terms of functionality the plan would need to be able to populate an psbt::Input with all the fields necessary to complete the witness for the plan.

Why put witnesses/Merkle root in Plan instead of adding them to psbt directly? I feel we are duplicating a lot of functionality that is already well-defined in psbts. The only psbts don't tell you is how to construct the initial psbt and which spend paths to use. IMO, this should be the only thing Plan does.

Who's going to add it to the PSBT though? Your proposed structure seems awkward to me. If the plan is going to tell you that you need to put the merkle root in the PSBT but not actually put it there you're going to have to re-fetch it yourself. But the plan has already traversed the descriptor and could easily have just saved the merkle root or tapleaf etc on its way down (it already saves the tapleaf but is missing merkle root).

Construct Assets and obtain a plan. Retry with other Assets if Plan creation fails. We could have multiple assets hot_assets, countersparty_assets, or cold_assets. We can change our plan based on the availability of different assets/counter parties.

Obtain the satisfaction weight for the given plan and compute the change output using the fees.

Create an unsigned psbt with required nSequence/nLockTime.

Update using update_utxo_with_descriptor. We can change this function to also take in a plan argument. Or embed the descriptor in the plan. Not really sure.

Using the witness template obtained from the plan to get the Template::required_signatures.

If we are updating the PSBT with the plan then isn't the required signatures implied by whatever fields that were set in the PSBT?

Sign with the required keys and add the required preimages. If we face any error here, perhaps the counterparty did not provide signatures. go back to step 1.

Finalize psbt.

Overall I think we are all on the same page here with some caveats about specifics in the API (e.g. I think there should just be Plan::update_psbt(input: &mut psbt::Input) without needing the original descriptor). In terms of future vision I think we probably differ in that I would like the above workflow to be possible without using PSBTs one day but I don't think that's super necessary right now.

Actually, opposite to my initial intuitions I'd say the way forward for this might be to go "all in" on PSBTs and reduce new API surface area we're adding here so:

Don't have required_signatures as they are implied from the fields that were populated in the PSBT by the plan.
Make Placeholder type private and all the fields of Plan private with read only accessors.
Merge witness template and plan so there's not need for WitnessTemplate:

#[derive(Debug, Clone)]
pub struct Plan {
    /// This plan's witness template
    pub template: Vec<Placeholder<DefiniteDescriptorKey>>,
    /// The absolute timelock this plan uses
    pub absolute_timelock: Option<LockTime>,
    /// The relative timelock this plan uses
    pub relative_timelock: Option<Sequence>,
    pub(crate) desc_type: DescriptorType,
}

Remove "interactive" satisfaction stuff. If we want the "interactive satisfaction" like API then I suggest going with the approach here: LLFourn@7e0df6a but it can be added later.
Add Plan::update_psbt(input: &mut psbt::Input).

Sorry for the "backflip" @afilini. I am somewhat torn by this recommendation since iterating over the placeholders and interactively satisfying them is one of the coolest things you can do with the plan but I think if we add PSBT input population logic we won't need it for the workflow @sanket1729 mentions above. I would love for it to be made public later once it's a little more refined and to eventually decouple it from PSBTs. Do you think this approach is viable @afilini?

afilini · 2022-12-03T07:39:05Z

Sorry for the huge delay, I've been travelling and I wanted to settle down a bit before getting back to this.

I think I agree that if we have to make a choice between having everything inside the plan or everything inside the PSBT, then the PSBT is the most reasonable choice.

I'll try to implement what you suggested @LLFourn, the only thing I'm not sure about is removing the WitnessTemplate, because I seem to remember it was a useful abstraction in a few places. But I'll try and see what happens.

I would love for it to be made public later once it's a little more refined and to eventually decouple it from PSBTs. Do you think this approach is viable @afilini?

Yes, I think it should be possible. I think it's better to initially focus on just satisfying everything in one go from a PSBT, which is probably what most people are going to use anyway. The current interactive satisfaction part was already a bit awkard, so I'm happy to remove it and work on it separately with more thought.

Harshil-Jani · 2023-06-30T12:13:32Z

src/miniscript/satisfy.rs

+    /// Replaces the placeholders with the information given by the satisfier
+    pub fn satisfy_self<Sat: Satisfier<Pk>>(&self, sat: &Sat) -> Option<Vec<u8>> {
+        match self {
+            Placeholder::Pubkey(pk, _) => Some(pk.to_public_key().to_bytes()),


This would work in case of ECDSA Sig. But with the Schnorr sig, It is not fulfilling.
Placeholder::Pubkey(pk, _). Here the second argument is length so we can have a length instead of _ and check if it was 33 or something else. If not 33 then do Some(pk.to_x_only_pubkey().serialize().to_vec())

Thanks! I think it should be: if the size is 33, then I have to use to_x_only_pubkey, otherwise I can use to_public_key. This is because we push Ctx::pk_len, which is 33 for Tap. Does this make sense?

Placeholder::Pubkey(pk, size) => { if *size == 33 { Some(pk.to_x_only_pubkey().serialize().to_vec()) } else { Some(pk.to_public_key().to_bytes()) } }

Yes Thanks. This looks good.

tcharding · 2023-07-27T02:32:22Z

src/descriptor/mod.rs

@@ -511,6 +512,60 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
    }
 }

+impl Descriptor<DefiniteDescriptorKey> {
+    /// Returns a plan if the provided assets are sufficient to produce a non-malleable satisfaction
+    pub fn get_plan<P>(self, provider: &P) -> Option<Plan>


Does it matter that this consumes self and if None is returned we have no way to get the descriptor back?

+1 on not consuming self here.

Oh right! But then it means that I need to clone self when returning the Plan, as the Plan owns the descriptor. Do you think it's ok to have this "hidden" clone?

--- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -512,7 +512,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> { impl Descriptor<DefiniteDescriptorKey> { /// Returns a plan if the provided assets are sufficient to produce a non-malleable satisfaction - pub fn get_plan<P>(self, provider: &P) -> Option<Plan> + pub fn get_plan<P>(&self, provider: &P) -> Option<Plan> where P: AssetProvider<DefiniteDescriptorKey>, { @@ -527,7 +527,7 @@ impl Descriptor<DefiniteDescriptorKey> { if let satisfy::Witness::Stack(stack) = satisfaction.stack { Some(Plan { - descriptor: self, + descriptor: self.clone(), template: stack, absolute_timelock: satisfaction.absolute_timelock.map(Into::into), relative_timelock: satisfaction.relative_timelock, @@ -538,7 +538,7 @@ impl Descriptor<DefiniteDescriptorKey> { } /// Returns a plan if the provided assets are sufficient to produce a malleable satisfaction - pub fn get_plan_mall<P>(self, provider: &P) -> Option<Plan> + pub fn get_plan_mall<P>(&self, provider: &P) -> Option<Plan> where P: AssetProvider<DefiniteDescriptorKey>, { @@ -553,7 +553,7 @@ impl Descriptor<DefiniteDescriptorKey> { if let satisfy::Witness::Stack(stack) = satisfaction.stack { Some(Plan { - descriptor: self, + descriptor: self.clone(), template: stack, absolute_timelock: satisfaction.absolute_timelock.map(Into::into), relative_timelock: satisfaction.relative_timelock,

@danielabrozzoni, I think they mean pub fn get_plan(self, provider: &P) -> Result<Plan, Descriptor>. That is we don't lose the descriptor in case of error .

tcharding · 2023-07-27T02:35:41Z

What needs doing to push this forward? I'm not able to provide much value just reading the diff but I've got clock cycles available if there is anything else I can do?

tcharding · 2023-07-27T03:48:14Z

src/plan.rs

+//
+// You should have received a copy of the CC0 Public Domain Dedication
+// along with this software.
+// If not, see <http://creativecommons.org/publicdomain/zero/1.0/>.


We have SPDX identifiers now, this can all be removed and just plain old:

// SPDX-License-Identifier: CC0-1.0

As of 4849893 this is still TODO.

Solved in 9a88807 in #592

The Satisfaction struct now contains `relative_timelock` and `absolute_timelock`, which represent the needed timelocks for that particular spending path. This is useful for the plan module. Co-authored-by: Daniela Brozzoni <[email protected]>

danielabrozzoni · 2023-07-31T17:09:43Z

@danielabrozzoni are you aware of the new BITCOIND_EXE environment variable? You can set this to a bitcoind outside of the crate itself and then tests will work on NixOS.

Oh yeah, I didn't know, thanks! Putting it in the README might be helpful, even though I might continue using patchelf to patch the bitcoind binary in order to make sure I'm using the same bitcoind version as the CI (and everyone else)

What needs doing to push this forward? I'm not able to provide much value just reading the diff but I've got clock cycles available if there is anything else I can do?

I'm currently rebasing this one on master, I'm having a few failing tests but I think I can figure it out :) After that, it should be ready for another round of reviews!

danielabrozzoni · 2023-08-02T18:00:30Z

I just pushed a rebase on master, now I'm looking through the review comments and fixing the ones that I missed previously

Add a `plan` module that contains utilities to calculate the cheapest spending path given an AssetProvider (that could keys, preimages, or timelocks). Adds a `get_plan` method on the various descriptor types. Co-authored-by: Daniela Brozzoni <[email protected]>

Co-authored-by: Alekos Filini <[email protected]>

sanket1729 · 2023-08-04T08:21:03Z

According to my supercial review. Only the pubkey size check is the only relevant comment unaddressed. Everything else looks good to me

danielabrozzoni · 2023-08-04T15:28:44Z

I just pushed the fix for the pubkey length, there are still a couple of comments I need to address:

Not being the PR author I can't resolve conversations, and given the length of the discussion, I might be missing something important

sanket1729 · 2023-08-09T04:46:00Z

@danielabrozzoni, would it help to split this PR and make progress on another fresh if this is annoying to load in Github? I would prefer listing out all the unresolved issues and addressing all of them in a separate PR. Github is unfriendly once it gets into hundreds of comments

danielabrozzoni · 2023-08-09T13:31:34Z

@sanket1729 yes, continuing in a different PR would be of great help!

tcharding

I went through all the comments and attempted to convince myself if they were resolved or not. I added "As of 4849893 this is still TODO" so that one can easily grep and I flagged things that were not clear to me so someone else can say if they are done or not. I also checked most of the "outdated" stuff too but did not explicitly comment on each one.

Hope this helps

tcharding · 2023-07-27T04:17:12Z

src/plan.rs

+        None
+    }
+
+    /// Given a SHA256 hash, look up its preimage


These docs are not valid, right? They are copied from Satisfy but the return value of the functions is different.

The docs on AssetProvider need fixing up a bit, seems they are copied over from Satisfier. I had a go but got confused about what exactly is return by each function. Some return bool or size but others still return the provided thing.

Solved in 14833b0 in #592

(Even tho the docs are still a bit confusing, I think this is already an improvement)

tcharding · 2023-08-10T03:20:29Z

src/miniscript/satisfy.rs

+    /// Public key and its size
+    Pubkey(Pk, usize),
+    /// Public key hash and its size
+    PubkeyHash(hash160::Hash, usize),


As of 4849893 this is resolved.

tcharding · 2023-08-10T03:28:57Z

src/descriptor/key.rs

@@ -485,6 +485,30 @@ impl DescriptorPublicKey {
        DefiniteDescriptorKey::new(definite)
            .expect("The key should not contain any wildcards at this point")
    }
+
+    /// Whether this key matches a [`DefiniteDescriptorKey`]


Not clear to me if this is resolved, still to be resolved, or to be left for another PR?

tcharding · 2023-08-10T03:59:57Z

src/descriptor/segwitv0.rs

@@ -393,6 +422,37 @@ impl<Pk: MiniscriptKey + ToPublicKey> Wpkh<Pk> {
    }
 }

+impl Wpkh<DefiniteDescriptorKey> {
+    /// Returns a plan if the provided assets are sufficient to produce a non-malleable satisfaction
+    pub fn get_plan<P>(&self, provider: &P) -> Option<Plan>


As of 4849893 this is resolved.

tcharding · 2023-08-10T04:10:53Z

src/miniscript/satisfy.rs

@@ -706,6 +706,12 @@ pub struct Satisfaction {
    /// Whether or not this (dis)satisfaction has a signature somewhere
    /// in it
    pub has_sig: bool,
+    // We use PackedLockTime here as we need to compare timelocks using Ord. This is safe,
+    // as miniscript checks for us beforehand that the timelocks are of the same type.


Looks like this is resolved but the comment is still in 4849893 albeit with the PackedLockTime changed to AbsLockTime - better to remove the comment,right? @sanket1729 can you confirm please.

src/plan.rs

tcharding · 2023-08-10T04:36:36Z

src/plan.rs

+//
+// You should have received a copy of the CC0 Public Domain Dedication
+// along with this software.
+// If not, see <http://creativecommons.org/publicdomain/zero/1.0/>.


As of 4849893 this is still TODO.

tcharding · 2023-08-10T04:39:30Z

src/plan.rs

+//! doing coin selection. Furthermore it provides which subset of those keys and hash pre-images you
+//! will actually need as well as what locktime or sequence number you need to set.
+//!
+//! Once you've obstained signatures, hash pre-images etc required by the plan, it can create a


Suggested change

//! Once you've obstained signatures, hash pre-images etc required by the plan, it can create a

//! Once you've obtained signatures, hash pre-images etc required by the plan, it can create a

Solved in 767e4f6 in #592

tcharding · 2023-08-10T04:40:21Z

src/plan.rs

+/// This trait mirrors the [`Satisfier`] trait with the difference that instead of returning the
+/// item if it's present, it only returns a boolean to indicate its presence.


Not quite right anymore.

Solved in 14833b0 in #592

tcharding · 2023-08-10T04:40:41Z

src/plan.rs

+/// This trait is automatically implemented for every type that is also a satisfier, and simply
+/// proxies the queries to the satisfier and returns whether an item is available or not.
+///
+/// All the methods have a default implementation that returns `false`.


Not correct at the moment.

Solved in 14833b0 in #592

sanket1729 · 2023-08-15T07:02:34Z

Thanks for the help @tcharding. @danielabrozzoni , can you raise a follow up PR that builds on top of this PR and addresses the issues highlighted? If there are no major issues there, we can merge this one first and then follow up again.

I would like to have the followup PR in a mergeable state before we merge this one.

danielabrozzoni

Thanks for the help @sanket1729 and @tcharding! I just opened #592 that builds on top of this one. I chose to push the fixes as separate commits instead of squashing into the main "add the planning module" commit in order to make review easier, but I can definitely squash if you prefer.

danielabrozzoni · 2023-08-18T12:08:07Z

src/miniscript/satisfy.rs

+
+#[derive(Debug, Clone, PartialEq, Eq)]
+/// Placeholder for some data in a [`Plan`]
+pub enum Placeholder<Pk: MiniscriptKey> {


In 1bb08f2 in #592 I'm making the template pub(crate) and adding a function to get a reference to it, so that it can be read but not modified from the outside

danielabrozzoni · 2023-08-18T12:09:00Z

src/miniscript/satisfy.rs

@@ -706,6 +706,12 @@ pub struct Satisfaction {
    /// Whether or not this (dis)satisfaction has a signature somewhere
    /// in it
    pub has_sig: bool,
+    // We use PackedLockTime here as we need to compare timelocks using Ord. This is safe,
+    // as miniscript checks for us beforehand that the timelocks are of the same type.


Removed in f3a8566 in #592

danielabrozzoni · 2023-08-18T12:09:34Z

src/plan.rs

+//
+// You should have received a copy of the CC0 Public Domain Dedication
+// along with this software.
+// If not, see <http://creativecommons.org/publicdomain/zero/1.0/>.


Solved in 9a88807 in #592

danielabrozzoni · 2023-08-18T12:09:54Z

src/plan.rs

+//! doing coin selection. Furthermore it provides which subset of those keys and hash pre-images you
+//! will actually need as well as what locktime or sequence number you need to set.
+//!
+//! Once you've obstained signatures, hash pre-images etc required by the plan, it can create a


Solved in 767e4f6 in #592

danielabrozzoni · 2023-08-18T12:10:18Z

src/plan.rs

+/// This trait mirrors the [`Satisfier`] trait with the difference that instead of returning the
+/// item if it's present, it only returns a boolean to indicate its presence.


Solved in 14833b0 in #592

danielabrozzoni · 2023-08-18T12:10:28Z

src/plan.rs

+/// This trait is automatically implemented for every type that is also a satisfier, and simply
+/// proxies the queries to the satisfier and returns whether an item is available or not.
+///
+/// All the methods have a default implementation that returns `false`.


Solved in 14833b0 in #592

danielabrozzoni · 2023-08-18T12:11:08Z

src/plan.rs

+        None
+    }
+
+    /// Given a SHA256 hash, look up its preimage


Solved in 14833b0 in #592

(Even tho the docs are still a bit confusing, I think this is already an improvement)

danielabrozzoni · 2023-08-18T12:12:24Z

src/descriptor/key.rs

@@ -485,6 +485,30 @@ impl DescriptorPublicKey {
        DefiniteDescriptorKey::new(definite)
            .expect("The key should not contain any wildcards at this point")
    }
+
+    /// Whether this key matches a [`DefiniteDescriptorKey`]


I think this is resolved in #592

ba6c8f2 in #592 removes the is_parent (previously called matches) method in favor of is_key_direct_child_of. Since the new method uses DerivationPath I can't check if the last step is a wildcard, and this makes the method definition a bit weirder, but I had to make it that way in order to support KeySource in Asset.

danielabrozzoni · 2023-08-18T12:14:59Z

src/descriptor/key.rs

+
+    /// Whether this key is the "parent" of a [`DefiniteDescriptorKey`]
+    ///
+    /// The key is considered "parent" if it represents the non-derived version of a definite key,


6151612 in #592 removes the method altogether

danielabrozzoni · 2023-08-18T12:15:12Z

src/plan.rs

+/// The Assets we can use to satisfy a particular spending path
+#[derive(Debug, Default)]
+pub struct Assets {
+    keys: HashMap<hash160::Hash, (DescriptorPublicKey, CanSign)>,


6151612 in #592 fixes this

a358076 test: absolute/relative timelocks in satisfaction (Daniela Brozzoni) cb9a769 tests: plan capabilities (Daniela Brozzoni) d29c298 Add plan capabilities to miniscript (Alekos Filini) fc20eb0 Fix test_cpp (sanket1729) 448fbd8 Add full_derivation_paths on DescriptorPublicKey (Daniela Brozzoni) 7ca9ba1 Add relative and absolute timelock in Satisfaction (Alekos Filini) Pull request description: This PR builds on top of #481, fixing all the review comments. I didn't squash my last commits on purpose to make review easier, I can squash them before merging if preferred. ACKs for top commit: apoelstra: ACK a358076 sanket1729: ACK a358076 Tree-SHA512: 32e547eedaf56d7ddb9ab8069ab394b655f46f6eae7b971d521befc800abadb785335a84c977875b050bcb202517381aba0fb9d8f2d418cd59a1f87147491d67

sanket1729 · 2023-09-22T02:28:03Z

Merged as a part of #592

afilini force-pushed the planning-module branch 4 times, most recently from a887241 to 0f90fb9 Compare October 25, 2022 22:08

apoelstra reviewed Oct 26, 2022

View reviewed changes

src/descriptor/mod.rs Outdated Show resolved Hide resolved

apoelstra reviewed Oct 26, 2022

View reviewed changes

apoelstra reviewed Oct 27, 2022

View reviewed changes

src/plan.rs Outdated Show resolved Hide resolved

apoelstra mentioned this pull request Oct 27, 2022

We should not be running pkh logic through rawpkh #483

Closed

afilini force-pushed the planning-module branch 3 times, most recently from 42601c4 to c68de8c Compare November 16, 2022 10:30

apoelstra reviewed Nov 17, 2022

View reviewed changes

apoelstra previously approved these changes Nov 17, 2022

View reviewed changes

afilini dismissed apoelstra’s stale review via c3ba4d3 November 17, 2022 15:10

afilini force-pushed the planning-module branch from 632a365 to c3ba4d3 Compare November 17, 2022 15:10

apoelstra previously approved these changes Nov 17, 2022

View reviewed changes

LLFourn reviewed Nov 19, 2022

View reviewed changes

sanket1729 reviewed Nov 21, 2022

View reviewed changes

apoelstra mentioned this pull request Nov 23, 2022

Another attempt at max_satisfaction_weight fixes #476

Merged

afilini dismissed apoelstra’s stale review via 816bf94 December 4, 2022 06:55

LLFourn mentioned this pull request Dec 4, 2022

Using KeyMap and DescriptorSecretKey for signing transactions #495

Open

Harshil-Jani reviewed Jun 30, 2023

View reviewed changes

tcharding reviewed Jul 27, 2023

View reviewed changes

afilini and others added 2 commits July 31, 2023 18:30

Add full_derivation_paths on DescriptorPublicKey

95edba5

danielabrozzoni force-pushed the planning-module branch from 7f1ea18 to 529d05a Compare August 2, 2023 16:17

Harshil-Jani mentioned this pull request Aug 1, 2023

Improving Plan API and adding examples. #559

Open

afilini and others added 4 commits August 3, 2023 15:51

tests: plan capabilities

c434fa7

Co-authored-by: Alekos Filini <[email protected]>

test: absolute/relative timelocks in satisfaction

64cce2e

Co-authored-by: Alekos Filini <[email protected]>

Fix test_cpp

4849893

danielabrozzoni force-pushed the planning-module branch from 529d05a to 4849893 Compare August 4, 2023 15:23

sanket1729 mentioned this pull request Aug 9, 2023

Make common generics more terse #573

Draft

LLFourn mentioned this pull request Aug 9, 2023

Replace bdk_tmp_plan with miniscript planning api bitcoindevkit/bdk#1068

Open

tcharding reviewed Aug 10, 2023

View reviewed changes

danielabrozzoni mentioned this pull request Aug 18, 2023

Add the planning module (updated PR) #592

Merged

danielabrozzoni reviewed Aug 18, 2023

View reviewed changes

sanket1729 closed this Sep 22, 2023

jp1ac4 mentioned this pull request Oct 30, 2024

spend: feerate can be lower than target value wizardsardine/liana#1371

Open

jp1ac4 mentioned this pull request Nov 11, 2024

Planning module: Missing varint length in size calculation of some placeholders #771

Open

	Terminal::After(t) => Satisfaction {
	stack: if stfr.check_after(t.into()) {
	Witness::empty()
	} else if root_has_sig {

	fn check_after(&self, n: LockTime) -> bool {
	let &($(ref $ty,)*) = self;
	$(
	if $ty.check_after(n) {
	return true;
	}
	)*
	false
	}

	//! Once you've obstained signatures, hash pre-images etc required by the plan, it can create a
	//! Once you've obtained signatures, hash pre-images etc required by the plan, it can create a

		/// This trait mirrors the [`Satisfier`] trait with the difference that instead of returning the
		/// item if it's present, it only returns a boolean to indicate its presence.

Add the "planning" module #481

Add the "planning" module #481

Conversation

afilini commented Oct 25, 2022 • edited Loading

apoelstra commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Oct 26, 2022

afilini commented Oct 26, 2022

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

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Nov 17, 2022

apoelstra left a comment

Choose a reason for hiding this comment

LLFourn left a comment • edited Loading

Choose a reason for hiding this comment

LLFourn Nov 18, 2022 • edited Loading

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

sanket1729 left a comment • edited Loading

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

afilini Dec 7, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afilini commented Nov 21, 2022

apoelstra commented Nov 21, 2022

sanket1729 commented Nov 21, 2022

LLFourn commented Nov 22, 2022 • edited Loading

afilini commented Dec 3, 2022

Harshil-Jani Jun 30, 2023 • edited Loading

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

tcharding commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielabrozzoni commented Jul 31, 2023

danielabrozzoni commented Aug 2, 2023

sanket1729 commented Aug 4, 2023 • edited Loading

danielabrozzoni commented Aug 4, 2023

sanket1729 commented Aug 9, 2023

danielabrozzoni commented Aug 9, 2023

tcharding left a comment • edited Loading

Choose a reason for hiding this comment

afilini commented Oct 25, 2022 •

edited

Loading

LLFourn left a comment •

edited

Loading

LLFourn Nov 18, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading

afilini Dec 7, 2022 •

edited

Loading

LLFourn commented Nov 22, 2022 •

edited

Loading

Harshil-Jani Jun 30, 2023 •

edited

Loading

sanket1729 commented Aug 4, 2023 •

edited

Loading

tcharding left a comment •

edited

Loading