Switch default bits of hiding to 52 #834

stevenroose · 2020-03-16T17:43:26Z

Replaces #793 and is basically that MR but rebased. (I can't rebase on Greg's branch.)

instagibbs · 2020-03-16T20:53:42Z

src/blind.cpp

@@ -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);


heh concept ACK

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.

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 ?

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

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.

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.

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.

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

apoelstra · 2020-03-17T13:09:37Z

utACK

jonasnick

Didn't check DEFAULT_RANGEPROOF_SIZE, otherwise ACK mod nit.

src/test/blind_tests.cpp

src/blind.cpp

jonasnick · 2020-03-17T17:07:39Z

Note that we should urge users to update. With this change and the default minfee/fallbackfee changes, wallet fingerprinting is trivial.

stevenroose · 2020-03-17T17:29:08Z

Yeah I'm sure Allen is taking care of that. Plus we're coordinating with Green to update at the same time.

In line with elements update ElementsProject/elements#834 Also removed the min/max construct, again in line with elements

stevenroose · 2020-03-25T13:57:31Z

I got an e-mail with this comment from Dmitry, but I can't find it on GitHub, was it removed?

What were a reason to choose CT_BITS=52 instead of 51, given that 51 bits

also cover amounts up to MAX_MONEY (2251799813685248), you can't have outputs > MAX_MONEY in Elements, and MAX_MONEY is the same as in Core, 21000000 * COIN ? Well, the reason to do this was not really to cover all Bitcoins. We won't ever have that many Bitcoins in Liquid anyway. But the reason was that people are issuing assets with bitcoin-like amounts (with 8 decimals). So we needed to update to a high number to cover tokens with high amounts of issuance. And we opted for 52 instead of 51 because it's a multiple of 4 and that made it easier to reason about the rangeproof size, because part of it grows with the bits divided by 4, so 52 made more sense.

…

On Wed, Mar 25, 2020 at 1:12 PM Dmitry Petukhov ***@***.***> wrote: What were a reason to choose CT_BITS=52 instead of 51, given that 51 bits also cover amounts up to MAX_MONEY (2251799813685248), you can't have outputs > MAX_MONEY in Elements, and MAX_MONEY is the same as in Core, 21000000 * COIN ? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#834 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGQLXGQSIEKGY4VRFXMB5DRJH7MNANCNFSM4LMPN7QQ> .

dgpv · 2020-03-25T14:10:48Z

I removed it because I got an answer (for a related question) from @instagibbs here: ElementsProject/elementsproject.github.io#127 (comment)

dgpv · 2020-03-25T14:12:28Z

Thanks for an explanation of the reasoning behind the decision!

@dgpv

…ment edfa40e Remove the min(max( construction for the ct_exponent argument (Steven Roose) Pull request description: As per @dgpv's suggestion here: #834 (comment) @dgpv could you review? Tree-SHA512: 6c91a0a86a593f22ff42fb617aa96aeb97e2e5225e2b5b6980aa470f45ee613dab53de49dbbae20aecf7a4bf21cc005839be770917d55cfb2c9afd821c2d0231

In line with elements update ElementsProject/elements#834 Also removed the min/max construct, again in line with elements

Hadta bump the fee test in feature_confidential_transactions.py since our default rangeproofs are now bigger.

stevenroose force-pushed the 51-bits branch from 48235ea to e87b390 Compare March 16, 2020 20:52

instagibbs reviewed Mar 16, 2020

View reviewed changes

instagibbs and others added 2 commits March 16, 2020 20:55

Switch default bits of hiding to 52

26e5490

Some readability improvements in blind.cpp

e87b390

stevenroose changed the title ~~Switch default bits of hiding to 51~~ Switch default bits of hiding to 52 Mar 16, 2020

stevenroose mentioned this pull request Mar 17, 2020

Switch default bits of hiding to 51 #793

Closed

jonasnick reviewed Mar 17, 2020

View reviewed changes

src/test/blind_tests.cpp Outdated Show resolved Hide resolved

src/blind.cpp Show resolved Hide resolved

nit: Comment in blind_tests.cpp was wrong

c780a51

stevenroose merged commit 95602ec into ElementsProject:master Mar 17, 2020

This was referenced Mar 24, 2020

features/confidential-transactions shows values for CT_BITS=32, but now CT_BITS is 52 ElementsProject/elementsproject.github.io#126

Closed

maximum confidential amount adjustment ElementsProject/elementsproject.github.io#127

Merged

stevenroose mentioned this pull request Mar 24, 2020

Remove the min(max( construction for the ct_exponent argument #840

Merged

jkauffman1 added a commit to Blockstream/gdk that referenced this pull request Mar 24, 2020

Increase rangeproof ct bits from 36 to 52

11c1849

In line with elements update ElementsProject/elements#834 Also removed the min/max construct, again in line with elements

greenaddress pushed a commit to Blockstream/gdk that referenced this pull request Mar 27, 2020

Increase rangeproof ct bits from 36 to 52

010a6d5

In line with elements update ElementsProject/elements#834 Also removed the min/max construct, again in line with elements

stevenroose pushed a commit that referenced this pull request Mar 17, 2021

Merge 95602ec into merged_master (Elements PR #834)

fc64d8f

Hadta bump the fee test in feature_confidential_transactions.py since our default rangeproofs are now bigger.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch default bits of hiding to 52 #834

Switch default bits of hiding to 52 #834

stevenroose commented Mar 16, 2020

instagibbs Mar 16, 2020

stevenroose Mar 16, 2020

dgpv Mar 23, 2020

dgpv Mar 23, 2020

stevenroose Mar 23, 2020

stevenroose Mar 23, 2020

dgpv Mar 23, 2020 •

edited

Loading

stevenroose Mar 24, 2020

apoelstra commented Mar 17, 2020

jonasnick left a comment

jonasnick commented Mar 17, 2020

stevenroose commented Mar 17, 2020

stevenroose commented Mar 25, 2020 via email

dgpv commented Mar 25, 2020 •

edited

Loading

dgpv commented Mar 25, 2020

Switch default bits of hiding to 52 #834

Switch default bits of hiding to 52 #834

Conversation

stevenroose commented Mar 16, 2020

instagibbs Mar 16, 2020

Choose a reason for hiding this comment

stevenroose Mar 16, 2020

Choose a reason for hiding this comment

dgpv Mar 23, 2020

Choose a reason for hiding this comment

dgpv Mar 23, 2020

Choose a reason for hiding this comment

stevenroose Mar 23, 2020

Choose a reason for hiding this comment

stevenroose Mar 23, 2020

Choose a reason for hiding this comment

dgpv Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

stevenroose Mar 24, 2020

Choose a reason for hiding this comment

apoelstra commented Mar 17, 2020

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick commented Mar 17, 2020

stevenroose commented Mar 17, 2020

stevenroose commented Mar 25, 2020 via email

dgpv commented Mar 25, 2020 • edited Loading

dgpv commented Mar 25, 2020

dgpv Mar 23, 2020 •

edited

Loading

dgpv commented Mar 25, 2020 •

edited

Loading