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.

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

[6.0.0] Conversion from Long-encoded nBits representation to BigInt and back #962

Open
wants to merge 28 commits into
base: v6.0.0
Choose a base branch
from

Conversation

kushti
Copy link
Member

@kushti kushti commented Apr 10, 2024

In this PR, two new methods, Global.encodeNbits() and Global.decodeNbits() are introduced, to support conversion from NBits-encoded numbers to big integers and back

Among new tests Bitcoin PoW check example can be found.

close #675

TODO: en/decodeNBits likely should work with UnsignedBigInt , which will be done in UnsignedBigInt PR, or during merging this into v6.0.0 with UnsignedBigInt impl being merged, depending on merging order

@aslesarenko
Copy link
Member

@kushti, I suggest to add these operations as methods of SGlobal (i.e. in SGlobalMethods)

@kushti kushti changed the base branch from develop to v6.0.0 June 4, 2024 19:38
@kushti kushti changed the title [6.0.0] Long.decodeNbits (nbits decoding support) [6.0.0] Conversion from Long-encoded nBits representation to BigInt and back Jul 31, 2024
@kushti
Copy link
Member Author

kushti commented Jul 31, 2024

@kushti, I suggest to add these operations as methods of SGlobal (i.e. in SGlobalMethods)

moved, also, the PR is finalized now, please review. If you think anything is not tested, please specify what exactly, and so I will be able to add tests.

Copy link
Member

@aslesarenko aslesarenko left a comment

Choose a reason for hiding this comment

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

IMO, the most important tests are those in LanguageSpecification, more specifically using verifyCases method.
When they are done, not only full coverage is ensured, but also cost profiling can be done.
I suggest to test these new operations as two separate property()

@@ -695,6 +695,20 @@ trait SigmaDslBuilder {
*/
def groupGenerator: GroupElement

/**
* @return big integer provided as input approximately encoded using NBits,
Copy link
Member

Choose a reason for hiding this comment

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

A bit confusing comment, says "@return big integer" while the actual return type is Long.

groupGeneratorMethod,
xorMethod
)
private lazy val EnDecodeNBitsCost = FixedCost(JitCost(5)) // the same cost for nbits encoding and decoding
Copy link
Member

Choose a reason for hiding this comment

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

The cost is too low. And encoding is cheaper than decoding (because of array creation).
There is a profiler in LSV5 (see the comments and afterAll method).
I suggest at least 2 for encoding and 40 for decoding.

@@ -175,6 +176,14 @@ class CSigmaDslBuilder extends SigmaDslBuilder { dsl =>

override def groupGenerator: GroupElement = _generatorElement

def encodeNbits(bi: BigInt): Long = {
Copy link
Member

Choose a reason for hiding this comment

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

override missing

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