Redesign PSET #900

achow101 · 2020-08-13T18:30:31Z

The original PSET had some issues with leaking private information and there were issues with the design of the various roles. This PR replaces it with a new PSET design that should be able to provide all of the things that people expect PSETs to do while preserving privacy.

Additionally, an actual specification for this new design has been written up. It describes all of the new fields, their serialization, and how roles should handle the PSET data.

This is still a work in progress as not everything with the blinder role has been worked out yet.

jgriffiths · 2020-08-15T01:39:55Z

doc/pset.mediawiki

+** Key: A 32 Scalar Offset computed by a blinder.
+*** <tt>{0x00}|{offset}</tt>
+** Value: None. The value must have a length of zero.
+


Should be PSET_GLOBAL_SCALAR

32 byte

The scalar value should be stored in the value (not the key) since that makes the key unique and prevents duplicates even in general PSBT utilities that pass this field through. It also then matches every other Elements type in the doc.

There can be multiple scalars. Putting it in the value then requires a unique key per scalar.

The original suggestion was to attach a scalar to an output, however they aren't really related to a particular output, so I opted to go with a global field.

Ah. This wasn't clear from the docs. given multiple scalars in a psbt, how do i know in which context to use one versus another? It seems like more data than just the scalar value will be needed?

The scalars are only used by the last blinder and they are used all together at that time. They're just used in the last blinding step to make sure that everything balances.

jgriffiths · 2020-08-15T01:44:33Z

doc/pset.mediawiki

@@ -21,6 +20,9 @@ signatures for an input while the input does not have a complete set of signatur
 The signer can be offline as all necessary information will be provided in the
 transaction.

+This is a modification of BIP 174 for the Elements sidechain. It includes changes necessary
+for Confidential Transactions, Confidential Assets, and Peg-in transactions.
+


Please consider making this doc a diff/addendum to the BIP rather than repeating the PSBT fields etc. Its hard to read, easy to miss the Elements specific stuff and it will only get out of date w.r.t. BIP 174 as its updated.

If you cant do this at least highlight the Elements changes in the markup somehow.

Commit 0b3b9ed is just the PSET changes.

The people who will be implementing support for PSET in their code have to use the full document, they cannot use commit as the reference. I concur with @jgriffiths that the doc should be an addendum rather than edit. I think that the described problems (need to read diff PSET/PSBT instead of the document, need to sync with PSBT as it is updated) are significant. It is the additions to the standard that are proprietary, not the standard itself. Therefore only the additions should be described in the spec.

*proprietary in a sense that they use the PSBT_PROPRIETARY type designed for proprietary extensions, not that the changes are somehow not open.

doc/pset.mediawiki

jgriffiths · 2020-08-15T01:52:18Z

doc/pset.mediawiki

+The amount is added to the PSET input as <tt>PSET_IN_ISSUANCE_VALUE</tt>.
+
+The Creator decides whether and which inputs are Peg-in inputs.
+Peg-in inputs must have the Peg-in flag set in their outpoints.


issuance and pegin are mutually exclusive, no? if so this should be documented above with their input fields.

I am under the impression that they are not mutually exclusive. I don't really see why they would be.

doc/pset.mediawiki

+The blinder will then subtract the sum of their inputs' blinding factors from the sum of their output's blinding factors.
+The result must be added as a <tt>PSET_GLOBAL_SCALAR</tt> field.
+
+For the Blinder who blinds the last output to be blinded, it will subtract all of the scalar offsets from the value blinding factor for the last output.


doc/pset.mediawiki

+
+For the Blinder who blinds the last output to be blinded, it will subtract all of the scalar offsets from the value blinding factor for the last output.
+The result will be the amount blinding factor for that last output. The creation of the asset commitment, proofs, and other fields proceeds as usual.
+Once all outputs are blinded, all <tt>PSET_GLOBAL_SCALAR</tt> fields must be removed from the PSET.


doc/pset.mediawiki

dgpv · 2020-09-07T08:36:08Z

src/psbt.cpp

        if (!input.IsSane()) return false;
+        // Check issuance is empty in CTxOut but set in input


This comment is confusing. The "Check issuance is empty in CTxOut" happens later, but it is referred to as it it happens just here. I think better wording would be along the lines of "Check that issuance is empty in the input, because later we check that issuance is empty in CTxOut"

dgpv · 2020-09-07T08:38:19Z

src/psbt.cpp

+            if (!tx->vin[i].assetIssuance.nAmount.IsNull()) return false;
+            if (!input.issuance_value && input.issuance_value_commitment.IsNull()) return false;
+        }
+        // Check whether any input is blinded


The comment is at the end of the loop. Usually the comment describes the code that is below (or left of) the comment. Does this comment refer to the code that is supposed to be here, but removed ?

dgpv · 2020-09-07T08:55:43Z

src/psbt.cpp

+
+bool PSBTOutput::IsBlinded() const
+{
+    return blinding_pubkey.IsValid();


Nor IsPartiallyBlinded neither IsFullyBlinded check blinding_pubkey.IsValid(). That means that there is a logical contradiction that an output can be 'FullyBlinded', but not 'Blinded'. This can create confusion that can result in mistakes. Maybe better wording can help, like 'Blindable' instead of 'Blinded' ? Or IsPartiallyBlinded/IsFullyBlinded should imply IsBlinded

Enforced it.

dgpv · 2020-09-07T08:58:03Z

src/psbt.cpp

@@ -329,6 +402,7 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti
        result.witness.vtxinwit[i].scriptWitness = psbtx.inputs[i].final_script_witness;
        PSBTInput& input = psbtx.inputs[i];

+        /*


Commented-out (and not removed) code without any further comment with justification of not removing it can be confusing and distracting. Is this commented out just temporary ?

Yes, it is just temporary. The comment is to just make the code compile so that the action that this code does can be reimplemented later in this PR.

dgpv · 2020-09-07T09:08:47Z

src/psbt.h

@@ -174,49 +159,83 @@ struct PSBTInput
            SerializeToVector(s, final_script_witness.stack);
        }

+        // Issuance value
+        // We shouldn't have both the value and value commitment, but maybe we do


The comments should make the code more clear, rather than introduce doubt.

"but maybe we do" is not connected to the action in the code, and should either be dropped, or expanded, "but maybe we do, and in this case we ignore the value"

dgpv · 2020-09-07T09:13:22Z

src/psbt.h

-                SerializeToVector(s, asset_commitment);
-            }
+        // Asset
+        // We shouldn't have both asset and asset commitment, but if we do, wrte only the asset commitment


mistype: "wrte" instead of "write"

dgpv · 2020-09-08T06:10:28Z

doc/pset.mediawiki

+* Type: Value Commitment <tt>PSET_OUT_VALUE_COMMITMENT = 0x01</tt>
+** Key: None. The key must only contain the 1 byte type.
+*** <tt>{0x01}</tt>
+** Value: The 33 byte Value Commitment for this output. Adding this field requires the removal of <tt>PSET_OUT_VALUE</tt>.


Adding PSET_OUT_VALUE requires remove of commitment, too. I think better say that the fields are mutually exclusive, as said for issuance value/commitment fields. (the same can be said for asset and asset commitment)

dgpv · 2020-09-08T06:11:18Z

doc/pset.mediawiki

+will still need to handle events where keys are duplicated when combining transactions with duplicated fields. In this event, with the exception of duplicate output commitments and proofs, the software may choose
+whichever value it wishes.<ref>'''Why can the values be arbitrarily chosen?''' When there are duplicated keys, the values that can be chosen will either be
+valid or invalid. If the values are invalid, a signer would simply produce an invalid signature and the final transaction itself would be invalid. If the
+values are valid, then it does not matter which is chosen as either way the transaction is still valid.</ref>. For <tt>PSBT_OUT_VALUE_COMMITMENT</tt>, <tt>PSBT_OUT_ASSET_COMMITMENT</tt>, <tt>PSBT_OUT_VALUE_RANGEPROOF</tt>, <tt>PSBT_OUT_ASSET_SURJ_PROOF</tt>, and <tt>PSBT_OUT_ECDH_PUBKEY</tt>, the PSET should be considered invalid if these are duplicated.


I think what is meant here is that 'the combined PSET should be considered invalid'. Wouldn't it be more clear to say that PSETs cannot be combined if these fields are present in both instances ?

Also I believe that if values of these fields are equal for the outputs of two PSETs, they can be combined. For example, two PSETs that was modified from one common ancestor could have these fields in outputs that are equal.

I've tried to clarify and generalize this.

dgpv · 2020-09-08T07:40:39Z

doc/pset.mediawiki

+For any output which is going to be blinded, the Creator must add the blinding pubkey in the <tt>PSET_OUT_BLINDING_PUBKEY</tt> field.
+
+If any input is blinded, at least one of the outputs must also be blinded, i.e. it must have a <tt>PSET_OUT_BLINDING_PUBKEY</tt>.
+Such an output can be a 0 value OP_RETURN output.


Creator can be unaware that Updater will add a blinded input afterwards. Then the updater have to enforce this rule, too.

That would violate the constraint that the unsigned tx cannot change. Maybe we should assume (or stipulate) that creators know which/whether inputs are blinded?

dgpv · 2020-09-08T07:44:06Z

doc/pset.mediawiki

+
+The Creator decides whether and which inputs are issuance inputs.
+For those inputs, the issuance data must be added to the input in the global unsigned transaction, except for the issuance amount.
+Such inputs must also have the issuance flag set in the outpoint.


I would add "in the outpoint of the prevout". Just "outpoint" can be mistakenly read as "output" and confuse the reader (I myself misread and was confused, like "how an output can be related here at all ?")

dgpv · 2020-09-08T08:17:43Z

doc/pset.mediawiki

+** Key: None. The key must only contain the 1 byte type.
+*** <tt>{0x09}</tt>
+** Value: The Peg-in witness for the Peg-in Transaction.
+*** <tt>{script}</tt>


There should be PSET_IN_ASSET_COMMITMENT to supply asset commitment of the prevout, so Creator/Updater can provide blinding information for the inputs when they will not be the Blinder

(this info is supplied in auxiliary_generators argument to BlindTransaction())

This should be in the UTXO already.

dgpv · 2020-09-08T08:44:31Z

doc/pset.mediawiki

+Once all outputs are blinded, all <tt>PSET_GLOBAL_SCALAR</tt> fields must be removed from the PSET.
+
+A single entity is likely to be a Creator, Updater, and Blinder.
+In that case, the PSET should never be output with <tt>PSET_IN_ISSUANCE_VALUE</tt>, <tt>PSET_OUT_VALUE</tt>, or <tt>PSET_OUT_ASSET</tt>.


Creator must set output values to 0 and assets to NULL. So for unblinded outputs, PSET_OUT_VALUE and PSET_OUT_ASSET need to remain after blinding. Or there should be a rule that unblinded output values should have their value/asset specified in embedded transaction, and these PSET_* fields used only for blindable outputs.

I clarified this to say that unblinded outputs should have these fields.

dgpv · 2020-09-08T08:50:28Z

doc/pset.mediawiki

+The resulting PSBT must contain all of the key-value pairs from each of the PSBTs.
+The Combiner must remove any duplicate key-value pairs, in accordance with the specification. It can pick arbitrarily when conflicts occur.
+For all PSET output fields, the combiner must fail to combine if these fields are not identical.
+If the PSETs to be combined have incomplete blinded outputs but the combined result would only complete blinded outputs, then the Combiner must fail unless it is also a Blinder and can reblind its outputs.


This wording is unclear. Does it says that if the two outputs that are combined are both incomplete-blinded, but the resulting output will have all the fields to consider it complete, there is a big chance that these fields are incompatible because different randomness was used to produce them ?

dgpv · 2020-09-10T13:22:08Z

doc/pset.mediawiki

+** Value: The rangeproof for the value of this output.
+*** <tt>{rangeproof}</tt>
+
+* Type: Asset Surjection Proof <tt>PSET_OUT_ASSET_SURJ_PROOF = 0x05</tt>


if RANGEPROOF is spelled without underscore, it would be more consistent to spell SURJPROOF or SURJECTIONPROOF also without underscore. Or use RANGE_PROOF instead.

I personally prefer PSET_OUT_VALUE_RANGE_PROOF and PSET_OUT_ASSET_SURJECTION_PROOF

Although in the Elements code they are referred to as rangeproof and surjectionproof, without underscores.

Also 'value' and 'asset' prefixes can be implicit in these names

In the various papers on CT and CA, rangeproof as one word is the typical usage while surjection proof is 2 words. So following that convention, it would be RANGEPROOF and SURJECTION_PROOF.

achow101 · 2020-09-15T20:53:39Z

I've updated pset.mediawiki to just be PSET and not contain PSBT.

dgpv · 2020-09-16T07:30:59Z

I'd like to note that I find PSET prefix confusing if used in implementation (not so much in the specification) that will most likely be based on PSBT. Unless BIP174 is used in prefix for the names rather than PSBT, I would say that it is better to use PSBT_Elements, PSBT_ELTS and the like prefixes for the names, and say that 'PSBT' stands for 'partially-signed bitcoin-like transaction'.

dgpv · 2020-09-16T07:34:32Z

Especially when used along with PSBT_-prefixed names, PSET_ will cause frequent pain due to mixups, especially because B and E are so close visually.

dgpv · 2020-09-18T06:22:45Z

In the various papers on CT and CA, rangeproof as one word is the typical usage while surjection proof is 2 words. So following that convention, it would be RANGEPROOF and SURJECTION_PROOF.

Implementors are more likely to look into reference code than the papers, and more likely to get the conventions from there. Elements code mainly uses no-underscore convention.

elements$ git grep -i surjectionproof|wc -l
303
elements$ git grep -i surjection_proof|wc -l
38
elements$ git grep -i rangeproof|wc -l
414
elements$ git grep -i range_proof|wc -l
16

walletfillpsbtdata These are all done by walletprocesspsbt

This test is for a condition that no longer matters. Prior to CA, there was only the single value blinder for a transaction that sent unconf to conf. This would necessitate setting that single blinding factor to 0, which would make the single output not actually blinded. But with CA, because each output has an additional blinding factor for the asset, this issue no longer exists.

Helper function for getting a CTxOut for a PSBTOutput

It doesn't work, disable for now.

apoelstra · 2021-06-24T21:08:55Z

utack e3a4081

I need an hour or two to build-test all the commits and run some tests on the tip, and we need to merge #1012 to fix CI, then we should be good to go!

apoelstra · 2021-06-25T23:43:08Z

ack f8553c5

apoelstra · 2021-06-25T23:43:27Z

CI failure due to #1012

…suances.py Should describe the outputs as an array rather than as an object. (The old behavior has long been deprecated but was eliminated entirely in ElementsProject#900.)

…ash_pegins_issuances.py 1c883db test: update `createrawtransaction` call in feature_taphash_pegins_issuances.py (Andrew Poelstra) Pull request description: Should describe the outputs as an array rather than as an object. (The old behavior has long been deprecated but was eliminated entirely in #900.) Fixes functional tests which were broken in master by simultaneous merge of #1002 and #900. ACKs for top commit: stevenroose: utACK 1c883db Tree-SHA512: f7963c34e7006a25ac5515e30966ef46777fa22d6125d219345731aef603e5f3179fc316134d59694af8e753f7cb48c825ad3434f2bceda0a10b8d5a52c32cd3

This fixes an undetected merge conflict between ElementsProject#1014 and ElementsProject#900.

…ew RPC changes c0936ba test: fix surjectionproof overflow functional test for new RPC changes (Andrew Poelstra) Pull request description: This fixes an undetected merge conflict between #1014 and #900. ACKs for top commit: gwillen: utACK c0936ba psgreco: UTACK c0936ba Tree-SHA512: a39d98db91d92f7e0e739e06d7a1bb13814d2bc7751cb6105a759af04ed0d61d206d1650f5e4e087feaeafcf1c482587b05b66d9cc497bc594a45df6bf23c07a

I hacked up rpc_fundrawtransaction.py a little bit because our `get_address` method here depends on nobody having called `createwallet` on a particular node ... the "correct" fix for this would be to redo the Elements changes to this functional test for the 0.20+ createwallet changes, but because there will be big future changes to this test (in Elements #900 (PSET)) I did the quick/hacky thing, and will defer cleanups to a separate post-rebase PR.

Surprisingly easy to do. Almost all of the diff resolution was mechanically * replacing boost::variant with std::variant * replacing Optional with std::optional * then replacing `nullopt` with `std::nullopt` * updating the RPC functions for the new RPCArg::Default type * update the tests/ directory to make new (since 22) tests use arrays for createrawtransaction outputs * other ad-hoc changes to function parameters etc (not too many of these) I had to "really" change the code in PrecomputePSBTData, which was introduced in 22.0 and affected by PSET, but this function was like 8 lines long so it was easy. Reviewing the diff may be a bit difficult because of the mix of mechanical changes and ad-hoc things. Probably the most straightforward thing to do is to redo the merge, `sed -i` to fix the boost::variant and Optional stuff, then diff the remaining conflicts against this commit. TODO: grep for `blindpsbt` and you will see that this RPC is still referenced in documentation and help text even though it was deleted. Need to fix this in 0.21 in a separate PR.

achow101 mentioned this pull request Aug 13, 2020

[wip] Implement redesigned PSET ElementsProject/libwally-core#217

Closed

jgriffiths reviewed Aug 15, 2020

View reviewed changes

doc/pset.mediawiki Outdated Show resolved Hide resolved

jgriffiths mentioned this pull request Aug 21, 2020

Elements psbt updates ElementsProject/libwally-core#218

Closed

achow101 force-pushed the pset branch from 0ae1e44 to cc6a862 Compare August 21, 2020 21:05

jgriffiths reviewed Aug 24, 2020

View reviewed changes

doc/pset.mediawiki Outdated Show resolved Hide resolved

doc/pset.mediawiki Outdated Show resolved Hide resolved

achow101 force-pushed the pset branch from cc6a862 to 78dac69 Compare August 25, 2020 18:35

dgpv reviewed Sep 7, 2020

View reviewed changes

dgpv reviewed Sep 8, 2020

View reviewed changes

dgpv reviewed Sep 10, 2020

View reviewed changes

achow101 force-pushed the pset branch from 78dac69 to 77d6fbc Compare September 15, 2020 20:53

dgpv mentioned this pull request Sep 16, 2020

Implement Partially-signed Elements transaction support Simplexum/python-elementstx#2

Open

achow101 force-pushed the pset branch from 993eda3 to b3be812 Compare September 17, 2020 22:19

achow101 force-pushed the pset branch from b3be812 to 317baef Compare October 8, 2020 20:04

achow101 force-pushed the pset branch from 317baef to 098a1ce Compare November 13, 2020 22:00

achow101 added 14 commits June 22, 2021 14:42

Remove walletsignpsbt, blindpsbt, walletblindpsbt, and

b1ebc81

walletfillpsbtdata These are all done by walletprocesspsbt

Only allow psbt_version 2

46ccf61

Update rpc_psbt.py for PSET

afd7950

Default to adding bip32 derivs

80ff739

Add PSBTOutput::GetTxOut()

b65a00f

Helper function for getting a CTxOut for a PSBTOutput

Enable analyzepsbt

64b30bf

Initialize optionals with nullopt

7533503

Allow specifying SIGHASH_RANGEPROOF

9fe6401

Check elements blinding field constraints during deser

4a44ff2

Use a constant for maximum surjection targets

4940cee

Enable converttopsbt for PSET

5425da4

Fix display of outputs in PSBT ops dialog

864f625

Disable joinpsbts

f8553c5

It doesn't work, disable for now.

achow101 force-pushed the pset branch from e3a4081 to f8553c5 Compare June 22, 2021 18:51

apoelstra merged commit 9a15459 into ElementsProject:master Jul 6, 2021

apoelstra mentioned this pull request Jul 9, 2021

test: update createrawtransaction call in feature_taphash_pegins_issuances.py #1017

Merged

apoelstra added a commit to apoelstra/elements that referenced this pull request Aug 31, 2021

test: fix surjectionproof overflow functional test for new RPC changes

c0936ba

This fixes an undetected merge conflict between ElementsProject#1014 and ElementsProject#900.

apoelstra mentioned this pull request Aug 31, 2021

test: fix surjectionproof overflow functional test for new RPC changes #1028

Merged

This was referenced Sep 5, 2021

createpsbt output_assets parameter not working #875

Closed

Add blinding role to PSET #879

Closed

gwillen mentioned this pull request Apr 12, 2022

Bring Elements up to date with Bitcoin Core 22.0 #1058

Closed

		if (!input.IsSane()) return false;
		// Check issuance is empty in CTxOut but set in input

Redesign PSET #900

Redesign PSET #900

Conversation

achow101 commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgpv Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgpv Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Sep 15, 2020

dgpv commented Sep 16, 2020

dgpv commented Sep 16, 2020 • edited Loading

dgpv commented Sep 18, 2020 • edited Loading

apoelstra commented Jun 24, 2021

apoelstra commented Jun 25, 2021

apoelstra commented Jun 25, 2021

dgpv Sep 7, 2020 •

edited

Loading

dgpv Sep 8, 2020 •

edited

Loading

dgpv commented Sep 16, 2020 •

edited

Loading

dgpv commented Sep 18, 2020 •

edited

Loading