Skip to content

include: add description of range proofs#328

Merged
real-or-random merged 1 commit into
BlockstreamResearch:masterfrom
luckyNik:rangeproofs-description
Mar 2, 2026
Merged

include: add description of range proofs#328
real-or-random merged 1 commit into
BlockstreamResearch:masterfrom
luckyNik:rangeproofs-description

Conversation

@luckyNik
Copy link
Copy Markdown
Contributor

Added the description of range proofs in Confidential Assets focusing on the differences between the description in the paper and the actual implementation.

Copy link
Copy Markdown
Member

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Please take all my comments as suggestions, and feel free to modify and deviate. (I tend to be picky about writing... )

Comment thread include/secp256k1_rangeproof.h Outdated
Comment thread include/secp256k1_rangeproof.h Outdated
Comment thread include/secp256k1_rangeproof.h Outdated
Comment thread include/secp256k1_rangeproof.h Outdated
Comment thread include/secp256k1_rangeproof.h Outdated
@real-or-random
Copy link
Copy Markdown
Member

Did you mean to push an update? The branch is still at the original version.

@luckyNik luckyNik closed this Mar 2, 2026
@luckyNik
Copy link
Copy Markdown
Contributor Author

luckyNik commented Mar 2, 2026

Yes, sorry, thought I've pushed the changes.

P. S. I'm honestly not sure why did the PR close automatically.

@luckyNik luckyNik reopened this Mar 2, 2026
Copy link
Copy Markdown
Member

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

I like that version!

Now that we've figured out, meanwhile, that what goes into the hashes is also different from the paper (the malleability discussion), can you mention this difference, too?

Comment thread include/secp256k1_rangeproof.h Outdated
* embedded and recovered within maximally-sized proofs. The implemented embedding
* method using the forged parts of ring signatures could also be applied to the
* construction in the paper, but is not mentioned there.
* This feature is used in Confidential Assets to transmit values and blinding
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a nit:

Does this line start a new paragraph or not? (If yes, there should be an empty line. If no, the paragraph should be "realigned".)

Either is fine, but if you touch this anyway, I suggest replacing "This feature" by "The message embedding feature" or just "Message embedding" to make it clear what it refers to (because the preceding sentence talks about the relation to the paper instead.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Meant it like a single paragraph, so realigned, thanks. And replaced "this feature" with "message embedding".

@real-or-random
Copy link
Copy Markdown
Member

Forgot to mention this: can you also squash everything in a single commit?

@luckyNik luckyNik force-pushed the rangeproofs-description branch from acf0bd2 to da8d478 Compare March 2, 2026 15:13
Copy link
Copy Markdown
Member

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK acf0bd2

@real-or-random
Copy link
Copy Markdown
Member

Ah, sorry, one last thing:

There are a bunch of trailing spaces. Could you remove these? It's really not essential, but it's a bit cleaner to omit these, and some projects insist on omitting trailing whitespace, so it's a good opportunity to configure your editor to show these (or even remove automatically)

@luckyNik luckyNik force-pushed the rangeproofs-description branch from da8d478 to 6f7c112 Compare March 2, 2026 15:51
@luckyNik
Copy link
Copy Markdown
Contributor Author

luckyNik commented Mar 2, 2026

Ah, sorry, one last thing:

There are a bunch of trailing spaces. Could you remove these? It's really not essential, but it's a bit cleaner to omit these, and some projects insist on omitting trailing whitespace, so it's a good opportunity to configure your editor to show these (or even remove automatically)

Yep, no problem. Thanks for the hint, will check for those in the future.

Copy link
Copy Markdown
Member

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK 6f7c112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants