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

Add get_signed_transaction_signers or similar command to CLI #1210

Closed
abitmore opened this issue Jul 30, 2018 · 16 comments

Comments

Projects
4 participants
@abitmore
Copy link
Member

commented Jul 30, 2018

User Story
As a user I want to know a signed transaction is signed by whom, specifically, the corresponding public keys of the private keys used to sign the transaction. It's able to get the public keys from signatures.

Additional Context (optional)
After got the keys, can use get_key_references API to search for related accounts, but it's not always accurate because get_key_references only queries current state. This is out of scope of this issue.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
    • Add priority label
    • Estimate: pending
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
    • Assigned: @OpenLedgerDev
    • Estimate:
      • Developer: 30 hours
      • Project Manager: 5 hours
  • Perform QA/Testing
  • Update Documentation

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jul 30, 2018

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

@abitmore May I request you help me refine the user story? If I understand correctly, the return value you desire from this new feature is the set of account names that signed the given transaction. It is possible to change the active authority set over time, so currently the get_key_references query may be unable to determine which account provided the signature in the past. Have I got that right?

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

the return value you desire from this new feature is the set of account names

No, public keys.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Example: on testnet the xeroc account has several accounts/keys in active authority, all with weight 1, and the threshold is 1, so every key/account is able to sign a transaction on behalf of xeroc. With the proposed command, we can know the transaction is signed by which public key in case when there is misbehavior, then can remove or swap it accordingly.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

Alright, I understand the requirement now. Historically BitShares has hidden the keys, representing them by the account name, so my assumption was wrong. I would find value in knowing the set of signers providing their signatures (by name), their weight and the associated key used. The active weight and key may change over time, so the return values must be at the time of the transaction validation (not when queried).

Should this feature be expanded to include userID, signingWeight and pubKey?

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

It's just hard to expand with node API only, so this issue is only for public keys (lack of data is better than wrong data). Perhaps can do something with ES when get the keys, but out of this issue's scope.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

@bitshares/core-dev Please review this Issue and update:

  • Add priority label
  • Add estimated development hours into description

@OpenLedgerDev intends to claim this Issue and begin working on it.

@ryanRfox ryanRfox removed this from New -Awaiting Core Team Evaluation in Project Backlog Sep 5, 2018

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Added Estimates from @OpenLedgerDev:

  • Estimate:
    • Developer: 30 hours
    • Project Manager: 5 hours

Still awaiting comment from @bitshares/core-dev

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

My estimation on this issue is 2-5 hours.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

Here's our detailed estimation.

This includes expanded version, not only public key.
In OpenLedger we also have a test framework that we use for all our BitShares development.
So our estimation also includes writing tests, running testnet and running tests to make sure everything is fine.

  • Find out how to extract public key from transaction: 4 hours

  • add get_transaction_signers to cli_wallet - get public keys only: 2 hours

  • unit tests: 2 hours
    get transaction signers from correctly signed transaction (positive)
    get transaction signers from signed and modified transaction (negative)

  • add get_key_references to cli_wallet: 2 hours

  • enumerate accounts authorities (recursive) and get keys weights and thresholds: 4 hours

  • unit tests- generate complicated multilevel multisign account, sign transaction, get transaction signers, check results: 8 hours

  • add smoke tests (python)- create multisign account, partially sign transaction, get signers, check result: 4 hours (QA)

  • run testnet, smoke tests, check new cli_wallet functionality manually: 4 hours

  • project coordination: 5 hours

Also, 4 hours to add automated smoke tests is actually done by our QA, not developer, so this work was incorrectly set as a developer, I am sorry.

Having only public keys instead of more expanded would take about 2 times fewer hours.
please let me know if we misunderstood and only the public key functionality is needed.

We would be happy to see @abitmore detailed estimation so that we can compare to ours.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

To implement the feature described in this issue, we need to add a cli command/API, which calls signed_transaction::get_signature_keys(chain_id) and return the result. How a transaction would be signed or verified is out of scope. Test cases can be: constructing a transaction, sign with a few private keys and/or don't sign, then call the API, check whether the result is expected.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@ryanRfox I think our developers estimated the extended way of this change request as I mentioned before. So here is our estimation for impelementing public keys only:

  • Find out how to extract public key from transaction: 4 hours
  • add get_transaction_signers to cli_wallet - get public keys only: 2 hours
  • unit tests: 2 hours
    get transaction signers from correctly signed transaction (positive)
    get transaction signers from signed and modified transaction (negative)
  • add get_key_references to cli_wallet: 2 hours
  • run testnet, smoke tests, check new cli_wallet functionality manually: 4 hours
  • project coordination: 2 hours
    So it's all together 14 hours of developer's time + 2 hours of project coordination

Although our developers are quite familiar with BitShares, they are not intimately familiar with 100% of all the code. Neither is anyone in this world, because it's quite a complicated system that has been in the development by many people over the last 3 years.
So that's why we took 4 hours of making sure we know what we're fixing and know all the specifics of the particular part of the code.

Moreover, as I already mentioned in our development process it's crucial that we run tests after every change that we make. We have a set of tests that run in the test framework on the testnet that we run specifically after a change.
Obviously if we do many changes in a row, we have to run these tests only ones. if we do only 1 bug, we still have to run these tests as part of the proper development process.
That's another 4 hours.

And this leaves us with 6 hours, which is quite close to 5 hours abit quoted.

Could we skip 4 hours of research? definitely not.
Could we possibly skip testing to save 4 extra hours? We never do this when we develop our own products. That would be circumventing our development process and generally it is quite bad practice. I am not sure at this point. I will have to talk to our development to raise the point of skipping tests.

Generally, I don't see any other ways of decreasing the development time.

Thanks,
Denis

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

Thanks Denis for the details. This will really help for our upcoming voice call.

My current understanding is OpenLedger proposes to develop an implementation for this Issue as a team, which is both welcome and novel for how Issues have been estimated historically as Community Claims (CC) against the Core Team WP. I believe this allows us to have a conversation about how we track and compensate a team's efforts.

In the revised estimate @OpenLedgerApp allocates hours to multiple Roles: Business Analyst (4), Developer (6) QA Tester (4) and Project Manager (2) totaling 16 hours. Specific to Developer hours, you are just beyond the upper range of the estimate @abitmore provided. However, historically, CC by individual devs have included the QA Testing in their estimates, so I assume his estimate included that. We may be farther apart on estimates as a result. Typically the Core team is doing the BA work within the Issue for the CC devs and I've been doing some of the PM work and Core Team does another round of QA prior to merge.

I value all of these roles as contributors within a highly functioning development team; that's why the Core Team is comprised of all of these (plus a documentation specialist). I believe we will arrive at solution to receive your team's contributions and properly compensate them for the effort. Look forward to chatting, Ryan

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

Just a remark,
these are all developers hours and activites (for 1 core developer that works with this task), except 2 hours for project coordination.
So for this particular feature request implementation, it's not a team effort.
Thanks, Denis

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

This can be done in a client or standalone tool. No need for an API call.
API call has the additional disadvantage that it opens a DoS attack vector on the node - recovering keys from signatures is computationally expensive.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

We have made PR - #1635
This PR describes how to add new commands to CLI: get_transaction_signers, get_key_references

@abitmore abitmore added this to To do in Feature Release (3.1.0) via automation Mar 6, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Resolved by #1635

@pmconrad pmconrad closed this Apr 11, 2019

Feature Release (3.1.0) automation moved this from To do to Done Apr 11, 2019

@pmconrad pmconrad removed this from In Development in Community Claims Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.