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.

Sign up for GitHub

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

Jump to bottom

go/consensus/api/transaction: Support multi-sig accounts #3094

Closed
wants to merge 1 commit into from
Closed

go/consensus/api/transaction: Support multi-sig accounts #3094

wants to merge 1 commit into from

Conversation

Yawning
Copy link
Contributor

@Yawning Yawning commented Jul 9, 2020 •
edited
Loading

  • Add the v0 envelope format
  • Use the v0 envelope for transactions
  • Kludge the existing tooling so that at least accounts with only 1 signer work
  • Unbreak entity node signing
  • Make the node/entity/runtime descriptors use cbor.Versioned
  • Make the tests pass
  • Restore the stupid PrettyPrint crap
  • Update transaction test vector generators (staking/registry gen_vectors)
  • Update the tooling to support multi-sig accounts

Not going to fully update the tooling, since my approach to writing tools is "a jumbled mess of command line flags, that tries to be as user-hostile as possible", and apparently people actually are expected to use the things.

@Yawning Yawning added c:consensus/cometbft Category: CometBFT c:breaking/consensus Category: breaking consensus changes labels Jul 9, 2020
@codecov
Copy link

codecov bot commented Jul 9, 2020 •
edited
Loading

Codecov Report

Merging #3094 into master will decrease coverage by 0.09%.
The diff coverage is 68.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3094      +/-   ##
==========================================
- Coverage   68.71%   68.62%   -0.10%     
==========================================
  Files         375      376       +1     
  Lines       36962    37110     +148     
==========================================
+ Hits        25398    25465      +67     
- Misses       8319     8409      +90     
+ Partials     3245     3236       -9
Impacted Files Coverage Δ
go/consensus/api/api.go 50.00% <ø> (ø)
...nsensus/tendermint/apps/keymanager/transactions.go 52.94% <0.00%> (ø)
go/consensus/tendermint/apps/registry/registry.go 62.16% <0.00%> (-0.34%) ⬇️
...nsensus/tendermint/apps/staking/signing_rewards.go 23.52% <0.00%> (+1.90%) ⬆️
go/staking/tests/debug/debug_stake.go 60.00% <ø> (ø)
go/consensus/tendermint/apps/scheduler/genesis.go 32.83% <14.28%> (+0.46%) ⬆️
go/oasis-node/cmd/common/common.go 57.98% <33.33%> (ø)
go/consensus/api/transaction/transaction.go 45.91% <34.78%> (-1.40%) ⬇️
go/registry/api/api.go 42.21% <37.50%> (+0.76%) ⬆️
go/registry/api/runtime.go 45.63% <37.50%> (-1.85%) ⬇️
... and 70 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 b176b1f...f8c84a2. Read the comment docs.

@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch 3 times, most recently from 249a4f0 to 2e76469 Compare July 13, 2020 15:56
kostko
kostko reviewed Jul 13, 2020
View reviewed changes
go/consensus/api/submission.go Outdated Show resolved Hide resolved
go/common/node/node.go Outdated Show resolved Hide resolved
go/consensus/api/transaction/v0/envelope.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/state/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/api/transaction/v0/envelope.go Outdated Show resolved Hide resolved
@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch 21 times, most recently from a250e6a to 88b1dc6 Compare July 16, 2020 12:45
pro-wh
pro-wh reviewed Jul 17, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

21/92

go/common/crypto/multisig/multisig.go Outdated Show resolved Hide resolved
go/common/crypto/multisig/multisig_test.go Outdated Show resolved Hide resolved
go/common/node/node.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch from be6e92e to 3126977 Compare July 17, 2020 10:03
@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch 2 times, most recently from 33fa8fd to 5c61e78 Compare July 17, 2020 10:11
@Yawning Yawning marked this pull request as ready for review July 17, 2020 10:12
@Yawning Yawning requested a review from peterjgilbert as a code owner July 17, 2020 10:12
pro-wh
pro-wh reviewed Jul 17, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

93/93

go/consensus/tendermint/apps/staking/fees.go Outdated Show resolved Hide resolved
go/genesis/genesis_test.go Show resolved Hide resolved
go/oasis-node/cmd/genesis/genesis.go Show resolved Hide resolved
go/oasis-node/cmd/stake/stake.go Outdated Show resolved Hide resolved
tests/fixture-data/consim/genesis.json Show resolved Hide resolved
@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch 3 times, most recently from b29f679 to dc6ba36 Compare July 20, 2020 08:39
@Yawning Yawning mentioned this pull request Jul 20, 2020
Merged
kostko
kostko reviewed Jul 20, 2020
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

(22/93)

Any measurable performance impact from this change?

Some changes like using account addresses instead of public keys for entity identifiers seem useful for future proofness.

go/consensus/api/transaction/transaction.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/common/node/node.go Outdated Show resolved Hide resolved
go/consensus/api/transaction/transaction.go Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch 2 times, most recently from 6bbf800 to 2222cb4 Compare July 20, 2020 12:56
@Yawning
Copy link
Contributor Author

Yawning commented Jul 20, 2020

Any measurable performance impact from this change?

No idea, shouldn't be though.

Some changes like using account addresses instead of public keys for entity identifiers seem useful for future proofness.

Moving to cbor.Versioned is also good here, but people will cry about entity regeneration if I wanted to extract that from this branch.

@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch from 2222cb4 to 4167be2 Compare July 20, 2020 13:17
@Yawning
go/common/crypto/multisig: Initial multisig implementation
f8c84a2 
This adds preliminary internal support required for the new
multi-signature accounts, and changes everything to use the new
multi-signature account addresses and envelope.

Notes:

* Tooling to actually generate multi-signature accounts, and to sign
  transactions with accounts containing more than 1 public key is not
  yet implemented.

* The `registry node` subcommand option `node.entity_id` has been
  renamed to `node.entity_address`.
@Yawning Yawning force-pushed the yawning/feature/tx-multisig branch from 4167be2 to f8c84a2 Compare July 20, 2020 13:19
pro-wh
pro-wh approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

thanks for the changes

@Yawning Yawning mentioned this pull request Jul 21, 2020
Closed
@Yawning
Copy link
Contributor Author

Yawning commented Jul 27, 2020

Welp, we're not doing this (for real this time). I'll close the PR but leave the branch lying around for a bit, so that I can salvage stuff out of it when people inevitably change their mind again in a few months.

@Yawning Yawning closed this Jul 27, 2020
@pro-wh
Copy link
Contributor

pro-wh commented Apr 30, 2021

oasisprotocol/oasis-sdk#91 hi from 2021

@pro-wh pro-wh mentioned this pull request May 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostko kostko kostko left review comments

@pro-wh pro-wh pro-wh approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

Assignees
No one assigned
Labels
c:breaking/consensus Category: breaking consensus changes c:consensus/cometbft Category: CometBFT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Yawning @pro-wh @kostko