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

Implement multisig #105

Merged
merged 11 commits into from
Aug 5, 2019
Merged

Implement multisig #105

merged 11 commits into from
Aug 5, 2019

Conversation

Kubuxu
Copy link
Contributor

@Kubuxu Kubuxu commented Jul 29, 2019

Resolves #38

@Kubuxu
Copy link
Contributor Author

Kubuxu commented Jul 29, 2019

Only Propose and Approve right now, no tests.

@Kubuxu Kubuxu marked this pull request as ready for review July 29, 2019 21:20
@Kubuxu
Copy link
Contributor Author

Kubuxu commented Jul 29, 2019

I want to write tests before this is merged.

whyrusleeping
whyrusleeping reviewed Jul 29, 2019
View reviewed changes
chain/actors/actor_multisig.go
if aerrors.IsFatal(err) {
return nil, err
}
tx.RetCode = aerrors.RetCode(err)
Copy link
Member

Choose a reason for hiding this comment

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

retcode stuff needs to be added to the spec

whyrusleeping
whyrusleeping reviewed Jul 29, 2019
View reviewed changes
chain/actors/actor_multisig.go
if params.Req < 1 {
return nil, aerrors.New(5, "requirement must be at least 1")
}
self.Required = params.Req
Copy link
Member

Choose a reason for hiding this comment

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

thinking about it, we should probably disallow setting Required higher than len(signers). Thats just an unnecessary way for people to shoot themselves in the foot.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, if we call RemoveSigner and Required > len(signers) after the operation, we should either drop required down by one, or error out

@Kubuxu
Copy link
Contributor Author

Kubuxu commented Jul 31, 2019

While writing tests I got this error:

--- FAIL: TestMultiSigOps (0.00s)
    actor_multisig_test.go:69: Applying message: during invoke:
            github.com/filecoin-project/go-lotus/chain/vm.(*VM).ApplyMessage
                /home/kubuxu/go/src/github.com/filecoin-project/go-lotus/chain/vm/vm.go:260
          - saving state (FATAL):
            github.com/filecoin-project/go-lotus/chain/actors.MultiSigActor.Propose
                /home/kubuxu/go/src/github.com/filecoin-project/go-lotus/chain/actors/actor_multisig.go:201
          - could not put new head (FATAL):
            github.com/filecoin-project/go-lotus/chain/actors.MultiSigActor.save
                /home/kubuxu/go/src/github.com/filecoin-project/go-lotus/chain/actors/actor_multisig.go:156
          - putting cid (FATAL):
            github.com/filecoin-project/go-lotus/chain/vm.(*storage).Put
                /home/kubuxu/go/src/github.com/filecoin-project/go-lotus/chain/vm/vm.go:60
          - invalid state: value already consumed
FAIL
FAIL	github.com/filecoin-project/go-lotus/chain/actors	0.035s
ok  	github.com/filecoin-project/go-lotus/chain/actors/aerrors	(cached)

@whyrusleeping can you help, I have no idea where it comes from

@whyrusleeping
Copy link
Member

Those errors almost always mean you forgot to register a type with the cbor library

@Kubuxu
Copy link
Contributor Author

Kubuxu commented Jul 31, 2019

Now it doesn't work because Send from an actor does not transfer funds.

@Kubuxu
Start implementing multisig
a48d38d 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Implement rest of the methods
d2edad5 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Clean lint
c2cb2c4 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
More save reuse
b6b4c04 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Make array prealloc correct size
f7a8d11 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Change op order
0a37ca0 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Wip on tests
f19b8c8 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Fix MTransaction
8e4661e 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Dual impl
9577348 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Remove old send
b44f778 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Add spec changes
a6bc33a 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
whyrusleeping
whyrusleeping reviewed Aug 5, 2019
View reviewed changes
chain/actors/harness_test.go
@@ -64,7 +64,7 @@ func NewHarness(t *testing.T) *Harness {

h.cs = store.NewChainStore(h.bs, nil)

h.vm, err = vm.NewVM(stateroot, IAMethods.Exec, maddr, h.cs)
h.vm, err = vm.NewVM(stateroot, 1, maddr, h.cs)
Copy link
Member

Choose a reason for hiding this comment

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

magic numbers?

Copy link
Member

Choose a reason for hiding this comment

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

oh thats block height. nvm

whyrusleeping
whyrusleeping approved these changes Aug 5, 2019
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This look good. The multisig methods match the spec (post spec changes), could use more tests, but i'm not gonna be stickler there yet, and the changes to send look correct to me. Ship it!

@whyrusleeping whyrusleeping merged commit c18711b into master Aug 5, 2019
@whyrusleeping whyrusleeping deleted the feat/multisig branch August 5, 2019 19:08
nonsense pushed a commit that referenced this pull request Nov 6, 2020
@raulk
make the testkit friends with local:exec. (#105)
8d5a8b0
dumikau pushed a commit to protofire/lotus that referenced this pull request Jun 8, 2023
@adlrocha
integrate new genesis in spacenet binary (filecoin-project#105)
0572d92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Multisig
2 participants
@Kubuxu @whyrusleeping