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

Bugfix: Runtime's Receiver() should only return ID addresses #3589

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Sep 6, 2020

No description provided.

@anorth
Copy link
Member

anorth commented Sep 8, 2020

😮 .

Could we add an assertion or something directly next to the return point of Receiver() and Caller() that will summon the four horsemen if the address it's about to return is not an ID-address?

@arajasek
Copy link
Contributor Author

arajasek commented Sep 8, 2020

Note to self to implement @anorth's suggestion above^ before merging this

@arajasek arajasek mentioned this pull request Sep 16, 2020
21 tasks
alanshaw pushed a commit to filecoin-project/test-vectors that referenced this pull request Sep 16, 2020
Uses the new chaos `InspectRuntime` method to test that `Caller()` and `Receiver()` always return an ID address not a robust address.

Depends on:

* [x] filecoin-project/lotus#3861

Confirmed that the following PR fixes the vector that is broken:

* filecoin-project/lotus#3589

resolves #128

func (m *Message) Caller() address.Address {
if m.msg.From.Protocol() != address.ID {
panic("runtime message has a non-ID caller")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay to just panic here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably

@arajasek
Copy link
Contributor Author

arajasek commented Sep 24, 2020

Will need to be merged into the 0.9.0 branch behind an upgrade version check

@magik6k magik6k added the P0 P0: Critical Blocker label Oct 5, 2020
@arajasek arajasek requested a review from raulk as a code owner October 6, 2020 05:26
@arajasek arajasek changed the base branch from master to asr/spec-v1 October 6, 2020 05:29
@arajasek
Copy link
Contributor Author

arajasek commented Oct 6, 2020

Good for review, should be merged into next after #3936 lands

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

This definitely should get some time in a devnet

Copy link
Contributor

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

High level seems ok

Base automatically changed from asr/spec-v1 to next October 6, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chain/legacyvm Area: Chain/VM impact/consensus Impact: Consensus kind/bug Kind: Bug P0 P0: Critical Blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants