--- Log opened Sat Dec 18 00:00:04 2021
09:29 < darosior> Small update regarding the PR to Bitcoin Core. It's getting close. I've rebased and updated it with the repo once again. I've completed the signing support and i've tested the watchonly support by porting my application (Revault) -which currently imports raw scriptpubkeys and handles the gap limit itself- to import Miniscript descriptors to the
09:29 < darosior> watchonly wallet. The (extensive) functional tests suite passed, which is a good sign that at least the watchonly support isn't obviously broken :). I've also done some small cosmetic changes on the code i modified to avoid too many nitpicks. I've split the PR in 3 parts:
09:29 < darosior> - https://github.com/darosior/bitcoin/pull/2 which introduces the Miniscript logic
09:29 < darosior> - https://github.com/darosior/bitcoin/pull/5 which introduces Miniscript descriptors
09:29 < darosior> - https://github.com/darosior/bitcoin/pull/6 which implements signing support for the Miniscript descriptors
09:29 < darosior> This is still not ready, i'm now working on adding functional tests to both the watchonly and signing PRs. Then i'll have a final look and would probably ask whoever is willing to here to have one as well, and then would move forward with the PR if there is no objection raised.
09:30 <@sipa> Are there any recursive tree walking algorithms in the watch-only part of the code left?
09:31 <@sipa> With inferring and parsing changed, I'm not sure there is.
09:31 <@sipa> But I think we should get rid of all of them before PRing to core at least.
09:32 < darosior> Ok, i'll have a look. I added a recursive helper for (much!) better error reporting on parsing errors though :/ https://github.com/darosior/bitcoin/pull/5/commits/0782e00624a2ebb55010d62ddbf759c97ad06081
09:34 <@sipa> The signing code is still obviously recursive at least.
09:35 <@sipa> I think it'd not be too difficult to write a generic "recursive walk" function to the miniscript object, which takes a templated visitor object that given a node, and evaluation of subnode, gives the evaluation of the node.
09:35 <@sipa> Which puts things on a stack like the parsing code does.
09:35 <@sipa> I'm happy to give that a shot too
09:37 < darosior> Ok, if you do ping me i'll review and test
11:38 < jeremyrubin> one thing i'd like to see as a part of a merge of miniscript to bitcoin core would be a testing suite that fuzzes/qas core miniscript against rust miniscript. maybe that burden can be borne by rust-miniscript, maybe elsewhere, but i feel it's really important before we start making a "core standard" version that we have a stronger spec to test
11:38 < jeremyrubin> impls against for identical semantics
11:41 < jeremyrubin> another thing approach wise that might make sense would be to p_impl all the miniscript stuff so you could hide different implmentations (e.g., rust-miniscript, cpp-miniscript) behind a plugin wrapper. this would require some effort to add a C API to rust-miniscript, but i think this sort of stuff is pretty important
11:41 < jeremyrubin> i suppose another approach would be a rust wrapper around c miniscript, but that seems like a worse direction...
11:41 <@sipa> That's very hard with the degree of templating going on.
11:42 <@sipa> Miniscript object are templated in the type of key objects they use
11:42 < jeremyrubin> same in rust -- but how many concrete types do we actually need to use from bitcoin-core?
11:43 <@sipa> Two, at least. But I don't see how you'd combine any templating cleanly with a pimpl.
11:43 <@sipa> I guess you could have one for each.
11:43 < jeremyrubin> i'd envision like... 5? {ECDSA, Taproot}x{HD, Not_HD} + {String Name}
11:43 <@sipa> No, no.
11:44 <@sipa> Miniscript as-is is P2WSH only really.
11:44 <@sipa> The templating is one for signing purposes, and one for descriptors.
11:44 < jeremyrubin> maybe let's ease the conversation back a step
11:45 < jeremyrubin> 1) if it were easy, is this something that would be desirable to do before inclusion in core?
11:45 <@sipa> Do what?
11:45 < jeremyrubin> p_impl plugin API, testing fuzzing both implementations across each other.
11:46 <@sipa> I don't think so.
11:46 <@sipa> I do think we need interop testing between the two implementations.
11:46 < jeremyrubin> ok, so then no need to discuss if it's easy or hard to do
11:46 < jeremyrubin> why don't you think it's desirable?
11:46 <@sipa> But I think there are easier ways to do that.
11:47 <@sipa> I think it will complicate the implementation a lot.
11:48 < jeremyrubin> we're still in 'assuming it was easy' mode
11:48 < jeremyrubin> what do you think would be desirable to have in terms of integration / interop testing?
11:48 <@sipa> Just have a binary that links both codebases, and tests them against each other e.g.
11:49 <@sipa> Or, as we've done before, two separate tools, one against each implementation, being fed the same randomly-generated input.
11:49 <@sipa> We found lots of bugs that way.
11:50 < jeremyrubin> That's part of what i was suggesting as an option. I agree that's good and helps with review of the code both ways, in addition to compatibility checking.
11:51 <@sipa> Yeah, no doubt we need a way to do interop testing on a continuous basis, not just one off.
11:51 <@sipa> But I don't think that needs to be part of the Bitcoin Core codebase.
11:53 < jeremyrubin> i'm not sure where it should live, but I do think if cpp-miniscript can't live nicely as a submodule that core consumes cleanly (hence my pimpl suggestion) then it makes it more difficult to feel it shouldn't be a part of core. it could be a part fo rust-miniscript instead, as i suggested
11:54 <@sipa> i think the plan is to first integrate it into core, so we have something that works
11:54 <@sipa> and then extract it as something more standalone if there is desire for that
11:54 < jeremyrubin> my worry with that approach is that it makes rust-miniscript heavily feel like a downstream project to core-miniscript
11:54 < jeremyrubin> maybe we can merge into knots :p
11:54  * jeremyrubin ducks
11:55 <@sipa> i mean... pimpl helps you hide implementation code
11:55 < jeremyrubin> and i think that if something is going to core it's just worth clarifying where the 'spec' lives, since core becomes a de-facto spec often times
11:55 <@sipa> it's not directly a way for swapping things out, or enabling comparison between two implementation
11:56 <@sipa> If you want to do that, just write a wrapper class that delegates every function to two separate implementations.
11:57 < jeremyrubin> that's what pimpl does? then you can just link against whichever miniscript you want
11:57 <@sipa> Right, but what's the point of that?
11:57 <@sipa> You want to be able to run tests that compare the two.
11:58 <@sipa> Not swap out the implementation for production.
11:58 < jeremyrubin> i think you might want to swap them for production.
11:58 <@sipa> why?
11:59 < jeremyrubin> e.g., i work on a better policy engine for rust-miniscript to be used across my organization. i want my bitcoin-core wallet to use the same one.
11:59 < jeremyrubin> i don't want to have to maintain both a cpp-miniscript policy extension AND a rust-miniscript one
11:59 <@sipa> what's a policy engine?
12:00 < jeremyrubin> i suppose policy compiler is not going to be in the miniscript library being added?
12:00 <@sipa> policy to miniscript compiler?
12:00 <@sipa> i wasn't expected that to be included in bitcoin core
12:01 < jeremyrubin> fair enough
12:03 < jeremyrubin> i think you could also extend the point to 'some piece of safety analysis being added to my rust-miniscript'
12:04 < jeremyrubin> in any case, i guess the meta point i'm making is that i would appreciate if we don't make core the de-facto spec for something it doesn't need to be.
12:04 <@sipa> i mean... descriptors are a standard that started as a bitcoin core specific language.
12:05 <@sipa> And miniscript is a proposal for extending that.
12:06 < jeremyrubin> sure, but in theory this is stuff we want all kinds of devices and software to be able to support
12:07 < jeremyrubin> e.g. anything that could consume a PSBT
12:07 < jeremyrubin> so it makes sense to me that changes to a protocol that should be consumable extenrally would be spec'd as a BIP and not just as 'this is what core does'
12:08 < jeremyrubin> altho BIPs are sometimes "this is what core does"
12:08 <@sipa> absolutely
12:08 <@sipa> but the BIP will describe something
12:08 <@sipa> it's not like you can get someone entirely neutral to write a BIP, and then let implementations come later
12:09 <@sipa> this stuff is so complex it grows over several iterations, and in the end you have a working prototype that gets written up as spec
12:09 < jeremyrubin> absolutely. miniscript is unique in that there already *are* two very strong implementations in existence
12:09 < jeremyrubin> i think that hasn't really happened before?
12:09 <@sipa> and we're lucky to have two presumed to be very compatible implementations to exist
12:10 <@sipa> yeah, and that has been very useful too
12:10 < jeremyrubin> hence making reifying a spec that both can work under and not prioritizing one over the other valuable unless we want to kill off the other implementation
12:10 <@sipa> lots of issues were found by comparing the two
12:10 <@sipa> i mean: we already have a spec, it's on my site
12:10 <@sipa> and it hasn't killed off anything
12:11 -!- roconnor [~roconnor@coq/roconnor] has quit [Ping timeout: 240 seconds]
12:11 <@sipa> it has led to small discrepancies being discovered, which were then either fixed in one of the implementations, or resulted in a spec change
12:20 < jeremyrubin> ok so i refreshed my notes a bit
12:22 < jeremyrubin> one of the things being worked on for rust-miniscript is a plugin-API so that extensions to miniscript can more easily be added, e.g., OP_CTV is currently a tough patchset on rust-miniscript but it'd be better if there was a clean way to extend with new functions. An org might prefer to link to a miniscript that can handle all of their descriptors
12:22 < jeremyrubin> across the org, and they might need to then also do cpp-miniscript and then check that things are equal for example, so that's why there might be a preference for linking against one or the other. 
12:22 < jeremyrubin> these extensions could arise on either side of the fence to fit new features in in a compatible way
12:23 < jeremyrubin> it would be a good effort if core should not release a new feature (and rust-miniscript alike) without ensuring that rust-miniscript and cpp-miniscript latest versions will be compatible.
12:23 <@sipa> but it's never a matter of just linking one implementation v.s. another
12:24 <@sipa> miniscript isn't a naturally abstractable API
12:24 <@sipa> given that it is parametrized by the user's key type and signing environment
12:26 <@sipa> what you're suggesting, if you actually want something that permits swapping of everything for production, effectively means replacing all of Bitcoin Core's key/signing logic, together with a significant portion of wallet/script code.
12:27 <@sipa> Unless you make the rust and C++ implementation so compatible that you can use one's key type and signing code in combination with the other miniscript logic.
12:28 < jeremyrubin> that seems like something that should be possible, modulo a serialization boundary?
12:28 -!- roconnor [~roconnor@coq/roconnor] has joined ##miniscript
12:28 < jeremyrubin> the performance penalty would only have to be paid by rust since c++ can just share the representations directly
12:29 <@sipa> And that sounds even more pointless; a large amount of work to make the two languages call each othet, plus needing APIs that need to be so compatible that it's hardly interesting anymore to have two.
12:30 <@sipa> Like the two implementations don't even agree on whay a key parameter is.
12:30 <@sipa> If you want to be able to enable mix and matching, they need to be basically identical.
12:32 <@sipa> Which also reduces the benefit of having two independent implementations in the first place.
12:33 < jeremyrubin> i'm not sure i see how it reduces the benefit...
12:33 < jeremyrubin> but in any case
12:33 < jeremyrubin> i'm gonna unplug for a bit; enjoy your saturday :) apologies if i've frustrated you. tagging andytoshi and sanket1729_ since they're project owners on this
12:33 < jeremyrubin> this == rust-miniscript
12:34 <@sipa> no worries!
12:35 <@sipa> I think we agree about this point at least: there should be some way of (continually) testing compatibility between the two implenentations.
12:40 < jeremyrubin> 100%
21:27 -!- sanket_cell_ [~sanket172@ec2-100-24-255-95.compute-1.amazonaws.com] has quit [Quit: ZNC 1.8.2 - https://znc.in]
21:27 -!- sanket1729_ [~sanket172@ec2-100-24-255-95.compute-1.amazonaws.com] has quit [Quit: ZNC 1.8.2 - https://znc.in]
21:28 -!- sanket1729 [~sanket172@ec2-100-24-255-95.compute-1.amazonaws.com] has joined ##miniscript
21:29 -!- sanket_cell [~sanket172@ec2-100-24-255-95.compute-1.amazonaws.com] has joined ##miniscript
--- Log closed Sun Dec 19 00:00:05 2021