-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changing PSET to not blind asset issuances by default. #1150
Changing PSET to not blind asset issuances by default. #1150
Conversation
…an option to blind or unblind issuances, defaulting to unblind.
@@ -386,7 +386,7 @@ BlindingStatus BlindPSBT(PartiallySignedTransaction& psbt, std::map<uint32_t, st | |||
} | |||
|
|||
// Handle issuances | |||
if (input.m_issuance_value) { | |||
if (input.m_issuance_value != std::nullopt || input.m_issuance_value_commitment.IsCommitment() || input.m_issuance_inflation_keys_amount != std::nullopt || input.m_issuance_inflation_keys_commitment.IsCommitment()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code missed cases where there was a re-issuance token but not an issuance. This is allowed, and should be accounted for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For complex/long-winded logic checks like this, it would be nice to have methods on the input such as:
bool PSBTInput::HasIssuance() const
{
if (m_issuance_value != std::nullopt || m_issuance_value_commitment.IsCommitment()) {
return true; // Blinded or unblinded issuance
}
if (m_issuance_inflation_keys_amount != std::nullopt || m_issuance_inflation_keys_commitment.IsCommitment()) {
return true; // Re-issuance token without an issuance
}
return false; // No issuance or re-issuance present
}
This would make the underlying logic much clearer IMO.
@@ -135,7 +135,7 @@ CMutableTransaction PartiallySignedTransaction::GetUnsignedTx(bool force_unblind | |||
txin.assetIssuance.nAmount.SetNull(); | |||
} | |||
if (input.m_issuance_inflation_keys_amount != std::nullopt && (input.m_issuance_inflation_keys_commitment.IsNull() || force_unblinded)) { | |||
txin.assetIssuance.nInflationKeys.SetToAmount(*input.m_issuance_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous fix introduced this bug.
@@ -1921,8 +1921,8 @@ BlindingStatus CWallet::WalletBlindPSBT(PartiallySignedTransaction& psbtx) const | |||
our_input_data[i] = std::make_tuple(amount, asset, asset_blinder, value_blinder); | |||
} | |||
|
|||
// Blind issuances on our inputs | |||
if (input.m_issuance_value || input.m_issuance_inflation_keys_amount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this logic in place even though blinding is removed, so that when the PSET spec is updated we can easily add blinding back.
src/blindpsbt.cpp
Outdated
memcpy(fixed_input_tags.back().data, issuance_asset.begin(), 32); | ||
ephemeral_input_tags.emplace_back(); | ||
if (input.m_issuance_value_commitment.IsNull()) { | ||
if (secp256k1_generator_generate(secp256k1_blind_context, &ephemeral_input_tags.back(), issuance_asset.begin()) != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe &ephemeral_input_tags.back()
should be ephemeral_input_tags.back().data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no, ephemeral_input_tags.back()
returns a secp256k1_generator
. The real bug here is that we ever touch .data
which is supposed to be a secp-zkp implementation detail, but that's a fight for another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ncorrect reissuance token ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not completely convinced that this is correct.
src/blindpsbt.cpp
Outdated
@@ -386,7 +386,7 @@ BlindingStatus BlindPSBT(PartiallySignedTransaction& psbt, std::map<uint32_t, st | |||
} | |||
|
|||
// Handle issuances | |||
if (input.m_issuance_value) { | |||
if (input.m_issuance_value != std::nullopt || input.m_issuance_value_commitment.IsCommitment() || input.m_issuance_inflation_keys_amount != std::nullopt || input.m_issuance_inflation_keys_commitment.IsCommitment()) { | |||
if (!input.m_issuance_value_commitment.IsCommitment() && input.m_issuance_rangeproof.size() == 0 && input.m_issuance_inflation_keys_rangeproof.size() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire block would be skipped if there is m_issuance_value_commitment
which seems wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for dealing with issuances/re-issuances should be same. If we are not doing it, most like a sign of bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I inspected the transaction it converted to a PSBT. The output for the reissuance token was that of the blinded reissuance token ID. This will cause a surjection proof failure because there is no input for the blinded reissuance token, only the unblinded one, when calling the PSBT version of this.
src/blindpsbt.cpp
Outdated
|
||
if (input.m_issuance_blinding_nonce.IsNull() && input.m_issuance_inflation_keys_amount) { | ||
if (input.m_issuance_blinding_nonce.IsNull() && (input.m_issuance_inflation_keys_amount != std::nullopt || input.m_issuance_inflation_keys_commitment.IsCommitment())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are we dealing with the case m_issuance_blinding_nonce
is not Null? Seems like this would not work correctly in re-issuance case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a reissuance case, there are no reissuance tokens being created, only the asset tokens. Those are handed in the section above.
@apoelstra , @allenpiscitello. I think the following fixups would fix this. sanket1729@6182dda. Feel free to cherry-pick/squash as you see fit and if my reasoning is correct :P
This should be easy to review if you directly compare it with validation logic in confidential_validation.cpp. Sorry if the code does not build, I do not have the machine to test building right now |
I believe this is incorrect as stated above. .
The intended behavior with this modification is to just create a transaction that would not be valid if you only specified one or the other as blinded. This transaction would be invalid if you mixed and matched it.
I believe this is wrong based on the comment above.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK dd2d758
…or rc5 37076d7 Bump version to -rc5 (Pablo Greco) 5629cae fs: Make compatible with boost 1.78 (Andrew Chow) 60b913e Elements-qt: Correctly display the amount in the sender's wallet after using Send button (Andrea Bonel) 872478b docs: describe elements transaction serialization format (#1148) (James Dorfman) dd2d758 fixed minor issues found in review (Allen Piscitello) 2da7d75 removing test that fails due to blinded issuances, which results in incorrect reissuance token ids (Allen Piscitello) 420de43 removing code to blind issuances. PSET should be modified to include an option to blind or unblind issuances, defaulting to unblind. (Allen Piscitello) Pull request description: Backport #1150 #1154 and #1148 from master. Backport bitcoin/bitcoin#24104 from Bitcoin Core Bump to -rc5 ACKs for top commit: jamesdorfman: utACK 37076d7. delta1: utACK 37076d7 Tree-SHA512: 616322f94c17008cc7fd582203c22b33c73b922269ab33fd13b270f491eda9b30d4f18efa3f7887e247bef46b63c4810f4084e409bea98120702c426a83343a8
PSET does not allow a user to specify if an issuance is to be blinded or not. The previous default was only allowing blinded, and it would blind the issuances if they were unblinded.
We should modify the PSET spec to allow specifying blinding or unblinded issuances, and in the meantime honor whatever is set in the PSET. If a commitment is set, use that, otherwise we should leave it unblinded.
Fixing this also uncovered some other minor bugs that were in previous fixes to PSET-related code.