--- Day changed Sun Sep 27 2020
01:08 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 260 seconds]     
01:47 -!- jonatack [~jon@37.165.205.15] has joined ##miniscript     
01:48 -!- jonatack [~jon@37.165.205.15] has quit [Client Quit]     
03:28 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 240 seconds]     
03:32 -!- digi_james [sid281632@gateway/web/irccloud.com/x-onkyiigagbyecwyz] has quit [Ping timeout: 256 seconds]     
03:34 -!- digi_james [sid281632@gateway/web/irccloud.com/x-ckyalkebeycczlce] has joined ##miniscript     
06:38 -!- instagibbs [~instagibb@pool-100-15-139-5.washdc.fios.verizon.net] has joined ##miniscript     
06:54 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined ##miniscript     
08:49 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript     
08:52 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Read error: Connection reset by peer]     
09:24 < michaelfolkson> There are a couple of open Miniscript PRs on Core. The test framework PR from James Chiang https://github.com/bitcoin/bitcoin/pull/17975
09:25 < michaelfolkson> And the adding Miniscript support in output descriptors https://github.com/bitcoin/bitcoin/pull/16800
09:26 < michaelfolkson> I'm trying to work out if there is any chance of getting these over the line until sipa is free from Taproot commitments
09:27 < michaelfolkson> Personally I'm on the look out for a 6-12 month project but I don't want to choose something that has no chance of getting over the line. Does anyone else have bandwidth to help on these?
09:28 < michaelfolkson> I guess there is the Blockstream team (andytoshi, sanket1729 etc) and then people working on Miniscript related projects (shesek, jeremyrubin, Alekos etc)
09:28 < michaelfolkson> So I guess the question is to everyone
09:29 < michaelfolkson> Obviously digi_james too who is the author of the Core test framework PR
09:30 < sipa> michaelfolkson: both the c++ code and python code are outdated by now, and will need attention from people familiar with the mechanisms and the changes, i think
09:31 < sipa> sanket1729 has said he's interested in helping move things forward, but has other commitmemts first as well
09:34 < michaelfolkson> So they would need a nonzero amount of attention from both you sipa and from digi_james? (I haven't spoken to digi_james yet about it)
09:34 < michaelfolkson> Maybe if James has some limited time we can get the test framework PR over the line but not the output descriptor support PR over the line
09:35 < sipa> the python code is further behind than the c++ code (and afaik has unaddressed review comments)
09:35 < sipa> but i'm not familiar with the latest changes myself
09:37 < michaelfolkson> Just one that I can see from the comments. But that is the only review attention since March. Ok I'll contact James and see what he is thinking
09:40 < sipa> i'm not sure there is a point in doing so while the c++ code isn't in a mergeable state
09:42 < michaelfolkson> Ah ok. That is certainly good to know
09:42 < michaelfolkson> Ok. Maybe worth temporarily closing both then and then reopening them, opening a new PR at a later date?
09:45 < michaelfolkson> I don't know if other people will have similar questions/thoughts to me when looking at these open PRs. I'm happy to communicate this on the PRs if that would be helpful
11:45 < sanket1729> andytoshi: I think that overloading the Associated type seems odd
11:47 < sanket1729> I also confused the two solutions. What I meant previously was having Pkh(Pk::Hash, Option<Pk>)
11:48 < sanket1729> So that should also solve the problem when someone uses the compiler on bitcoin::PublicKey type gets a hash160::Hash, but then manually has to write to supply a mapping to satisfier. 
11:52 < sanket1729> Maybe, I just feel odd because it is called Hash
11:55 < sanket1729> But it is Hash in the sense that the compiler would covert Pk -> Pk::Hash type. And that is the only(?) use of associated type. 
11:56 < sanket1729> I guess I want is a simple satisfier API in case the compiler coverts Pk to PkH.
11:58 < sanket1729> Right now, we lose that information. Ideally, we should save return it in some form that should be easier to use in satisfy API
12:00 < sanket1729> andytoshi: What do you think about returning a (Pkh -> Pk) mapping as additional compiler output? Maybe that should also solve darosior's issue?
12:05 < sanket1729> sipa, michaelfolkson: How does the test Framework PR fit in the process of getting Miniscript into core?
12:07 < sanket1729> Does core require an test framework support for a new feature?
12:09 < sanket1729> I guess practically speaking it maybe good for people to be familiar with Miniscript in general before reviewing the c++ code and maybe python is a faster way to do that? 
12:10 < sanket1729> And once reviewers are more familiar with the testframework code, we can get better reviews for main c++ code.
12:11 < sanket1729> I don't think that many people are familiar with Miniscript in general to review it. But maybe I am wrong. 
12:16 < michaelfolkson> sanket1729: If you look at sipa's PR introducing Miniscript to Core he just included some unit tests. He didn't write any functional tests https://github.com/bitcoin/bitcoin/pull/16800
12:18 < michaelfolkson> I guess the follow up question to that is what functional tests ideally would be added to sipa's PR if he had access to Miniscript in the functional test framework
12:20 < sanket1729> Okay, makes sense. I agree that we should have TestFramework support first so that we can have functional tests. 
12:20 < sanket1729> We can have functional tests for generic signer.
12:22 < sipa> one advantage can be that the miniscript parsing/descriptor logic could go in first as a part of the C++ code, and be tested against generic signing code implemented in python
12:22 < sipa> actually, nevermind - parsing/descriptor logic has nothing to do with signing
12:24 < michaelfolkson> But breaking down both PRs into smaller pieces and then introducing them bit by bit
12:26 < michaelfolkson> I trust your judgement though sipa. If you don't think it can't be done without you and ideally James too then I'm not in a position to argue :)
12:34 < sanket1729> there is also the question of Taproot support? I guess that will atleast be a year from now. 
12:35 < sanket1729> I guess that will have new descriptors altogether so there won't be any breaking changes?
12:35 < michaelfolkson> I spoke to James and he has some time to help which is great on the functional test framework PR side
12:36 < sipa> sanket1729: yeah
12:41 < michaelfolkson> James says he doesn't know if achow101 has any wallet plans for Miniscript support. I'm presuming he doesn't right?
12:42 < achow101> michaelfolkson: The plan is for miniscript to be part of descriptors, which then implicitly becomes part of the wallet via descriptor wallets
12:45 < michaelfolkson> Gotcha. So that will happen when (if!) #16800 is merged
12:45 < michaelfolkson> https://github.com/bitcoin/bitcoin/pull/16800
12:53 < sipa> michaelfolkson: only for watchonly though, as #16800 doesn't include signing support
16:12 < andytoshi> sanket1729: i really don't see what is confusing about this :(
16:12 < andytoshi> nor do i see how an additional compiler output could possibly help anything
16:12 < andytoshi> it sounds like there needs to be more documentation on the MiniscriptKey trait because it was explicityl designed to make my suggestion work
16:15 < andytoshi> sanket1729: what part of the word "Hash" contradicts the use of the identity function in your mind?
16:16 < andytoshi> remember that we are using fixed-size inputs here
16:16 < andytoshi> so i am not aware of any academic definition of the word that would exclude the identity
16:18 < sanket1729> It does contradict anything directly. We are using it as a translation from Pk -> Pkh only in the compiler method. So it is return type for PkH for the compiler. 
16:19 < sanket1729> Sometimes, we lose information in that transformation. 
16:19 < sanket1729> I don't think there is any other place we use to_pubkeyhash() method
16:20 < andytoshi> ok, cool, so forget about the method
16:20 < andytoshi> Pk::Hash is the type that goes into the PkH variant of the astelem enum
16:20 < andytoshi> it is called Hash because that's what the H stands for
16:21 < andytoshi> in wallet code, the type in the PkH variant should be the same sa the type in the Pk variant. there is no reason not to do this, and things break if yuo don't
16:21 < andytoshi> therefore in any wallet pubkey type, Pk::Hash should be the same as Pk
16:21 < sipa> maybe Identifier would be a clearer name than Hash?
16:21 < andytoshi> the only type where it actually _has_ to be a hash is for bitcoin::PublicKey, because specifically for Miniscript<bitcoin::PublicKey> we want to be able to roundtrip from bitcoin::Scipt
16:22 < sipa> (i'm only very vaguely following, ignore me if what i say doesn't make sense)
16:22 < andytoshi> sipa: maybe, though i would like to clarify that its purpose is to go into PkH
16:22 < andytoshi> sipa: no, you're making sense
16:22 < andytoshi> there's a tension between calling it "Hash" because it goes in PkH, and calling it something much more generic because outside of Script, it is actually much more generic
16:22 < sanket1729> In a multi-party setting with different wallets, it maybe possible that Pk::Hash is different types?
16:22 < andytoshi> if multiple parties are using different keytypes then they can't interoperate
16:23 < sanket1729> I was hoping all of them could use the default impl of MiniscriptKey on bitcoin::Publickey
16:23 < andytoshi> no, bitcoin::PublicKey is not really appropriate for wallet code
16:23 < andytoshi> because it has no information about how to derive keys etc
16:23 < andytoshi> it's useful as an interchange format, provided everyone has mappings to get to their "real" keytypes
16:24  * sipa believes there should just be a Key type, and no distinction between what is used in Pk and PkH
16:24 < andytoshi> sipa: how could you parse a script then?
16:25 < sipa> andytoshi: in places where you're parsing a script, without context to help resolve pkhashes, you can't, and IMO don't need to, convert to descriptor text form
16:25 < andytoshi> converting to text form is only one thing that you migth want to do with a miniscript
16:25 < sipa> e.g. in signing code you can make Key = uint160, and use a lookup map for what the keys mean for signing (as you have the private key anyway)
16:26 < andytoshi> in signing code you don't need anyting script related, you just need enough of the transaction to make a sighash
16:26 < andytoshi> if you mean witness assembly code, i don't see how you can assume anything about which keys are available
16:26 < sipa> in signing code you can get away with having even the argument of Pk() be a uint160 i mean... as you never need the full pubkey
16:27 < sipa> signing/witness assembly
16:27 < andytoshi> then you can't compute the witness script
16:27 < andytoshi> for signing you don't need miniscript at all. if you are trying to write a Combiner or something you need to distinguish between pubkeys and pubkeyhashes
16:28 < sipa> well you can if you have a lookup from pkh to pk; which you have anyway if you have a PSBT, or a descriptor, or the private key when signing
16:28 < andytoshi> ok, sure, but then the miniscript API needs a bunch of extra lookups and error paths
16:29 < andytoshi> and tbh i don't remember what problem we are solving here?
16:29 < sipa> just saying that things are simpler with just one key type :)
16:29 < sipa> maybe that's not the case in how your api is structured, that's ok
16:29 < andytoshi> ok :) i respectfully disagree
16:29 < andytoshi> this maybe comes down to a difference in how the API is structured
16:29 < andytoshi> probably*
16:30 < andytoshi> in any case -- i have an API which allows you to use the same thing in Pk and PkH if you want, when this is the simpler thing
16:30 < andytoshi> and to distinguish them if you want, when this is the simpler thing
16:30 < sipa> ok     
16:30 < andytoshi> and then all the extra lookups/error paths happen when converting keytypes, which you only need to do if you're switching between these views
16:40 < sanket1729> andytoshi: agreed. Thinking of "Hash as something that goes into PkH astelem" instead of something that "compiler translates Pk to" helps clarify things.
16:42 < andytoshi> ah that makes sense
16:42 < andytoshi> fwiw i never think in terms of the compiler .. or use or even build the compiler :P
16:43 < sanket1729> But that does not completely answer why in our impl for MiniscripKey, Pk::Hash is not (Option<bitcoin::PublicKey>, hash160::Hash) for bitcoin::PublicKey.
16:45 < andytoshi> you mean the impl for DescriptorWalletKey?
16:45 < andytoshi> MiniscriptKey is the trait
16:45 < sanket1729> bitcoin::PublicKey
16:45 < sanket1729> impl MiniscriptKey for bitcoin::PublicKey
16:46 < sanket1729> Item Hash = Option<bitcoin::PublicKey>, hash160::Hash
16:46 < andytoshi> because that would result in a more complex API .. it wouldn't round-trip to script
16:46 < andytoshi> and when trying to do anything with the actual keys, there would be extra error paths
16:47 < andytoshi> a better question is, what would work better if this was our choice?
16:47 < sanket1729> Just the compiler :) 
16:47 < andytoshi> lol :)
16:48 < andytoshi> i do not think the miniscript API should be complicated for the sake of the compiler
16:48 < andytoshi> especially when, as in this case, it is easy to just define your own keytype which has the right Hash type
16:48 < andytoshi> iirc as long as you have ToPublicKey implemented on your keytype the compiler will work?
16:48 < sanket1729> Yes     
16:49 < sanket1729> It does not require ToPublicKey
16:49 < andytoshi> oh, does it just assume all keys are 33 bytes?
16:49 < sanket1729> Let me check
16:49 < sanket1729> I think yes
16:49 < andytoshi> i also think yes
16:50 < andytoshi> i can't imagine we'd have put any work into supporting 65-byte keys inthe compiler..
16:51 < sanket1729> Does PkH work on 65 byte pubkeys?
16:51 < andytoshi> i think so, yes
16:51 < sanket1729> It should, right.
16:51 < andytoshi> like, does witness size estimation etc
16:51 < andytoshi> i'm pretty sure it does
16:52 < andytoshi> oh, you mean does hash computotino work
16:52 < sanket1729> Yeah, so we assume 33 byte keys
16:52 < andytoshi> yeah ofc, it just serializes the keys and puts that into a hash engine
16:52 < andytoshi> right, i remember us deciding that for the compilre
16:54 < sanket1729> So, what about returning Pk::Hash -> Pk mapping for the compiler. The only problematic workflow I have is 1) User compiles policy 2) Pk gets changed to PkH 3) Writing satisfaction API is ackward.
16:55 < sanket1729> *satisfier trait is akward
16:55 < sanket1729> I agree with everything else makes sense about Pk::Hash type
16:56 < andytoshi> yeah, having the compiler return a mapping seems sensible
16:56 < sanket1729> But as you say, the users should use their own MiniscriptKey
16:56 < andytoshi> sure
16:56 < andytoshi> so in that case the mapping would wind up being silly/redundant
16:57 < sanket1729> Yeah
16:57 < sanket1729> Maybe, things are best the way are :)
16:57 < andytoshi> so there's a sensible workflow where the user creates policies with DescriptorWalletKey (which has Pk::Hash == Hash), in which case there is no mapping needed
16:57 < andytoshi> for almost everything they do, they just leave things as is
16:58 < andytoshi> but specifically for conversion to script / inclusion in PSBT, they convert to bitcoin::PublicKey
16:58 < sanket1729> Is it possible to give a default value to Item Hash = Self?
16:58 < andytoshi> hmmm, i'm unsure
16:59 < andytoshi> the short answer is yes, the long answer is that rust's support for default type parameters is pretty sketchy
16:59 < andytoshi> try it and see :P
16:59 < sanket1729> If we can do it, do you agree that we should have it merged?
16:59 < sanket1729> Because most users want only one type for pk and pkh?
17:00 < andytoshi> yeah
17:00 < andytoshi> and in any case, we should document that this is usually the rigth thing to do
17:01 < sanket1729> It is unstable
17:01 < sanket1729> #(associated_type_defaults)
17:01 < andytoshi> heh damn. not too surprised
19:20 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has joined ##miniscript     
21:02 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection]     
21:03 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined ##miniscript     
21:59 -!- jeremyrubin [~jr@c-73-15-215-148.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds]     
22:32 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Quit: jonatack]     
23:18 -!- jonatack [~jon@37.167.90.115] has joined ##miniscript