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

replace the KMD usage in pingpong with direct signing #2653

Merged
merged 8 commits into from
Jul 31, 2021
Merged

replace the KMD usage in pingpong with direct signing #2653

merged 8 commits into from
Jul 31, 2021

Conversation

AlgoStephenAkiki
Copy link
Contributor

@AlgoStephenAkiki AlgoStephenAkiki commented Jul 28, 2021

Summary

Removes signing calls to KMD by creating internal hashmap and signing transactions locally.

Resolves #2644

Test Plan

Tested using performance system.

@brianolson
Copy link
Contributor

Still should have local storage for generated accounts

@@ -208,12 +220,12 @@ func fundAccounts(accounts map[string]uint64, client libgoal.Client, cfg PpConfi
}

fmt.Printf("adjusting account balance to %d\n", minFund)
for addr, balance := range accounts {
for addr, val := range accounts {
Copy link
Contributor

Choose a reason for hiding this comment

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

val -> acct

cfg PpConfig
accounts map[string]uint64
cfg PpConfig
//accounts map[string]uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

drop commented out lines.

return
}

psig = accounts[signer].sk.Sign(logic.Program(cfg.Program))
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong, but I think that you might want to do

progb := logic.Program(cfg.Program)
psig = accounts[signer].sk.Sign(&progb)

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

This PR seems to fail for the following test cases:
asset_transfer_small, asset_transfer_large, stateful_teal_small, stateful_teal_medium, stateful_teal_large.
Could you manually test that locally and make sure it generates valid transactions ?

@codecov-commenter
Copy link

Codecov Report

Merging #2653 (fa75e5f) into master (434b98f) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2653      +/-   ##
==========================================
+ Coverage   47.04%   47.05%   +0.01%     
==========================================
  Files         349      349              
  Lines       55814    55814              
==========================================
+ Hits        26256    26262       +6     
+ Misses      26608    26605       -3     
+ Partials     2950     2947       -3     
Impacted Files Coverage Δ
network/requestTracker.go 70.25% <0.00%> (-0.87%) ⬇️
network/wsPeer.go 74.37% <0.00%> (-0.28%) ⬇️
network/wsNetwork.go 60.73% <0.00%> (-0.19%) ⬇️
ledger/acctupdates.go 62.21% <0.00%> (-0.09%) ⬇️
catchup/service.go 70.12% <0.00%> (+0.77%) ⬆️
cmd/tealdbg/debugger.go 73.86% <0.00%> (+1.00%) ⬆️
ledger/blockqueue.go 85.05% <0.00%> (+4.02%) ⬆️

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 434b98f...fa75e5f. Read the comment docs.

@tsachiherman tsachiherman changed the title Feature/2644 faster pingpong replace the KMD usage in pingpong with direct signing Jul 31, 2021
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks good.

@tsachiherman tsachiherman merged commit 8835bfd into algorand:master Jul 31, 2021
tsachiherman added a commit that referenced this pull request Aug 4, 2021
tsachiherman added a commit that referenced this pull request Aug 4, 2021
#2689)

This reverts commit 8835bfd.
Revert "replace the KMD usage in pingpong with direct signing"
#2653 created some undesired performance regressions reported on the perf monitoring system. I'm rolling this change back so we can take it offline.
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.

faster pingpong
4 participants