Skip to content

Switch default bits of hiding to 52 #834

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

Merged
stevenroose merged 3 commits into from
Mar 17, 2020
Merged

Switch default bits of hiding to 52 #834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
26e5490
Switch default bits of hiding to 52
instagibbs Dec 3, 2019
e87b390
Some readability improvements in blind.cpp
stevenroose Mar 16, 2020
c780a51
nit: Comment in blind_tests.cpp was wrong
stevenroose Mar 17, 2020
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
5 changes: 4 additions & 1 deletion src/blind.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,11 @@ bool GenerateRangeproof(std::vector<unsigned char>& rangeproof, const std::vecto
memcpy(asset_message+32, asset_blindptrs[asset_blindptrs.size()-1], 32);

// Sign rangeproof
int ct_exponent = std::min(std::max((int)gArgs.GetArg("-ct_exponent", 0), -1), 18);
Copy link
Contributor

@instagibbs instagibbs Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh concept ACK

Copy link
Contributor Author

@stevenroose stevenroose Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just taking the arguments out of the giant line. Nothing else changed. I don't fully understand the strange min max thing there. I suppose it's to get the values between -1 and 18.

Copy link
Contributor

@dgpv dgpv Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ct_bits was also previously limited via this min/max construction. It was changed to just 52, I don't see why ct_exponent in this line could not be changed to just 0. The min/max limit for ct_bits was removed in #793 without any explanation, was it deemed useless ?

Copy link
Contributor

@dgpv dgpv Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask because I'll need to adjust my lib accordingly, but I hesitate to remove the limitations without fully understanding the consequences. Changing the number of bits result in the change in the size of the proof, and it looks like this only affects the tx size and therefore fee estimation. Setting ct_bits outside sane range would mean the blinding will just not work and fail with error, is this change (removing min/max limitation) added to help detect incorrect settings rather than work with limited settings ? If this is the logic behind this change, then ct_exponent should not be min/max restricted, too, and then secp256k1_rangeproof_sign_impl will just return 0 if incorrect exp was supplied

Copy link
Contributor Author

@stevenroose stevenroose Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if your reference to #793 was a typo, but that PR was superseded with this one and it just sets the default number of bits from 38 to 52. We did this because the 38 bits covered only up to 10^-3 BTC or something. Because some assets are issuing assets using the BTC amounts instead of satoshi, we did this to increase the default blinding about up to 45*10^6 BTC.

It's true that it will about double the tx size. But given that we're also reducing the tx fee rate for Liquid to 0.1 sat/byte, that more than compensates for that increase in tx size.

Copy link
Contributor Author

@stevenroose stevenroose Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're probably right about the min(max( construction of ct_exponent. Any wrong number will result in a failure on the secp side. It might be worth it to remove that construction, yes.

Copy link
Contributor

@dgpv dgpv Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference to #793 was not a typo, the change of the value of ct_bits was not that my question was about. I asked why the std::min(std::max(... is still applied to ct_exponent, but not to ct_bits anymore. If there was certain logic behind removing this for ct_bits, the same logic should apply to ct_exponent. I don't see why this min/max thing is needed for either of these two parameters, but I might be missing something, and it would be good to know the reasoning why min/max was there in the first place. I have this code ported to python, and I will need to sync the code, but I would like the code to be consistent, and having different approaches to ct_exponent and ct_bits does not seem consistent to me.

Copy link
Contributor Author

@stevenroose stevenroose Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed that construction in followup PR. You were right that it was not needed in either position and it's actually a bit annoying.

int ct_bits = (int)gArgs.GetArg("-ct_bits", 52);
// If min_value is 0, scriptPubKey must be unspendable
int res = secp256k1_rangeproof_sign(secp256k1_blind_context, rangeproof.data(), &nRangeProofLen, scriptPubKey.IsUnspendable() ? 0 : 1, &value_commit, value_blindptrs.back(), nonce.begin(), std::min(std::max((int)gArgs.GetArg("-ct_exponent", 0), -1),18), std::min(std::max((int)gArgs.GetArg("-ct_bits", 36), 1), 51), amount, asset_message, sizeof(asset_message), scriptPubKey.size() ? &scriptPubKey.front() : NULL, scriptPubKey.size(), &gen);
uint64_t min_value = scriptPubKey.IsUnspendable() ? 0 : 1;
int res = secp256k1_rangeproof_sign(secp256k1_blind_context, rangeproof.data(), &nRangeProofLen, min_value, &value_commit, value_blindptrs.back(), nonce.begin(), ct_exponent, ct_bits, amount, asset_message, sizeof(asset_message), scriptPubKey.size() ? &scriptPubKey.front() : NULL, scriptPubKey.size(), &gen);
rangeproof.resize(nRangeProofLen);
return (res == 1);
}
Expand Down
4 changes: 2 additions & 2 deletions src/blind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
#include <secp256k1_surjectionproof.h>

//! ELEMENTS:
// 36-bit rangeproof size
static const size_t DEFAULT_RANGEPROOF_SIZE = 2893;
// 52-bit rangeproof size
static const size_t DEFAULT_RANGEPROOF_SIZE = 4174;
// 32 bytes of asset type, 32 bytes of asset blinding factor in sidechannel
static const size_t SIDECHANNEL_MSG_SIZE = 64;

Expand Down
2 changes: 1 addition & 1 deletion src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ void SetupServerArgs()
gArgs.AddArg("-feeasset=<hex>", strprintf("Asset ID (hex) for mempool/relay fees (default: %s)", defaultChainParams->GetConsensus().pegged_asset.GetHex()), false, OptionsCategory::CHAINPARAMS);
gArgs.AddArg("-subsidyasset=<hex>", strprintf("Asset ID (hex) for the block subsidy (default: %s)", defaultChainParams->GetConsensus().pegged_asset.GetHex()), false, OptionsCategory::CHAINPARAMS);
gArgs.AddArg("-initialreissuancetokens=<n>", "The amount of reissuance tokens created in the genesis block. (default: 0)", false, OptionsCategory::CHAINPARAMS);
gArgs.AddArg("-ct_bits", strprintf("The default number of hiding bits in a rangeproof. Will be exceeded to cover amounts exceeding the maximum hiding value. (default: %d)", 36), false, OptionsCategory::CHAINPARAMS);
gArgs.AddArg("-ct_bits", strprintf("The default number of hiding bits in a rangeproof. Will be exceeded to cover amounts exceeding the maximum hiding value. (default: %d)", 52), false, OptionsCategory::CHAINPARAMS);
gArgs.AddArg("-ct_exponent", strprintf("The hiding exponent. (default: %s)", 0), false, OptionsCategory::CHAINPARAMS);

// Add the hidden options
Expand Down
10 changes: 5 additions & 5 deletions src/test/blind_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,17 +265,17 @@ BOOST_AUTO_TEST_CASE(naive_blinding_test)

// Check wallet borromean-based rangeproof results against expected args
size_t proof_size = DEFAULT_RANGEPROOF_SIZE;
BOOST_CHECK(tx4.witness.vtxoutwit[2].vchRangeproof.size() == proof_size);
BOOST_CHECK_EQUAL(tx4.witness.vtxoutwit[2].vchRangeproof.size(), proof_size);
secp256k1_context *ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY);
int exp = 0;
int mantissa = 0;
uint64_t min_value = 0;
uint64_t max_value = 0;
BOOST_CHECK(secp256k1_rangeproof_info(ctx, &exp, &mantissa, &min_value, &max_value, tx4.witness.vtxoutwit[2].vchRangeproof.data(), proof_size) == 1);
BOOST_CHECK(exp == 0);
BOOST_CHECK(mantissa == 36); // 36 bit default
BOOST_CHECK(min_value == 1);
BOOST_CHECK(max_value == 68719476736);
BOOST_CHECK_EQUAL(exp, 0);
BOOST_CHECK_EQUAL(mantissa, 52); // 52 bit default
BOOST_CHECK_EQUAL(min_value, 1);
BOOST_CHECK_EQUAL(max_value, 4503599627370496);
}
{
inputs.clear();
Expand Down
4 changes: 3 additions & 1 deletion test/functional/feature_confidential_transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,9 @@ def run_test(self):

# Send some bitcoin and other assets over as well to fund wallet
addr = self.nodes[2].getnewaddress()
self.nodes[0].sendtoaddress(addr, 5)
txid = self.nodes[0].sendtoaddress(addr, 5)
# Make sure we're doing 52 bits of hiding which covers 21M BTC worth
assert_equal(self.nodes[0].getrawtransaction(txid, 1)["vout"][0]["ct-bits"], 52)
self.nodes[0].sendmany("", {addr: 1, self.nodes[2].getnewaddress(): 13}, 0, "", [], False, 1, "UNSET", {addr: test_asset})

self.sync_all()
Expand Down