Removing C/crypto dependencies from `data/abi` package #3375

ahangsu · 2022-01-05T22:31:46Z

Summary

We want to remove C/crypto dependencies from data/abi package, because the previous commit d3bbe62 introduced dependencies to crypto package with C dependencies (SHA512/256 needed). This led to go-algorand-sdk cannot easily update go-algorand past that commit.

The current solution is: "rewrite" the checksum function for address in data/abi package, which removes dependencies to data/basics and algorand/crypto, and sets dependency to crypto (golang).

Refactor the data/abi package to avoid referencing data/basics, closes #3355.

…s...

cce · 2022-01-06T01:43:37Z

Theoretically, it would be possible to move crypto.Digest, crypto.Hash, and basics.Address into packages that didn't import libsodium crypto...

codecov-commenter · 2022-01-06T16:34:36Z

Codecov Report

Merging #3375 (30d3db4) into master (7248b95) will increase coverage by 0.02%.
The diff coverage is 34.21%.

@@            Coverage Diff             @@
##           master    #3375      +/-   ##
==========================================
+ Coverage   47.65%   47.68%   +0.02%     
==========================================
  Files         369      369              
  Lines       59660    59681      +21     
==========================================
+ Hits        28433    28460      +27     
+ Misses      27914    27909       -5     
+ Partials     3313     3312       -1

Impacted Files	Coverage Δ
data/abi/abi_type.go	`87.67% <ø> (-0.95%)`	⬇️
data/basics/address.go	`90.32% <ø> (ø)`
data/abi/abi_json.go	`52.63% <32.25%> (-3.92%)`	⬇️
data/abi/abi_encode.go	`64.89% <42.85%> (-0.11%)`	⬇️
cmd/algoh/blockWatcher.go	`77.77% <0.00%> (-3.18%)`	⬇️
network/wsNetwork.go	`62.84% <0.00%> (-0.20%)`	⬇️
ledger/acctupdates.go	`65.77% <0.00%> (ø)`
data/transactions/verify/txn.go	`44.29% <0.00%> (ø)`
catchup/service.go	`69.40% <0.00%> (+0.74%)`	⬆️
... and 5 more

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 7248b95...30d3db4. Read the comment docs.

jasonpaulos · 2022-01-06T22:46:01Z

This looks correct, but I don't love that it reimplements address encoding & decoding.

Refactoring crypto.Digest, crypto.Hash, and basics.Address into new packages that don't have C deps like @cce suggested seems like the best long term solution, but in the spirit of fixing our Go SDK import problem quickly, I think I'm ok with this.

Though I do have two recommendations:

Add a comment somewhere in data/basics/address.go that mentions there's another implementation of this in the abi package due to dependency issues (probably link to this PR too).
Add a unit test for addressCheckSum that ensures it produces the same output as basics.Address. Maybe generate a large number of random addresses and compare the output of both functions.

cce · 2022-01-07T16:16:01Z

Another idea, from @winder, is to wrap libsodium in a driver interface to make the existing crypto package importable even if libsodium doesn't exist. So calls to methods that use libsodium would return "Not implemented" errors, panic, or use an alternate Go implementation, if go-algorand was imported without it.

That way you wouldn't have to have a huge refactor everywhere we use crypto.Hash, crypto.Digest, etc, which don't rely on libsodium anyway.

data/abi package only requires Go code (I believe the digest, address, hashing code used by basics.Address, etc are implemented in pure Go) so this would make it easier to import for go-algorand-sdk and other projects.

jannotti

I think I'm ok with this too. We reimplement these things in every SDK. It seems extra weird to do it in Go, since there is already one implementation in Go. But I guess I don't really mind very much.

jasonpaulos

Looks good

* testing: Fix unit test TestAsyncTelemetryHook_QueueDepth (#2685) Fix the unit test TestAsyncTelemetryHook_QueueDepth * Deprecate `FastPartitionRecovery` from `ConsensusParams` (#3386) ## Summary This PR removes `FastPartitionRecovery` option from consensus parameters. The code now acts as if this value is set to true. Closes https://github.com/algorand/go-algorand-internal/issues/1830. ## Test Plan None. * Remaking a PR for CI (#3398) * Allow setting manager, reserve, freeze, and clawback at goal asset create * Add e2e tests * Add more tests for goal asset create flags Co-authored-by: Fionna <[email protected]> * [Other] CircleCI pipeline change for binary uploads (#3381) For nightly builds ("rel/nightly"), we want to have deadlock enabled. For rel/beta and rel/stable, we want to make sure we can build and upload a binary with deadlock disabled so that it can be used for release testing and validation purposes. * signer.KeyDilution need not depend on config package (#3265) crypto package need not depend on config. There is an unnecessary dependency on config. signer.KeyDilution takes the `config.ConsensusParams` as argument to pick the DefaultKeyDilution from it. This introduces dependency from the crypto package to config package. Instead, only the DefaultKeyDilution value can be passed to signer.KeyDilution. * algodump is a tcpdump-like tool for algod's network protocol (#3166) This PR introduces algodump, a tcpdump-like tool for monitoring algod network messages. * Removing C/crypto dependencies from `data/abi` package (#3375) * Feature Networks pipeline related changes (#3393) Added support for not having certain files in signing script Co-authored-by: Tsachi Herman <[email protected]> Co-authored-by: Tolik Zinovyev <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: Fionna <[email protected]> Co-authored-by: algobarb <[email protected]> Co-authored-by: Shant Karakashian <[email protected]> Co-authored-by: Nickolai Zeldovich <[email protected]> Co-authored-by: Hang Su <[email protected]>

* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments * Impose limits on the entire "tree" of inner calls. This also increases the realism of testing of multiple app calls in a group by creating the EvalParams with the real constructor, thus getting the pooling stuff tested here without playing games manipulating the ep after construction. * Move appID tracking into EvalContext, out of LedgerForLogic This change increases the seperation between AVM execution and the ledger being used to lookup resources. Previously, the ledger kept track of the appID being executed, to offer a narrower interface to those resources. But now, with app-to-app calls, the appID being executed must change, and the AVM needs to maintain the current appID. * Stupid linter * Fix unit tests error messages * Allow access to resources created in the same transaction group The method will be reworked, but the tests are correct and want to get them visible to team. * Access to apps created in group Also adds some tests that are currently skipped for testing - access to addresses of newly created apps - use of gaid in inner transactions Both require some work to implement the thing being tested. * Remove tracked created mechanism in favor of examining applydata. * Allow v6 AVM code to use in-group created asas, apps (& their accts) One exception - apps can not mutate (put or del) keys from the app accounts, because EvalDelta cannot encode such changes. * lint docs * typo * The review dog needs obedience training. * Use one EvalParams for logic evals, another for apps in dry run We used to use one ep per transaction, shared between sig and and app. But the new model of ep usage is to keep using one while evaluating an entire group. The app ep is now built logic.NewAppEvalParams which, hopefully, will prevent some bugs when we change something in the EvalParams and don't reflect it in what was a "raw" EvalParams construction in debugger and dry run. * Use logic.NewAppEvalParams to decrease copying and bugs in debugger * Simplify use of NewEvalParams. No more nil return when no apps. This way, NewEvalParams can be used for all creations of EvalParams, whether they are intended for logicsig or app use, greatly simplifying the way we make them for use by dry run or debugger (where they serve double duty). * Remove explicit PastSideEffects handling in tealdbg * Always create EvalParams to evaluate a transaction group. We used to have an optimization to avoid creating EvalParams unless there was an app call in the transaction group. But the interface to allow transaction processing to communicate changes into the EvalParams is complicated by that (we must only do it if there is one!) This also allows us to use the same construction function for eps created for app and logic evaluation, simplifying dry-run and debugger. The optimization is less needed now anyway: 1) The ep is now shared for the whole group, so it's only one. 2) The ep is smaller now, as we only store nil pointers instead of larger scratch space objects for non-app calls. * Correct mistaken commit * Spec improvments * More spec improvments, including resource "availability" * Recursively return inner transaction tree * Lint * No need for ConfirmedRound, so don't deref a nil pointer! * license check * Shut up, dawg. * testing: Fix unit test TestAsyncTelemetryHook_QueueDepth (#2685) Fix the unit test TestAsyncTelemetryHook_QueueDepth * Deprecate `FastPartitionRecovery` from `ConsensusParams` (#3386) ## Summary This PR removes `FastPartitionRecovery` option from consensus parameters. The code now acts as if this value is set to true. Closes algorand/go-algorand-internal#1830. ## Test Plan None. * base64 merge cleanup * Remaking a PR for CI (#3398) * Allow setting manager, reserve, freeze, and clawback at goal asset create * Add e2e tests * Add more tests for goal asset create flags Co-authored-by: Fionna <[email protected]> * Remove the extraneous field type arrays. * bsqrt * acct_holding_get, a unified opcode for account field access * Thanks, dawg * [Other] CircleCI pipeline change for binary uploads (#3381) For nightly builds ("rel/nightly"), we want to have deadlock enabled. For rel/beta and rel/stable, we want to make sure we can build and upload a binary with deadlock disabled so that it can be used for release testing and validation purposes. * signer.KeyDilution need not depend on config package (#3265) crypto package need not depend on config. There is an unnecessary dependency on config. signer.KeyDilution takes the `config.ConsensusParams` as argument to pick the DefaultKeyDilution from it. This introduces dependency from the crypto package to config package. Instead, only the DefaultKeyDilution value can be passed to signer.KeyDilution. * CR and more spec simplification * algodump is a tcpdump-like tool for algod's network protocol (#3166) This PR introduces algodump, a tcpdump-like tool for monitoring algod network messages. * Removing C/crypto dependencies from `data/abi` package (#3375) * Feature Networks pipeline related changes (#3393) Added support for not having certain files in signing script * e2e test for inner transaction appls * testing: Add slightly more coverage to TestAcctUpdatesLookupRetry (#3384) Add slightly more coverage to TestAcctUpdatesLookupRetry * add context to (most) agreement logged writes (#3411) Current agreement code only writes a `context : agreement` to a subset of the logged messages. This change extends the said entry, which would make it easier to pre-process logs entries by their corresponding component. The change in this PR is focused on: 1. make sure that the "root" agreement logger always injects the `context : agreement` argument. 2. change the various locations in the agreement code to use the root agreement logger instead of referring to the application-global instance (`logging.Base()`). * network: faster node shutdown (#3416) During the node shutdown, all the current outgoing connections are being disconnected. Since these connections are web sockets, they require a close connection message to be sent. However, sending this message can take a while, and in situations where the other party has already shut down, we might never get a response. That, in turn, would lead the node waiting until the deadline is reached. The current deadline was 5 seconds. This PR changes the deadline during shutdown to be 50ms. * Give max group size * 16 inner txns, regardless of apps present * Adjust test for allowing 256 inners Co-authored-by: Tsachi Herman <[email protected]> Co-authored-by: Tolik Zinovyev <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: Fionna <[email protected]> Co-authored-by: algobarb <[email protected]> Co-authored-by: Shant Karakashian <[email protected]> Co-authored-by: Nickolai Zeldovich <[email protected]> Co-authored-by: Hang Su <[email protected]> Co-authored-by: chris erway <[email protected]>

minor update to circumvent data/basic, but we are using crypto anyway…

0a73d91

…s...

ahangsu added the Team Scytale label Jan 6, 2022

update abi json file, use crypto sha512 not algorand crypto

62857f1

ahangsu marked this pull request as ready for review January 6, 2022 15:58

ahangsu requested review from jannotti and jasonpaulos January 6, 2022 15:58

minor optimization on parameters in test and implementation

4b3bf22

ahangsu added 3 commits January 10, 2022 14:51

use golang 1.16 and fillbytes, add notes of discussion about crypto

e17cc7a

update abi address test, update note on basics.address

1788979

update

30d3db4

jannotti approved these changes Jan 10, 2022

View reviewed changes

jasonpaulos approved these changes Jan 11, 2022

View reviewed changes

jannotti merged commit 326d04a into algorand:master Jan 13, 2022

ahangsu deleted the refactoring-abi branch January 13, 2022 19:02

algobarb mentioned this pull request Jan 18, 2022

go-algorand 3.3.0-beta #3423

Merged

Algo-devops-service mentioned this pull request Jan 20, 2022

Update reference to arm32 build for 3.3.0 release #3461

Merged

onetechnical mentioned this pull request Mar 1, 2022

go-algorand 3.4.2-stable #3700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removing C/crypto dependencies from `data/abi` package #3375

Removing C/crypto dependencies from `data/abi` package #3375

Uh oh!

ahangsu commented Jan 5, 2022 •

edited

Loading

Uh oh!

cce commented Jan 6, 2022

Uh oh!

codecov-commenter commented Jan 6, 2022 •

edited

Loading

Uh oh!

jasonpaulos commented Jan 6, 2022

Uh oh!

cce commented Jan 7, 2022 •

edited

Loading

Uh oh!

jannotti left a comment

Uh oh!

jasonpaulos left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Removing C/crypto dependencies from data/abi package #3375

Removing C/crypto dependencies from data/abi package #3375

Uh oh!

Conversation

ahangsu commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

cce commented Jan 6, 2022

Uh oh!

codecov-commenter commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jasonpaulos commented Jan 6, 2022

Uh oh!

cce commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

jasonpaulos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Removing C/crypto dependencies from `data/abi` package #3375

Removing C/crypto dependencies from `data/abi` package #3375

ahangsu commented Jan 5, 2022 •

edited

Loading

codecov-commenter commented Jan 6, 2022 •

edited

Loading

cce commented Jan 7, 2022 •

edited

Loading