Support output descriptor #696

joemphilips · 2019-05-22T06:28:54Z

depends on #694 , previous attempt is #684

Currently it is independent from #695 , and it follows bitcoin core's spec.

…ption

…matException

* Preparet test vectors from bitcoin core. * Allow to parse private keys * Inject ISigningRepository to pickup private key info when parsing * Prevent parsing Uncompressed pubkey when it is in witness context.

…esctipro

…r warnings

* Before this commit, Parser did not allow whitespace around '(', ')' and ','. By removing all whitespace as a preprocess for parsing, it is now rebust against user inputs. Also it is now bit faster because we don't have to call `.Token` combinator anymore.

* Remove useless `Tag` field from OutputDescriptor and PubKeyProvider * Refactor pattern match for `Equals` method. Both for the sake of consiceness.

... for `OutputDescriptor`, `PubKeyProvider`, `OutputDescriptorParser` and `ISiginingRepository`

...for OutputDescriptor and PubKeyProvider Since user must always access these types through parent type, there are no point for repeating the prefix twice. Thus we can simplify by dropping the its prefix (in this case `Descriptor` and `PubKeyProvider`)

lontivero

Some comments for your consideration.

NBitcoin/Scripting/OutputDescriptorParser.cs

NBitcoin/Scripting/PubKeyProvider.cs

* Add null check to everywhere public * Add check for user-provided enum value. * Make an indentation for switch expression a bit more natual.

NicolasDorier · 2020-09-18T13:05:05Z

I want implementation of the OutputDescriptor to be "closed" inside this library. i.e. user's should not create the new type by overriding OutputDescriptor, So I did not want to use virtual or abstract methods.

When you want to do this, you can just declare the abstract/virtual methods internal.

joemphilips · 2020-09-18T13:20:52Z

When you want to do this, you can just declare the abstract/virtual methods internal.

But doesn't it make it impossible for user to use the method?

NicolasDorier · 2020-09-18T13:43:48Z

@joemphilips yes, one thing often used also is have a method "BlahBlah" public and another one "BlahBlahCore" internal/protected.
With BlahBlah which don't have to do much... That's ugly if you have too many methods that need that, but if it is a one or two, I think that;s fine. (that said, I am not advocating for changing your code for using that, I did not reviewed enough your code yet to know if it indeed simplify things)

NicolasDorier · 2020-09-20T02:02:58Z

@lontivero you plan to still review some stuff or should I start review as well?

lontivero · 2020-09-22T16:59:58Z

I will not review it more.

NBitcoin.Tests/util_tests.cs

NicolasDorier · 2020-09-23T04:01:14Z

@joemphilips I think I will merge that soon. One question: About KeyId and WitScriptId. Do you really need it in say FlatSigningRepository?
As time pass, I get more and more convinced that KeyId/ScriptId/WitScriptId are classes that should never have existed in the first place, so I am curious why you need it.

joemphilips · 2020-09-24T01:59:26Z

I can try using uint160/uint250 instead if you want, I've just used it because I like using different types for those cases. (It makes hard to misuse the API.)
Let me check if that works or not.

joemphilips · 2020-09-24T02:59:03Z

After testing a bit, I think it is possible to use uint160/uint256 for ISigningRepository but It requires substantial refactoring.

e.g. I want to use the api like pk.WitHash.ScriptPubKey.Hash.ScriptPubKey or PayToPubkeyHashTemplate.ExtractScriptPubKeyParameters() which uses KeyId or ScriptIdso I first have to add a new methods/properties to PubKey , ScriptTemplate , that will be huge change on the existing code.

I suppose it is out of the scope for this PR, if you really want to make KeyId/ScriptId obsolete, first we’d better make those types obsolete in existing code.

(which I don’t think is necessary though btw, having wrapper types like WitScriptId and ScriptId is a nice way to specify the intention.)

* To perform the property test which uses an RPC.

NicolasDorier · 2020-09-24T09:53:03Z

@joemphilips don't refactor to use uint160/uint256 instead I just want to know, why you need those Ids classes rather than just ScriptPubKey

joemphilips · 2020-09-24T11:03:32Z

It is done in that way in original implementation
It is a bit smaller
It is more easy to understand the intention (e.g. for TryGetPubKey(KeyId keyId, out PubKey pubkey) , if the first argument is Script, reader might get confused whether that is ScriptPubKey for the p2pk, or ScriptPubKey for the p2pkh)

joemphilips · 2020-10-27T13:57:48Z

Would you consider merging this? I have a plan to use it. @NicolasDorier

* Implement `OutputDescriptorJsonConverter`.

NicolasDorier · 2020-10-29T03:21:36Z

Released a new version. (.62)

joemphilips mentioned this pull request May 22, 2019

Support Miniscript (with C#) #684

Closed

joemphilips changed the title ~~Support output descriptor~~ WIP: Support output descriptor May 22, 2019

joemphilips changed the title ~~WIP: Support output descriptor~~ Support output descriptor May 23, 2019

joemphilips force-pushed the OutputDescriptor branch 3 times, most recently from ef4f091 to e5bbb1a Compare May 27, 2019 09:55

joemphilips mentioned this pull request May 27, 2019

Prepare Parser for scripting #694

Merged

NicolasDorier mentioned this pull request Aug 24, 2020

Monkey porting I #904

Closed

joemphilips added 22 commits August 25, 2020 18:11

Prepare parser for scripting

4e885de

Rename ParseException -> ParsingException and inherit from FormatExce…

58fa6bb

…ption

fixup! Rename ParseException -> ParsingException and inherit from For…

ff398a5

…matException

Update output descriptor according to bitcoin core

fa33110

Update OutputDescriptor Parser

8f70153

Update OutputDescriptor

17ca10a

* Preparet test vectors from bitcoin core. * Allow to parse private keys * Inject ISigningRepository to pickup private key info when parsing * Prevent parsing Uncompressed pubkey when it is in witness context.

Handle Empty KeyPath correctly

2f61e1d

Fix OutputDescriptorGenerator

cd348b2

Fix bug in OutputDescriptor CheckSum && Use IDestination for AddressD…

b5d1731

…esctipro

Update Parser

df9665f

Update

6d0aad0

Update OD generator

817685c

Finish InferFromScript method

608c80c

Pass Core test for path derivation

e3a532e

Remove Miniscript related things

f4313c8

Move reuseable helpers in transaction_tests to separate class

85eda4e

Update comment

1073da3

Test Signability by txBuilder

ca9f9cb

Pass all OutputDescriptorTests

0008256

Clean the code a bit

326d26e

Rename ParseException -> ParsingException and get rid of some compile…

6617bdb

…r warnings

Use string.ToCharArray instead of ToArray

f300f35

joemphilips added 5 commits August 31, 2020 11:15

Pursue more functional beauty.

4d7df4f

* Remove useless `Tag` field from OutputDescriptor and PubKeyProvider * Refactor pattern match for `Equals` method. Both for the sake of consiceness.

use nullable reference type everywhere

589dc83

... for `OutputDescriptor`, `PubKeyProvider`, `OutputDescriptorParser` and `ISiginingRepository`

parameterize OutputDescriptorGenerator with Network

7881ac2

remove prefix from subtype name

bcf6320

...for OutputDescriptor and PubKeyProvider Since user must always access these types through parent type, there are no point for repeating the prefix twice. Thus we can simplify by dropping the its prefix (in this case `Descriptor` and `PubKeyProvider`)

lontivero reviewed Sep 17, 2020

View reviewed changes

NBitcoin/Scripting/OutputDescriptorParser.cs Show resolved Hide resolved

NBitcoin/Scripting/PubKeyProvider.cs Show resolved Hide resolved

Make an OD and PKProvider a bit stylish and safe

47b95fd

* Add null check to everywhere public * Add check for user-provided enum value. * Make an indentation for switch expression a bit more natual.

NicolasDorier reviewed Sep 23, 2020

View reviewed changes

NBitcoin.Tests/util_tests.cs Outdated Show resolved Hide resolved

joemphilips added 3 commits September 24, 2020 12:18

Revert the behavior of RootedKeyPath.ToString()

2d521d4

add RPCPropertyTests

dceeaf9

* To perform the property test which uses an RPC.

fixup! Revert the behavior of RootedKeyPath.ToString()

4d3d8b7

NicolasDorier mentioned this pull request Oct 5, 2020

Tor support cryptoadvance/specter-desktop#483

Closed

NicolasDorier force-pushed the master branch from 6794746 to 9592fb6 Compare October 21, 2020 13:40

joemphilips force-pushed the OutputDescriptor branch from 2626fe4 to 3cb820c Compare October 28, 2020 13:10

Enable to pass OutputDescriptor with importmulti

65abec5

* Implement `OutputDescriptorJsonConverter`.

joemphilips force-pushed the OutputDescriptor branch from 3cb820c to 65abec5 Compare October 29, 2020 02:12

NicolasDorier merged commit 825f265 into MetacoSA:master Oct 29, 2020

dennisreimann mentioned this pull request Dec 5, 2022

Remove "3 pubkeys in top level multisig" limit #1151

Closed

Support output descriptor #696

Support output descriptor #696

Uh oh!

Conversation

joemphilips commented May 22, 2019

Uh oh!

lontivero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NicolasDorier commented Sep 18, 2020

Uh oh!

joemphilips commented Sep 18, 2020

Uh oh!

NicolasDorier commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Sep 20, 2020

Uh oh!

lontivero commented Sep 22, 2020

Uh oh!

Uh oh!

NicolasDorier commented Sep 23, 2020

Uh oh!

joemphilips commented Sep 24, 2020

Uh oh!

joemphilips commented Sep 24, 2020

Uh oh!

NicolasDorier commented Sep 24, 2020

Uh oh!

joemphilips commented Sep 24, 2020

Uh oh!

joemphilips commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NicolasDorier commented Sep 18, 2020 •

edited

Loading

joemphilips commented Oct 27, 2020 •

edited

Loading

NicolasDorier commented Oct 29, 2020 •

edited

Loading