Fixing issuance cases and half blinded cases in PSET#1145
Fixing issuance cases and half blinded cases in PSET#1145psgreco merged 1 commit intoElementsProject:masterfrom
Conversation
| txin.assetIssuance.assetBlindingNonce = input.m_issuance_blinding_nonce; | ||
| txin.assetIssuance.assetEntropy = input.m_issuance_asset_entropy; | ||
| if (input.m_issuance_value != std::nullopt && input.m_issuance_inflation_keys_amount != std::nullopt && force_unblinded) { | ||
| // If there is a commitment we should set the value to the commitment unless we are forcing unblinded. |
There was a problem hiding this comment.
This code handled blinded vs. unblinded issuances badly. It always tried to blind the issuances, even if they were unblinded issuance assets.
Since there are no fields in the PSET spec to define if an issuance should be blinded or not, we will use the presence of the issuance blinding commitment to tell us what to do.
There was a problem hiding this comment.
Not sure is related, but it seems very similar?
There was a problem hiding this comment.
This looks similar that it may solve the problem. The use case I was trying to solve was an unblinded re-issuance.
| txin.assetIssuance.nAmount = input.m_issuance_value_commitment; | ||
| } else if (input.m_issuance_value) { | ||
| txin.assetIssuance.nAmount.SetToAmount(*input.m_issuance_value); | ||
| } else { |
There was a problem hiding this comment.
These variables were never cleared, so they retained the value of the previous loop iteration, which resulted in problems when there were multiple inputs beyond the first issuance, as well as problems when reissuing assets (where there are no inflation keys created).
| @@ -531,12 +544,9 @@ bool PSBTOutput::Merge(const PSBTOutput& output) | |||
| CTxOut PSBTOutput::GetTxOut() const | |||
| { | |||
There was a problem hiding this comment.
This would fail on situations where an asset was blinded but a value was not. I simplified the code to ensure that a value or commitment was checked in the assert (note: this will still crash if you don't provide either, maybe we should't assert?). Then it will use whatever combination of what is given.
allenpiscitello
force-pushed
the
pset-fixes
branch
from
cf71aa5 to
5954b10
Compare
|
Tests pass after the assert fix, @apoelstra can you re-ack, even after I merge this? |
|
re-utACK 5954b10 |
…or rc4 85c25b1 Bump version to -rc4 (Pablo Greco) bb684f2 Update manpages (Pablo Greco) 38e9490 Elements-qt: Fix arrows to increase/decrease Amount (Andrea Bonel) 5954b10 Fixing issuance cases and half blinded cases in PSET (Allen Piscitello) 707cf20 Fix tapscript comment (roconnor-blockstream) 7fb99ff Fix Icon position in dmg (Pablo Greco) Pull request description: Backport #1140 #1141 #1145 and #1146 from master. Fix manpages Bump to -rc4 ACKs for top commit: delta1: utACK 85c25b1 Tree-SHA512: 78a30bec870530275d103b6cab630ebf96b2df39fd03fc426bb16b6743ce9311f7e7128e43bf7a6452d261377e6ac017d61b77cd9ad4e4c20040629ae20e13b4
| } | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
Prefer to have this as the if, with the existing code in the else. As it is its difficult to see what if this else relates to when reviewing. i.e.
if (!blind_issuance) {
input_asset_blinders.emplace_back();
} else {
There was a problem hiding this comment.
Also all of your else statements (here and elsewhere) are on new lines which is inconsistent with the rest of the code. Please use
} else {
Instead of
}
else {
Here and in the other changes.
There was a problem hiding this comment.
for code formatting/style wouldn't be better to rely on a GitHub action that enforces shared rules automatically?
There was a problem hiding this comment.
That would make the code harder to rebase over upstream core changes, unfortunately.
5 participants
A few minor cases fixed in PSET-related code. Caused either a crash or invalid transactions created in some cases.