Support Miniscript (with C#) #684

joemphilips · 2019-04-22T09:09:11Z

Previous attempt is in #675
I'e been rewriting from F# to C# for following reason.

Mixing F# and C# in one solution complicates the build process.
Often the editor screwed me up. which also caused a Maintainability issue.
I wanted to refactor some logic. Namely, internal representation can be simplified by using classes more closer to the actual script.
Ease for review.

NicolasDorier · 2019-04-29T10:07:18Z

Let me know when it is ready to review. I will probably use miniscript in NBXplorer so users can't have their own way to make arbitrary scripts.

joemphilips · 2019-04-29T11:19:00Z

I think I can finish it in three days or so.

NicolasDorier · 2019-04-29T16:07:01Z

I have quickly checked. I think you do way too complicated for GetHashCode/Equal implementations.
I would say you don't have to implement them at all, or if you do, you can just compare the string representation of the AST of the miniscript.

joemphilips · 2019-04-30T06:26:40Z

@NicolasDorier Thanks for review.
Yeah those GetHashCode and Equal methods are based on decompiled F# version.
I think it doesn't hurt so we can leave it there though.

joemphilips · 2019-05-01T18:22:46Z

Ready for review.

NicolasDorier · 2019-05-02T03:43:33Z

Wow so F# just do this by default? cool.

knocte · 2019-05-02T10:19:53Z

Wow so F# just do this by default?

Yes!

joemphilips · 2019-05-02T11:50:43Z

The error in CI seems nothing to do with my code.

Wow so F# just do this by default? cool

In fact, it does more than this. IEquatable and IStructuralEquatable and IComparable by default.
three line DU in F# will be few hundreds line of code in C# . One reason I tried to write this thing in F# first.

joemphilips · 2019-05-02T12:00:09Z

The error in CI seems nothing to do with my code.

~~Oops sorry. It was my fault. Will fix now.~~ Fixed

joemphilips

I did some simple self review.

joemphilips · 2019-05-03T16:25:19Z

NBitcoin.Tests/Helpers/PrimitiveUtils.cs

@@ -0,0 +1,41 @@
+


This class contains a method used to to be in transaction_test .
Some functions (e.g. RandomCoin ) is quite reusable so I've moved it to here.

joemphilips · 2019-05-03T16:26:23Z

NBitcoin.Tests/MiniscriptTests.cs

+		[Fact]
+		[Trait("UnitTest", "UnitTest")]
+
+		public void ShouldPlayWellWithTransactionBuilder_2()


See this test for how TransactionBuilder Supports HTLC by Miniscript

joemphilips · 2019-05-03T16:27:29Z

NBitcoin/BIP174/PSBTInput.cs


 		public bool IsFinalized() => final_script_sig != null || final_script_witness != null;

-		/// <summary>


Removing this method has nothing to do with Miniscript. But I noticed that it is legacy code from my version of PSBT. So I removed it.

NBitcoin/BuilderExtensions/MiniscriptBuilderExtension.cs

joemphilips · 2019-05-03T16:36:11Z

NBitcoin/Miniscript/Satisfy.cs

+			}
+			if (ageS.LockType == SequenceLockType.Time)
+			{
+				errors.Add(new SatisfyError(SatisfyErrorCode.UnSupportedRelativeLockTimeType, this));


Currently, I decided to support only block-height based lock for time (which is OP_CSV )
so in DSL "time(0) ~ time(65535)" is allowed.

I don't know this will be standard among other Miniscript implementation. So it might change in the future.

NBitcoin/TransactionBuilder.cs

NicolasDorier · 2019-05-06T03:25:22Z

NBitcoin/TransactionBuilder.cs

 		}

+		Sequence? _Sequence;
+		public TransactionBuilder SetRelativeLockTime(int lockHeight)


I think you can just use SetSequence(Sequence).

My only problem with this is that sequence is a per input concept, not a global tx concept.
But I guess this is still useful if a wallet has several keys depending on the same miniscript with same expiration?

I think the api to take int blockHeight or TimeSpan period is better than taking Sequence because we must assure tx nVersion is bigger than 1.
users are not interested into how the rule is encoded. they just want to use the relative locktime feature.

What about
SetRelativeLockTimeTo(ICoin whichCoin, int lockHeight)

and hold the info as Dictionary<ICoin, Sequence> ? In this way, we can assure that it will affect only specific input.

Tell me if the current implementation is not good. I decided to use Dictionary<OutPoint, Sequence> since Coins does not support GetHashCode

NicolasDorier · 2019-05-06T03:27:45Z

NBitcoin/TransactionBuilder.cs

-					var scriptSig1 = extension.GenerateScriptSig(scriptPubKey, keyRepo, signer);
-					var scriptSig2 = extension.GenerateScriptSig(scriptPubKey, signer2, signer2);
+					var scriptSig1 = extension.GenerateScriptSig(scriptPubKey, keyRepo, signer, preimageRepo, txIn.TxIn.Sequence);
+					var scriptSig2 = extension.GenerateScriptSig(scriptPubKey, signer2, signer2, preimageRepo, txIn.TxIn.Sequence);


I don't think that txIn.TxIn.Sequence can ever be useful to an extension.

It is not necessary, but I think it is useful in the case of Miniscript extension
because txbuilder fails to sign when

script contains OP_CSV. and

user forgets to set Sequence appropriately.

without this, users will notice that they have signed to the invalid tx when they try to broadcast.

(To make me clear, the user does not have to set nSequence even if OP_CSV is in the script if he tries to redeem through non-timelocked path)

But don't you have this problem in all case anyway? The transaction might still not be broadcastable depending on the Sequence, and the only way to know is to have the previous UTXOs's height.

Yes, TX might be un-broadcastable anyway, difference is that if the user signs to nSequence which is not suitable to pass OP_CSV (e.g. disable flag is set) that signature will be useless for redeeming through timelocked redeem condition. so the tx wont be valid forever.

NicolasDorier · 2019-05-06T03:32:58Z

Unrelated to this PR, but it makes me think that a BIP should be proposed for PSBT to support adding preimages.

joemphilips · 2019-05-07T06:27:47Z

Unrelated to this PR, but it makes me think that a BIP should be proposed for PSBT to support adding preimages.

Indeed, I was quite surprised that no one has written yet.

NicolasDorier · 2019-05-07T13:42:38Z

@joemphilips if you feel motivated to do the BIP, you can ask to people on #bitcoin-dev on IRC about it. If nobody did, do your own, publish on the mailing list.
I don't think there is lot's of way to disagree with, so as long as it is well redacted, it should be fine and we can later add implementation to TxBuilder.

NicolasDorier · 2019-05-16T16:12:35Z

Hey, so I just created the type IHDScriptPubKey that you want to implement for MiniScript.
This allow to use PSBT methods like GetBalance or SignAll.

If it get possible to finalize any miniscript then I can move this to NBXplorer then BTCpayServer.

BTCPayServer now support the signature of arbitrary PSBT, so as long as we have the finalizer... it will work!

joemphilips · 2019-05-16T23:46:05Z

Ah thanks for mentioning.
PSBT and OutputDescriptor support for BTCPayServer is super cool IMO ... It opens up possibility for many extensions.

I've just realized that the spec for output descriptor in bitcoin core has been much detailed and broad than last time I've read. I will update parsers and its tests in this weekend. (And finalizer afterwords)

BTW, I've been using namespace NBitcoin.Miniscript but this collides with Miniscript class in there, so I want to change the namespace name, what do you think is the best name? ( NBitcoin.SmartContract or NBitcoin.MiniscriptModule ?)

NicolasDorier · 2019-05-17T02:11:34Z

@joemphilips I did an object model to build a OutputDescriptor, but it does not support parsing.

Maybe NBitcoin.Scripting for both MiniScript and OutputDescriptor is good.

* Remove useless methods from PSBT. * Introduce ISha256PreimageRepsitory for TransacitonBuilder * Fix sum bug in * SatisfyException * MiniscriptScriptParser

* Add tests for SatisfyCSV * Fix bug when instantiating AbstractPolicy.Time * Support only blockheight-based relative locktime in Minsicript. * Add null check when instantiating OutputDescriptor

…o specific input

* 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.

NicolasDorier · 2019-05-21T08:43:53Z

lol this PR is massive, can you break it down into 2 PR, like one with only the output descriptor stuff and the other one with miniscript?

joemphilips · 2019-05-21T10:26:06Z

OK. I should have created separate PR in the first place ... Miniscript and OutputDescriptor is quite different beast.

joemphilips · 2019-05-22T06:30:29Z

Closing in favor of #694, #695 , #696 .

joemphilips mentioned this pull request Apr 22, 2019

Support Miniscript as a separate dll. #675

Closed

4 tasks

joemphilips force-pushed the csharp_miniscript branch 3 times, most recently from 1491607 to 2901351 Compare April 24, 2019 07:24

joemphilips force-pushed the csharp_miniscript branch 2 times, most recently from b30723d to cf3cce2 Compare May 1, 2019 07:21

joemphilips changed the title ~~WIP: Support Miniscript (with C#)~~ Support Miniscript (with C#) May 1, 2019

joemphilips force-pushed the csharp_miniscript branch from 448d5cf to ec0b258 Compare May 2, 2019 12:02

joemphilips commented May 3, 2019

View reviewed changes

NBitcoin/TransactionBuilder.cs Outdated Show resolved Hide resolved

NicolasDorier reviewed May 6, 2019

View reviewed changes

joemphilips force-pushed the csharp_miniscript branch from f7079cf to 59df0c0 Compare May 7, 2019 09:46

joemphilips force-pushed the csharp_miniscript branch from 6b0dbf8 to fcb6e72 Compare May 10, 2019 16:05

WIP: rewrite miniscript in C# from scratch

6ea9dae

joemphilips added 20 commits May 19, 2019 13:01

Prepare OutputDescriptor

f302d28

Add test to assert Satisfy does not at least throw error

539567f

Add testcase for ScriptParser

eee55dd

Move reuseable helpers in transaction_tests to separate class

41dd18d

* Add method to calculate max push items size for Miniscript.

2be27bd

* Remove useless methods from PSBT. * Introduce ISha256PreimageRepsitory for TransacitonBuilder * Fix sum bug in * SatisfyException * MiniscriptScriptParser

Finish working with HTLC in TransactionBuilder

9faf00d

Improve performance for OutputDescriptor.[GetHashCode|Equals]

85903c1

Add validation in OutputDescriptor constructor

7116817

Improve OP_CSV related behaviour of Miniscript

036cfe7

* Add tests for SatisfyCSV * Fix bug when instantiating AbstractPolicy.Time * Support only blockheight-based relative locktime in Minsicript. * Add null check when instantiating OutputDescriptor

Support HTLC in TransactionBuilder

a4ed336

Improve Shrinker

db3ba2e

Improve TransactionBuilder.SetRelativeLocktime() so that it can set t…

1248504

…o specific input

Improve compiler and deserializer

6b52675

Assure miniscript holds only T expression

316765c

Remove unused Tuck from ScriptToken

84cdf27

Optimize Miniscript by using Debug.Assert

69de96a

Make ScriptParser.TryParse more safe

0ee78db

nits

c0b37e3

Rename NBitcoin.Miniscript to NBitcoin.Scripting

cd063e3

WIP: Update output descriptor according to bitcoin core

0cb0e0a

joemphilips force-pushed the csharp_miniscript branch from 7d99209 to 0cb0e0a Compare May 19, 2019 04:02

joemphilips added 2 commits May 20, 2019 08:44

Update OutputDescriptor Parser

3913920

Update OutputDescriptor

f23e17a

* 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.

This was referenced May 22, 2019

Prepare Parser for scripting #694

Merged

Support miniscript #695

Closed

Support output descriptor #696

Merged

joemphilips closed this May 22, 2019


		public bool IsFinalized() => final_script_sig != null \|\| final_script_witness != null;

		/// <summary>

Support Miniscript (with C#) #684

Support Miniscript (with C#) #684

Uh oh!

Conversation

joemphilips commented Apr 22, 2019

Uh oh!

NicolasDorier commented Apr 29, 2019

Uh oh!

joemphilips commented Apr 29, 2019

Uh oh!

NicolasDorier commented Apr 29, 2019

Uh oh!

joemphilips commented Apr 30, 2019

Uh oh!

joemphilips commented May 1, 2019

Uh oh!

NicolasDorier commented May 2, 2019

Uh oh!

knocte commented May 2, 2019

Uh oh!

joemphilips commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joemphilips commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joemphilips left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joemphilips May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NicolasDorier May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented May 6, 2019

Uh oh!

joemphilips commented May 7, 2019

Uh oh!

NicolasDorier commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joemphilips commented May 16, 2019

Uh oh!

NicolasDorier commented May 17, 2019

Uh oh!

NicolasDorier commented May 21, 2019

Uh oh!

joemphilips commented May 21, 2019

Uh oh!

joemphilips commented May 22, 2019

Uh oh!

Reviewers

joemphilips commented May 2, 2019 •

edited

Loading

joemphilips commented May 2, 2019 •

edited

Loading

joemphilips May 3, 2019 •

edited

Loading

NicolasDorier May 6, 2019 •

edited

Loading

NicolasDorier commented May 7, 2019 •

edited

Loading

NicolasDorier commented May 16, 2019 •

edited

Loading