Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Changing PSET to not blind asset issuances by default. #1150

Merged
merged 3 commits into from
Aug 25, 2022
Merged

Changing PSET to not blind asset issuances by default. #1150

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
420de43
removing code to blind issuances. PSET should be modified to include …
Aug 24, 2022
2da7d75
removing test that fails due to blinded issuances, which results in i…
Aug 24, 2022
dd2d758
fixed minor issues found in review
Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 24 additions & 67 deletions src/blindpsbt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor Author

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.

Copy link
Contributor

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.

if (!input.m_issuance_value_commitment.IsCommitment() && input.m_issuance_rangeproof.size() == 0 && input.m_issuance_inflation_keys_rangeproof.size() == 0) {
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

CAsset issuance_asset;
CAsset reissuance_asset;
Expand All @@ -400,83 +400,40 @@ BlindingStatus BlindPSBT(PartiallySignedTransaction& psbt, std::map<uint32_t, st
GenerateAssetEntropy(entropy, input.GetOutPoint(), input.m_issuance_asset_entropy);
}

// Asset isn't blinded yet. Add it to the list of input assets
CalculateAsset(issuance_asset, entropy);
fixed_input_tags.emplace_back();
memcpy(fixed_input_tags.back().data, issuance_asset.begin(), 32);
ephemeral_input_tags.emplace_back();
if (secp256k1_generator_generate(secp256k1_blind_context, &ephemeral_input_tags.back(), issuance_asset.begin()) != 1) {
return BlindingStatus::INVALID_ASSET;
if (input.m_issuance_value != std::nullopt || input.m_issuance_value_commitment.IsCommitment()) {
// Asset isn't blinded yet. Add it to the list of input assets
CalculateAsset(issuance_asset, entropy);
fixed_input_tags.emplace_back();
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) {
Copy link
Member

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.

Copy link
Member

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.

return BlindingStatus::INVALID_ASSET;
}
}
else {
memcpy(ephemeral_input_tags.back().data, input.m_issuance_value_commitment.vchCommitment.data(), 33);
}
input_asset_blinders.emplace_back();
}
unsigned int iss_to_blind = 1; // Always do the first issuance blinding iteration for the issuance value

bool blind_issuance = our_issuances_to_blind.count(i) > 0;
bool blind_issuance = input.m_issuance_inflation_keys_commitment.IsCommitment() || input.m_issuance_value_commitment.IsCommitment();

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())) {
Copy link
Member

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?

Copy link
Contributor Author

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.

// New issuance, do reissuance token things
CalculateReissuanceToken(reissuance_asset, entropy, blind_issuance);
// Add the reissuance_asset to the list of input assets
fixed_input_tags.emplace_back();
memcpy(fixed_input_tags.back().data, reissuance_asset.begin(), 32);
ephemeral_input_tags.emplace_back();
if (secp256k1_generator_generate(secp256k1_blind_context, &ephemeral_input_tags.back(), reissuance_asset.begin()) != 1) {
return BlindingStatus::INVALID_ASSET;
}
iss_to_blind++; // If we have a reissuance, do the second blinding iteration for the inflation keys
}

if (blind_issuance) {
for (unsigned int blind_i = 0; blind_i < iss_to_blind; ++blind_i) {
// To blind an issuance, both the issuance value and the number of inflation keys need to be blinded
// Since this process is basically the same for both, do it in a loop and switch based on the index
bool blind_value = blind_i == 0; // True for blinding the value, false for blinding the inflation keys
CAmount value = blind_value ? *input.m_issuance_value : *input.m_issuance_inflation_keys_amount;
CAsset asset = blind_value ? issuance_asset : reissuance_asset;
CKey blinding_privkey = blind_value ? our_issuances_to_blind.at(i).first : our_issuances_to_blind.at(i).second;

uint256 value_blinder;
GetStrongRandBytes(value_blinder.begin(), value_blinder.size());

// Create unblinded generator. Throw away everything except asset_gen
uint256 asset_blinder;
CConfidentialAsset conf_asset;
secp256k1_generator asset_gen;
CreateAssetCommitment(conf_asset, asset_gen, asset, asset_blinder);
input_asset_blinders.push_back(asset_blinder);

// Compute the scalar for this blinding and add to the input scalar
if (!ComputeAndAddToScalarOffset(input_scalar, value, asset_blinder, value_blinder)) return BlindingStatus::SCALAR_UNABLE;

// Create value commitment
secp256k1_pedersen_commitment value_commit;
CConfidentialValue conf_value;
CreateValueCommitment(conf_value, value_commit, value_blinder, asset_gen, value);

// Nonce is the blinding key
uint256 nonce = uint256(std::vector<unsigned char>(blinding_privkey.begin(), blinding_privkey.end()));

// Generate rangeproof
std::vector<unsigned char> rangeproof;
bool rangeresult = CreateValueRangeProof(rangeproof, value_blinder, nonce, value, CScript(), value_commit, asset_gen, asset, asset_blinder);
assert(rangeresult);

// Create explicit value rangeproofs
std::vector<unsigned char> blind_value_proof;
rangeresult = CreateBlindValueProof(blind_value_proof, value_blinder, value, value_commit, asset_gen);
assert(rangeresult);

if (blind_value) {
input.m_issuance_value_commitment = conf_value;
input.m_issuance_rangeproof = rangeproof;
input.m_blind_issuance_value_proof = blind_value_proof;
} else {
input.m_issuance_inflation_keys_commitment = conf_value;
input.m_issuance_inflation_keys_rangeproof = rangeproof;
input.m_blind_issuance_inflation_keys_proof = blind_value_proof;
if (input.m_issuance_inflation_keys_commitment.IsNull()) {
if (secp256k1_generator_generate(secp256k1_blind_context, &ephemeral_input_tags.back(), reissuance_asset.begin()) != 1) {
return BlindingStatus::INVALID_ASSET;
}
}
}
else {
else if(input.m_issuance_inflation_keys_commitment.IsCommitment()){
memcpy(ephemeral_input_tags.back().data, input.m_issuance_inflation_keys_commitment.vchCommitment.data(), 33);
}
input_asset_blinders.emplace_back();
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/psbt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

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.

txin.assetIssuance.nInflationKeys.SetToAmount(*input.m_issuance_inflation_keys_amount);
}
else if(!input.m_issuance_inflation_keys_commitment.IsNull()) {
txin.assetIssuance.nInflationKeys = input.m_issuance_inflation_keys_commitment;
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

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.

// Blind issuances on our inputs if at least one commitment was provided.
if (input.m_issuance_value_commitment.IsCommitment() || input.m_issuance_inflation_keys_commitment.IsCommitment()) {
CScript blinding_script(CScript() << OP_RETURN << std::vector<unsigned char>(input.prev_txid.begin(), input.prev_txid.end()) << *input.prev_out);
our_issuances_to_blind[i] = std::make_pair(GetBlindingKey(&blinding_script), GetBlindingKey(&blinding_script));
}
Expand Down