PSBT for Confidential Assets #600

gwillen · 2019-05-01T09:38:37Z

This PR extends the PSBT format and RPCs to handle Confidential Assets transactions.

New fields in PSBT inputs: Unblinded value, Value blinder, Unblinded asset, Asset blinder.
New fields in PSBT outputs: Recipient blinding pubkey, Value commitment, Value blinder, Asset commitment, Asset blinder, Nonce commitment, Range proof, Surjection proof.

We preserve the existing invariant that the unsigned transaction inside a PSBT never changes during the process; all updates that need to be applied to produce the final signed transaction are accumulated in the PSBT fields listed above.

The process is as follows:

Create a PSBT using converttopsbt [deprecated], walletcreatefundedpsbt, createpsbt
- At this point, the output pubkey fields will be filled, for any outputs to confidential addresses.
- Input fields may be filled, if the wallet was used; otherwise they will be filled in the next step.
- Incremental creation of the unsigned transaction itself is OUTSIDE the scope of this work. The unsigned transaction must be fully populated with inputs and outputs before the PSBT is created.
- Transactions with peg-in, issuance, or reissuance outputs are NOT supported at this time.
If necessary, update the psbt with input data using walletfillpsbtdata.
- This is like the old walletprocesspsbt RPC [deprecated], which tried to both fill and sign the PSBT (which is not workable in the Confidential Assets setting.)
Once all inputs have had necessary data updated, possibly using multiple wallets if necessary, any wallet can be used to blind the transaction using blindpsbt.
- This uses the input blinding data, along with the output blinding pubkeys, to compute the output blinding data.
- Incremental blinding is not supported. All input blinding data must be available when blindpsbt is called.
Then, the blinded PSBT must be signed using walletsignpsbt. As with updating, this can be done by multiple wallets as necessary for the inputs being signed.
Once all signatures are present, finalizepsbt is used to create the final transaction in the regular transaction format, as before.
Then sendrawtransaction is used, which will check to make sure that blinding was performed properly before sending.

instagibbs · 2019-05-01T13:36:02Z

could you rebase?

instagibbs

concept ACK, first look seems good.

instagibbs · 2019-05-01T13:37:56Z

src/wallet/wallet.cpp

 }

-CAmount CWalletTx::GetOutputValueOut(unsigned int output_index) const {
+void CWalletTx::GetNonIssuanceBlindingData(const unsigned int output_index, CPubKey* blinding_pubkey_out, CAmount* value_out, uint256* value_factor_out, CAsset* asset_out, uint256* asset_factor_out) const {


this refactoring could be its own commit

instagibbs · 2019-05-01T13:38:15Z

src/wallet/wallet.cpp

                // One change script per output asset.
                size_t index = 0;
-                for (const std::pair<CAsset, CAmount>& value : mapValue) {
+                for (const std::pair<const CAsset, CAmount>& value : mapValue) {


compiler complaint squashing could be its own commit

I think this must have gone away in the rebase

instagibbs · 2019-05-01T13:52:07Z

src/rpc/rawtransaction.cpp

+
+    CMutableTransaction tx_tmp = tx;  // We don't want to mutate the transaction in the PSBT yet, just extract blinding data
+
+    // TODO(gwillen): Make this do something better than fail silently if there are any issuances, reissuances, pegins, etc.


PSBTs can't really encode these anyways yet, I think we're safe for now.

Supporting the creation of issuance/peg-in inputs in a PSBT format is the harder part.

Well, the PSBT can encode anything the CTransaction can encode, so I can make a PSBT of an issuance -- I just can't blind it. (Perhaps I also can't sign it -- I didn't try a fully unblinded one.)

instagibbs · 2019-05-01T13:53:06Z

src/rpc/rawtransaction.cpp

+        o.range_proof = tx_tmp.witness.vtxoutwit[i].vchRangeproof;
+        o.surjection_proof = tx_tmp.witness.vtxoutwit[i].vchSurjectionproof;
+
+        o.blinding_pubkey = CPubKey();  // Once we're done blinding, remove the pubkeys to signal that it's complete


I guess this makes sense. We remove partial sigs once a full sig is complete right?

Yes, and I suspect that there's more pruning I could do with the goal of keeping the PSBTs small (which I expect is why we do that), but this specific removal is actually important as a signal to signpsbt that it's okay to sign. (Since the hack with the nonce field is gone in this flow.)

why wouldn't the presence of the other fields show that?

The only way to tell that an output is intended to be blinded is that it has a pubkey. Absence of blinders could mean "not going to be blinded" or "hasn't been blinded yet".

So I guess you could indeed check for "has a pubkey and blinders" or "has no pubkey", but just checking for not having a pubkey seems easier (and is similar to what the code was doing before). But I can go either way as you like.

instagibbs · 2019-05-01T13:54:21Z

src/rpc/rawtransaction.cpp

+
+    CMutableTransaction& tx = *psbtx.tx;
+
+    // TODO(gwillen): Replace all this with the 'bonus output' scheme to use an OP_RETURN to balance blinders, with a rangeproof exponent of -1 (public).


with known blinding pubkey, right?

I believe the scheme we talked about -- which I don't expect to fully understand before implementing it -- was to just make up a random pubkey and then throw it away after, because nobody ever needs to be able to rewind these outputs.

that works too. Could just be the generator point G or something stupid as well.

gwillen · 2019-05-02T01:18:22Z

Rebased, still need to split things up a bit. Also looks like I killed CI with the rebase, will investigate.

EDIT: nevermind, of course CI fails -- the tests don't pass with all the extra printfs in there. That will go away on its own when I remove those.

gwillen · 2019-05-03T01:12:05Z

apparently somehow I lost the compiler warning squashing in the rebase, but not because it was fixed upstream? So I added it back as its own commit.

Also fixed tests by removing all the stray printfs, none of them appeared worthy of upgrading to LogPrintf.

I think this should be ready for a real review.

gwillen · 2019-05-03T01:42:13Z

fixed travis complaints (coming from lints that don't run on my system because they have some python dependency I can't get to install.)

instagibbs · 2019-05-03T01:46:45Z

Linters are hard to replicate locally for me as well. Thanks.

…

On Thu, May 2, 2019, 9:42 PM gwillen ***@***.***> wrote: fixed travis complaints (coming from lints that don't run on my system because they have some python dependency I can't get to install.) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMAFU2D27IOK5VJPQOZAB3PTOJ7PANCNFSM4HJSS4DA> .

instagibbs · 2019-05-06T13:33:52Z

ok looks like everything but bitcoin compatibility test for psbt is working:

test_framework.authproxy.JSONRPCException: TX decode failed Duplicate Key, output asset_commitment already provided: iostream error (-22)

see: test/bitcoin_functional/functional/rpc_psbt.py

gwillen · 2019-05-06T18:34:38Z

Hm, interesting. It only fails on a single platform, which makes me worry it's nondeterministic. Also, I didn't realize those compatibility tests existed, so I wasn't running them -- I will try and see what it does locally.

ALSO, I think I'm still waiting for you to help me figure out where to condition on g_con_elementsmode. So assuming the compatibility tests run without that, that may fix it?

gwillen · 2019-05-20T17:53:14Z

@instagibbs formal reminder that I am waiting for your review on this (and guidance on where to put g_con_elementsmode checks)

instagibbs · 2019-05-20T17:54:58Z

@stevenroose promised a review Soon(TM), I think after his next catchup PR.

gwillen · 2019-05-29T01:52:40Z

@stevenroose @instagibbs piiiing? :-) I would really like to get this cleaned up and in. I definitely still need help sorting out what g_con_elementsmode is for, and where it should fit in (to make the bitcoin backwards-compat tests pass, which still fail at present.)

instagibbs · 2019-05-29T13:00:57Z

please add tests for walletcreatefundedpsbt, I don't think it works correctly. It's adding the old hack-style nonce commitments as pubkeys and not filling out the blinding pubkeys as psbt fields which makes blinding impossible.

gwillen · 2019-06-25T08:37:39Z

Ok so, belated comment on your last comment @instagibbs -- there already were tests for walletcreatefundedpsbt, but nothing is actually testing that things that should be blinded are, and nothing is testing that nonces don't show up where they shouldn't.

I have fixed walletcreatefundedpsbt and converttopsbt, and tested by hand, and the tests pass; but I haven't gone back and made the tests check that expected blinding is actually happening.

I expect that the code itself should be done at this point, though -- can you take another look?

instagibbs · 2019-06-26T19:04:22Z

src/rpc/rawtransaction.cpp

            "blindpsbt \"psbt\" ( ignoreblindfail )\n"
-            "\nUse the blinding data from the PSBT inputs to generate the blinding data for the PSBT outputs.\n"
+            "\nUse the blinding data from the PSBT inputs to generate the blinding data for the PSBT outputs.\n\n"
+            "TODO: Not expected to work on issuance/reissuance/peg transactions yet.\n"


I'd rather not have a TODO in help text :) Just telling the user it isn't supported is enough

Yeah fair. Fixing.

instagibbs · 2019-06-26T19:05:10Z

src/rpc/rawtransaction.cpp

+        // At this point, if the nonce field is present it should be a smuggled
+        //   pubkey, and not a real nonce. Convert it back to a pubkey and strip
+        //   it out.
+        psbtx.outputs[i].blinding_pubkey = CPubKey(tx.vout[i].nNonce.vchCommitment);


do we want to sanity check it's a IsFullyValid key?

Leaving this per discussion.

instagibbs · 2019-06-26T19:06:34Z

src/wallet/rpcwallet.cpp

+    // If we're signing, check that the transaction is not still in need of blinding
+    if (sign) {
+        for (const PSBTOutput& o : psbtx.outputs) {
+            if (o.blinding_pubkey.IsValid()) {


I usually prefer IsFullyValid()

Leaving this per discussion.

instagibbs · 2019-06-26T19:08:21Z

test/functional/rpc_psbt.py

 import json
 import os

+import IPython


whoop lol

was using this for debugging. Removed.

gwillen · 2019-07-02T03:35:46Z

Addressed comments, still have to debug the issue I uncovered with the walletcreatefundedpsbt change output coming out improperly unblinded.

gwillen · 2019-07-03T10:00:47Z

Sigh, ok, gave up on doing createfundedpsbt in (what I felt was) the nice way. The mystery of FundTransaction runs way too deep. Instead, I let FundTransaction do its thing, then extract the pubkeys from the nonces afterwards.

Still need to rebase on 0.18.

instagibbs · 2019-07-03T16:06:00Z

@gwillen right, seems to be the least painful thing to do. We let Core-ish stuff alone if it makes rebasing upstream easier.

gwillen · 2019-07-03T16:53:15Z

@instagibbs Heh... I didn't even think about the ease-of-rebase issue. ^^;

I am still working on the rebase onto 0.18. Currently the rpc_psbt.py test is erroring out immediately at the top of the very first test with "insufficient funds", which confuses me and I'm not sure what's up.

gwillen · 2019-07-27T03:21:53Z

Rebased onto master.

After a bunch of fixups, the errors left from Travis are (1) an apparent memory-exhaustion running tsan, and (2) a timeout. Not sure what's the best way to deal with those but they're not obviously something I can fix.

instagibbs · 2019-07-27T03:43:44Z

Sounds like the builds we haven't been able to get working from upstream. Will take a look Monday!

…

On Fri, Jul 26, 2019, 11:21 PM Glenn Willen ***@***.***> wrote: Rebased onto master. After a bunch of fixups, the errors left from Travis are (1) an apparent memory-exhaustion running tsan, and (2) a timeout. Not sure what's the best way to deal with those but they're not obviously something I can fix. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#600?email_source=notifications&email_token=ABMAFU2AXMAU4OFJHVEBQB3QBO5NJA5CNFSM4HJSS4DKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD26CQKQ#issuecomment-515647530>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMAFU3IIXQMRPRUBWZZO4TQBO5NJANCNFSM4HJSS4DA> .

test/functional/rpc_fundrawtransaction.py

src/wallet/rpcwallet.cpp

test/functional/rpc_psbt.py

src/psbt.h

src/rpc/rawtransaction.h

instagibbs · 2019-07-29T17:58:30Z

Please update the OP to list some of the common usage sequences for RPC.

Also an explicit list of the new fields in the description is probably apt.

instagibbs · 2019-07-29T18:07:50Z

oh could you also say what the PR isn't doing in the OP text? e.g., doesn't handle issuance/peg-ins.

gwillen · 2019-07-29T23:24:26Z

Updated the PR description.

gwillen · 2019-07-31T02:48:09Z

Addressed all comments. PTAL?

instagibbs · 2019-07-31T13:52:04Z

Changes look good, one more linter:

./test/functional/rpc_psbt.py:493:58: E703 statement ends with a semicolon
266^---- failure generated from test/lint/lint-python.sh

gwillen · 2019-07-31T15:35:25Z

Lol, it's really hard for me to not type semicolons at the end of lines. I blame C/C++ for this. Fixed. (One day I will get the python lints working on OS X.)

gwillen · 2019-07-31T17:38:53Z

Added a sentence of explanation of output_pubkeys_out in ConstructTransaction. I did leave it in because there's still once place I did use it.

… instead.

gwillen · 2019-07-31T17:51:14Z

Squashed down to a few sensible commits.

instagibbs · 2019-07-31T19:07:32Z

ACK

gwillen · 2019-07-31T19:08:11Z

Thanks @instagibbs! Been a long road for this one.

instagibbs · 2019-07-31T19:08:46Z

appreciate the heavy lifting!

apoelstra · 2020-12-14T15:21:58Z

src/rpc/rawtransaction.cpp

+
+    // How many are we trying to blind?
+    int num_pubkeys = 0;
+    unsigned int keyIndex = -1;


Flagged by ubsan for being an implicit signedness conversion (which is not UB but ubsan lints for it). But also this is literally UB because keyIndex is never checked against this -1 value, it's just used as an array index.

Should be fixed by #900, just noting here for posterity.

Oh, this block appears to have been copied from rawblindrawtransaction which has had this issue since at least #515

Oh, my bad, in both cases it is actually safe becasue keyIndex is only -1 if num_pubkeys is left at 0, and keyIndex is only used as an index when num_pubkeys is nonzero.

gwillen requested a review from instagibbs May 1, 2019 09:38

instagibbs reviewed May 1, 2019

View reviewed changes

gwillen force-pushed the feature-psbt-ca branch from ef7e009 to b6bb19a Compare May 2, 2019 01:14

gwillen force-pushed the feature-psbt-ca branch 2 times, most recently from d7a44e5 to 2921cb4 Compare May 3, 2019 00:21

gwillen force-pushed the feature-psbt-ca branch from 51ad0ab to 33fe9b4 Compare May 3, 2019 01:12

gwillen changed the title ~~WIP: First pass at PSBT for Confidential Assets~~ PSBT for Confidential Assets May 3, 2019

gwillen force-pushed the feature-psbt-ca branch from 33fe9b4 to e4b9d43 Compare May 3, 2019 01:41

gwillen force-pushed the feature-psbt-ca branch from e4b9d43 to 7f5496e Compare May 3, 2019 07:07

gwillen mentioned this pull request May 3, 2019

Define PSBT, Re-enable PSBT functional tests #504

Closed

gwillen force-pushed the feature-psbt-ca branch from 7f5496e to d6e9a99 Compare May 3, 2019 22:54

gwillen force-pushed the feature-psbt-ca branch from d6e9a99 to 20dae1e Compare June 25, 2019 08:24

instagibbs reviewed Jun 26, 2019

View reviewed changes

Add convenience method GetNonIssuanceBlindingData

f87684a

gwillen force-pushed the feature-psbt-ca branch 2 times, most recently from 5becc1e to c4f3605 Compare July 26, 2019 17:09

instagibbs reviewed Jul 29, 2019

View reviewed changes

gwillen added 3 commits July 31, 2019 10:50

PSBT for Confidential Assets

2848b52

Disable PSBT RPCs when not in g_con_elementsmode.

d39925b

Add legacy help text for walletprocesspsbt suggesting to use new RPCs…

3127c65

… instead.

gwillen force-pushed the feature-psbt-ca branch from fd5aed4 to 3127c65 Compare July 31, 2019 17:50

instagibbs merged commit 3127c65 into ElementsProject:master Jul 31, 2019

apoelstra reviewed Dec 14, 2020

View reviewed changes


		CMutableTransaction tx_tmp = tx; // We don't want to mutate the transaction in the PSBT yet, just extract blinding data

		// TODO(gwillen): Make this do something better than fail silently if there are any issuances, reissuances, pegins, etc.


		CMutableTransaction& tx = *psbtx.tx;

		// TODO(gwillen): Replace all this with the 'bonus output' scheme to use an OP_RETURN to balance blinders, with a rangeproof exponent of -1 (public).

PSBT for Confidential Assets #600

PSBT for Confidential Assets #600

Uh oh!

Conversation

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

Uh oh!

Uh oh!

instagibbs commented May 1, 2019

Uh oh!

instagibbs 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!

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!

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!

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

Uh oh!

Uh oh!

gwillen commented May 3, 2019

Uh oh!

gwillen commented May 3, 2019

Uh oh!

instagibbs commented May 3, 2019 via email

Uh oh!

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

Uh oh!

Uh oh!

gwillen commented May 6, 2019

Uh oh!

gwillen commented May 20, 2019

Uh oh!

instagibbs commented May 20, 2019

Uh oh!

gwillen commented May 29, 2019

Uh oh!

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

Uh oh!

Uh oh!

gwillen commented Jun 25, 2019

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

gwillen commented May 1, 2019 •

edited

Loading

gwillen commented May 2, 2019 •

edited

Loading

instagibbs commented May 6, 2019 •

edited

Loading

instagibbs commented May 29, 2019 •

edited

Loading