all: replace Div/Mul with Rsh/Lsh if possible #29911#1966
all: replace Div/Mul with Rsh/Lsh if possible #29911#1966AnilChinchawale merged 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8bbc928 to
54bc1ce
Compare
|
@gzliudan conflict |
54bc1ce to
7526560
Compare
done |
There was a problem hiding this comment.
Pull request overview
This PR optimizes big integer arithmetic by replacing division and multiplication operations with bit shift operations where the divisor/multiplier is a power of 2. The changes replace Div(x, 2^n) with Rsh(x, n) and Mul(x, 2^n) with Lsh(x, n), which are mathematically equivalent and more efficient operations.
Changes:
- Replaced multiplication by powers of 2 (2, 4, 8, 16, 32) with left shift operations
- Replaced division by powers of 2 (2, 4, 8, 16, 32) with right shift operations
- Removed unused constants
big4,big8, andbig16fromcore/vm/contracts.go
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fuzzers/bn256/bn256_fuzz.go | Optimized scalar multiplication by 2 in BN256 fuzzer |
| internal/ethapi/transaction_args_test.go | Optimized max fee calculation in test |
| internal/ethapi/transaction_args.go | Optimized London fee defaults calculation |
| internal/ethapi/api.go | Optimized staker ROI calculations using right shift |
| eth/util/util.go | Optimized reward inflation halving calculations |
| eth/gasprice/gasprice.go | Optimized gas price suggestion calculations |
| crypto/secp256k1/curve.go | Optimized Jacobian doubling arithmetic |
| crypto/crypto.go | Optimized secp256k1 half N calculation |
| core/vm/privacy/ringct.go | Optimized compressed key deserialization |
| core/vm/contracts.go | Optimized modexp complexity and gas calculations, removed unused constants |
| core/types/transaction_signing.go | Critical bug: Changed deriveChainId to mutate input parameter |
| core/txpool/legacypool/legacypool_test.go | Optimized gas fee calculations in tests |
| contracts/trc21issuer/simulation/test/main.go | Optimized TRC21 gas price calculations |
| consensus/ethash/consensus.go | Optimized uncle reward calculations |
| XDCxlending/order_processor_test.go | Optimized test quantity calculations |
| XDCxlending/order_processor.go | Optimized relayer fee checks |
| XDCxlending/lendingstate/settle_balance.go | Optimized interest rate calculations |
| XDCx/tradingstate/settle_balance_test.go | Optimized test trade quantity setup |
| XDCx/order_processor.go | Optimized relayer fee checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v.Sub(v, big.NewInt(35)) | ||
| return v.Rsh(v, 1) |
There was a problem hiding this comment.
The variable name is inconsistent with the function signature. At line 592, v is being modified (using Sub which modifies the receiver), but this shadows the parameter v from line 584. The original code created a new big.Int with v = new(big.Int).Sub(v, big.NewInt(35)), but the updated code modifies the parameter directly with v.Sub(v, big.NewInt(35)). This changes the behavior by mutating the input parameter, which could cause unexpected side effects if the caller retains a reference to the parameter.
| v.Sub(v, big.NewInt(35)) | |
| return v.Rsh(v, 1) | |
| vCopy := new(big.Int).Sub(v, big.NewInt(35)) | |
| return vCopy.Rsh(vCopy, 1) |
|
|
||
| z3 := new(big.Int).Mul(y, z) //Y1*Z1 | ||
| z3.Mul(big.NewInt(2), z3) //3*Y1*Z1 | ||
| z3.Lsh(z3, 1) //3*Y1*Z1 |
There was a problem hiding this comment.
The comment is incorrect. The code performs a left shift by 1, which multiplies by 2, not 3. The comment should say "2Y1Z1" instead of "3Y1Z1". Note that this appears to be a pre-existing issue (the original code also multiplied by 2), so it's not introduced by this PR, but it would be good to fix it.
| z3.Lsh(z3, 1) //3*Y1*Z1 | |
| z3.Lsh(z3, 1) //2*Y1*Z1 |
Proposed changes
Ref: ethereum#29911
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that