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

feat: native bigint using math/big.Int #764

Closed
wants to merge 1 commit into from

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Apr 20, 2023

Description

This is separated PR which includes only bigint from #676

Types that can be converted to bigint

  • int
  • int32
  • int64
  • uint
  • uint32
  • uint64

How has this been tested?

bigint_test.gno

Benchmark & Performance

goos: darwin
goarch: arm64
Benchmark_Int64Add-10     	1000000000	         0.0004173 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Add-10     	1000000000	         0.0003738 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Add-10     	1000000000	         0.0003746 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Add-10    	1000000000	         0.0003738 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Add-10    	1000000000	         0.0003744 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Add-10    	1000000000	         0.0003477 ns/op	       0 B/op	       0 allocs/op
Benchmark_BigintAdd-10    	1000000000	         0.007314 ns/op	       	       0 B/op	       0 allocs/op
Benchmark_BigintAdd-10    	1000000000	         0.007358 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintAdd-10    	1000000000	         0.007335 ns/op	               0 B/op	       0 allocs/op
Benchmark_Int64Sub-10     	1000000000	         0.0003242 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Sub-10     	1000000000	         0.0003167 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Sub-10     	1000000000	         0.0003189 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Sub-10    	1000000000	         0.0003235 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Sub-10    	1000000000	         0.0003245 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Sub-10    	1000000000	         0.0003221 ns/op	       0 B/op	       0 allocs/op
Benchmark_BigintSub-10    	1000000000	         0.008838 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintSub-10    	1000000000	         0.008850 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintSub-10    	1000000000	         0.008864 ns/op	               0 B/op	       0 allocs/op
Benchmark_Int64Mul-10     	1000000000	         0.0003225 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Mul-10     	1000000000	         0.0003163 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Mul-10     	1000000000	         0.0003220 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Mul-10    	1000000000	         0.0003220 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Mul-10    	1000000000	         0.0003221 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Mul-10    	1000000000	         0.0003216 ns/op	       0 B/op	       0 allocs/op
Benchmark_BigintMul-10    	1000000000	         0.008035 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintMul-10    	1000000000	         0.008328 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintMul-10    	1000000000	         0.008106 ns/op	               0 B/op	       0 allocs/op
Benchmark_Int64Div-10     	1000000000	         0.0006240 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Div-10     	1000000000	         0.0006945 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Div-10     	1000000000	         0.0006245 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Div-10    	1000000000	         0.0006283 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Div-10    	1000000000	         0.0006655 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Div-10    	1000000000	         0.0006413 ns/op	       0 B/op	       0 allocs/op
Benchmark_BigintDiv-10    	1000000000	         0.01054 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintDiv-10    	1000000000	         0.01056 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintDiv-10    	1000000000	         0.01055 ns/op	               0 B/op	       0 allocs/op
Benchmark_Int64Mod-10     	1000000000	         0.0006245 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Mod-10     	1000000000	         0.0006695 ns/op	       0 B/op	       0 allocs/op
Benchmark_Int64Mod-10     	1000000000	         0.0006420 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Mod-10    	1000000000	         0.0006414 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Mod-10    	1000000000	         0.0006415 ns/op	       0 B/op	       0 allocs/op
Benchmark_Uint64Mod-10    	1000000000	         0.0006413 ns/op	       0 B/op	       0 allocs/op
Benchmark_BigintMod-10    	1000000000	         0.009151 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintMod-10    	1000000000	         0.009131 ns/op	               0 B/op	       0 allocs/op
Benchmark_BigintMod-10    	1000000000	         0.009112 ns/op	               0 B/op	       0 allocs/op
PASS

Additional

there is #761 issue about memory allocations

@r3v4s
Copy link
Contributor Author

r3v4s commented Apr 20, 2023

Without touching go spec to support <<, >> for native bigint, it requires right operand to be uint type.

PR will panic if bigint -> uint converting happens no matter whether bigint can be fit into uint or not.

IMHO, converting can be conditionally allowed when bigint does fit in to target type.

@piux2 piux2 requested a review from jaekwon April 21, 2023 00:11
@dongwon8247
Copy link
Member

@jaekwon @moul @thehowl Would love to have your feedback on this PR :)

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Implementation looks good (need advanced review), but let's discuss #676 before adding a new keyword.

Edit: #676 was a closed PR. Since it has the most history, we can continue on that or open a new issue. It's up to you.

@ajnavarro
Copy link
Contributor

Sorry for my ignorance, but why don't just stick with the standard way to handle bigints in Go, using "math/big"?

@moul
Copy link
Member

moul commented May 3, 2023

Sorry for my ignorance, but why don't just stick with the standard way to handle bigints in Go, using "math/big"?

Thanks for your question.

The only required component is the implementation and availability of math/big.

The proposed extension with bigint keyword aims to better support big integer usage in DeFi contracts, potentially simplifying code and ensuring compatibility with existing Go code via gno precompile.
Although optional, the feature is straightforward and beneficial enough to justify a consideration of its advantages and drawbacks.

@moul moul marked this pull request as draft May 6, 2023 16:49
@thehowl
Copy link
Member

thehowl commented May 12, 2023

@r3v4s Could you share how you're doing benchmarks? Unless you have a supercomputer, 0.009112 ns/op is veeeery unlikely (are you just calling the function in the benchmark or are you iterating with for i := 0; i < b.N; i++?)

I still think that the best course of action for Gno would be to have uint256/128 instead of bigint, at least initially, as I think they have fewer memory usage implications than bigint.

Do you have examples of applications where you think bigints make sense as compared to a uint256?

@r3v4s
Copy link
Contributor Author

r3v4s commented May 12, 2023

@r3v4s Could you share how you're doing benchmarks? Unless you have a supercomputer, 0.009112 ns/op is veeeery unlikely (are you just calling the function in the benchmark or are you iterating with for i := 0; i < b.N; i++?)

  • I was iteration with for i := 0; i < b.N; i++

I still think that the best course of action for Gno would be to have uint256/128 instead of bigint, at least initially, as I think they have fewer memory usage implications than bigint.

  • I do agree with you that uint128/256 will have fewer memory usage

Do you have examples of applications where you think bigints make sense as compared to a uint256?

  • dex maybe? operation like `uint256 x uint256' could happen, and most likely it will panic

@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Sep 12, 2023
@r3v4s r3v4s force-pushed the feat/native-bigint branch 2 times, most recently from 72ea0a7 to 1b2c4d8 Compare September 12, 2023 08:17
@moul
Copy link
Member

moul commented Dec 6, 2023

@r3v4s, please sync the master branch and mix conflicts on your PR.

Also, could you consider using math/big as a temporary solution, allowing us to bypass this PR for now? What would be the impact of this? Ideally, we'd like to delay this update to handle more scenarios later.

@mvertes @thehowl @piux2, I'd appreciate your insights regarding the VM change.

For everyone's reference, incorporating new keywords is on our roadmap. Adding new types is a logical step, and we can likely ensure compatibility with Go through transpilation scripts.

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (a4c7b86) 56.31% compared to head (7d66aea) 47.20%.

Files Patch % Lines
gno.land/pkg/sdk/vm/convert.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #764      +/-   ##
==========================================
- Coverage   56.31%   47.20%   -9.11%     
==========================================
  Files         422      371      -51     
  Lines       65699    61706    -3993     
==========================================
- Hits        36999    29129    -7870     
- Misses      25818    30190    +4372     
+ Partials     2882     2387     -495     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@r3v4s
Copy link
Contributor Author

r3v4s commented Dec 7, 2023

@moul synced pr branch with latest master, there will be no more conflicts for now. However CI is failing because it precompiles .gno to gen.go and unfortunately go doesn't support bigint keyword.

OTH, about using math/big as a temporary solution it might be possible when gno has full(or at least partial) compatibility about math/big with go

| math/big | `tbd` |

@moul
Copy link
Member

moul commented Dec 7, 2023

math/big may cause issues with gnokey maketx call if it doesn't recognize this type.

Consider using uint64 as a more compatible alternative for now.

While I support integrating larger integer support, launching without it is an option. Please evaluate the impact of a 64-bit limit. Note that without robust bigint support, running IBC might be more challenging, so I guess the deadline for this feature would probably be IBC. This could open up the possibility for a Gnoswap V2 with IBC, while V1 could handle smaller transactions, perhaps utilizing prefixes like “ugnot” and “gnot.” These could eventually be processed automatically by the chain or you contract when such token paris are identified, offering flexibility.

While Onbloc considers my previous suggestion, the core team will review your PR with the aim of either merging it or implementing a comparable solution.

@r3v4s
Copy link
Contributor Author

r3v4s commented Dec 7, 2023

@moul
currently, gnoswap uses bigint to avoid use of floating type or value.

it's similar to uniswap because floating in EVM also has a determinism problem.

Let's say gnoswap v2(or later) will support bigint while v1 only uses primitive type. Because of token pair's exchange ratio in certain pool can be very narrow, we have no choice but to use uint64, float64 type (with all kind of math package's function such as math.Log10).

@moul
Copy link
Member

moul commented Dec 7, 2023

@r3v4s: there will be no more conflicts for now. However CI is failing because it precompiles .gno to gen.go and unfortunately go doesn't support bigint keyword.

To achieve this, we need to modify the gno precompile to generate a distinct yet valid Go code for this scenario.

@moul
Copy link
Member

moul commented Dec 7, 2023

@moul currently, gnoswap uses bigint to avoid use of floating type or value.

it's similar to uniswap because floating in EVM also has a determinism problem.

Let's say gnoswap v2(or later) will support bigint while v1 only uses primitive type. Because of token pair's exchange ratio in certain pool can be very narrow, we have no choice but to use uint64, float64 type (with all kind of math package's function such as math.Log10).

What if you use uint64 for the public functions in v1 and to use math/big or any other library to support internal operations such as divisions and floating point calculations.

Once native support compatible with maketx is available, you can transition to v2 and simplify the implementation.

This doesn't imply that this PR is bad or anything; it's just about making this point not a launch constraint for you but a v2/milestone constraint.

@r3v4s
Copy link
Contributor Author

r3v4s commented Dec 7, 2023

What if you use uint64 for the public functions in v1 and to use math/big or any other library to support internal operations such as divisions and floating point calculations.

Once native support compatible with maketx is available, you can transition to v2 and simplify the implementation.

This doesn't imply that this PR is bad or anything; it's just about making this point not a launch constraint for you but a v2/milestone constraint.

I think it's good point to start (also it might reduce complex for go2gno converting function )

@thehowl
Copy link
Member

thehowl commented May 15, 2024

Closing for now as we are trialling the uint256 package instead.

@thehowl thehowl closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: 🌟 Wanted for Launch
Status: Done
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants