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

Enhancement: Re-enable fillBytes method in ABI and eval.go implementation #3856

Merged
merged 4 commits into from
Apr 2, 2022

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Apr 1, 2022

Description

The current go-algorand build is over golang 1.16, which means we can revert #3498.

#3498 is introduced for the sake of downgrading golang for perf issues, now we should lift back to earlier implementation.

We also re-enable FillBytes in leadingZero implementation, and update elliptic.(un)marshalCompressed for #2852 .

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2022

Codecov Report

Merging #3856 (83d3619) into master (9d9d759) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3856      +/-   ##
==========================================
- Coverage   49.97%   49.97%   -0.01%     
==========================================
  Files         393      393              
  Lines       68390    68353      -37     
==========================================
- Hits        34181    34160      -21     
+ Misses      30473    30464       -9     
+ Partials     3736     3729       -7     
Impacted Files Coverage Δ
data/abi/abi_encode.go 63.24% <100.00%> (+0.74%) ⬆️
data/transactions/logic/eval.go 89.72% <100.00%> (+0.12%) ⬆️
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
catchup/service.go 68.14% <0.00%> (-1.24%) ⬇️
cmd/tealdbg/debugger.go 71.42% <0.00%> (-0.99%) ⬇️
network/wsNetwork.go 62.99% <0.00%> (+0.19%) ⬆️
network/wsPeer.go 68.88% <0.00%> (+3.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d9d759...83d3619. Read the comment docs.

@ahangsu ahangsu changed the title Enhancement: Re-enable fillBytes method in ABI implementation Enhancement: Re-enable fillBytes method in ABI and eval.go implementation Apr 1, 2022
@ahangsu ahangsu requested a review from algorandskiy April 1, 2022 18:54
Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@ahangsu Thanks for recalling that simplifications exist and for bringing the changes in. ☕

Aside - The PR relates to #3570.

@@ -3070,7 +3014,7 @@ func opEcdsaPkDecompress(cx *EvalContext) error {
return fmt.Errorf("invalid pubkey")
}
} else if fs.field == Secp256r1 {
x, y = unmarshalCompressed(elliptic.P256(), pubkey)
x, y = elliptic.UnmarshalCompressed(elliptic.P256(), pubkey)
Copy link
Contributor

@jannotti jannotti Apr 2, 2022

Choose a reason for hiding this comment

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

Should we retest the speed of decompress to reconsider opcode cost? Or was our implementation the same as this library function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous (un)marshalCompressed was copying the library implementation, so I felt like this should not change the speed.

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

Successfully merging this pull request may close these issues.

4 participants