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

Make state tipset usage consistent in the API #4545

Merged
merged 10 commits into from
Feb 5, 2021

Conversation

Stebalien
Copy link
Member

Always (almost) use the tipset's parent state, instead of computing.

Exceptions:

  • MinerGetBaseInfo. Fixing this would break things so we need to be careful (although we could bump the API version, fix it, then fix the call sites).
  • StateReplay. This is replaying a message on top of the given tipset.
  • GasEstimateGasLimit. This executes the message on-top-of the tipset's computed state (unlike call which executes it on the tipset's parent state).
    • Having this method and Call apply the message at different heights is really weird.

@Stebalien Stebalien marked this pull request as draft October 22, 2020 19:32
@Stebalien
Copy link
Member Author

Stebalien commented Oct 22, 2020

The test failures look problematic.

(resolved)

@Stebalien Stebalien force-pushed the steb/refactor-consistent-tipset-methods branch 2 times, most recently from 21e13ef to c6a7c6d Compare October 23, 2020 01:38
@@ -144,20 +144,6 @@ func MinerSectorInfo(ctx context.Context, sm *StateManager, maddr address.Addres
return mas.GetSector(sid)
}

func GetMinerSectorSet(ctx context.Context, sm *StateManager, ts *types.TipSet, maddr address.Address, snos *bitfield.BitField) ([]*miner.SectorOnChainInfo, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a drive-by fix. Removed because it was only being used in one place at this point.

err error
}

type cachedActorLookup struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

dead code

@@ -109,7 +108,7 @@ func (a *StateAPI) StateMinerActiveSectors(ctx context.Context, maddr address.Ad
return nil, xerrors.Errorf("merge partition active sets: %w", err)
}

return stmgr.GetMinerSectorSet(ctx, a.StateManager, ts, maddr, &activeSectors)
return mas.LoadSectors(&activeSectors)
Copy link
Member Author

Choose a reason for hiding this comment

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

This avoids loading the actor twice.

node/modules/mpoolnonceapi.go Show resolved Hide resolved
@Stebalien
Copy link
Member Author

The tests pass but there are still outstanding questions.

@Stebalien Stebalien force-pushed the steb/refactor-consistent-tipset-methods branch 2 times, most recently from 3188820 to 0b37dda Compare October 26, 2020 17:46
@Stebalien Stebalien marked this pull request as ready for review October 26, 2020 17:47
@arajasek
Copy link
Contributor

1.2.1 is probably the right release for this, after careful review and letting it run on our nodes for a while

_Always_ (almost) use the tipset's parent state, instead of computing.

Exceptions:

* MinerGetBaseInfo. Fixing this would break things so we need to be
careful (although we could bump the API version, fix it, then fix the call
sites).
* StateReplay. This is replaying a message on top of the given tipset.
* GasEstimateGasLimit. This executes the message on-top-of the tipset's
computed state (unlike call which executes it on the tipset's parent state).
  * Having this method and Call apply the message at different heights is really
  weird.
… the tipset

We could also do this in the message pool itself, but I'm not sure if it's worth it?
StateGetActor now gets the actor at the tipset parent state.
@Stebalien Stebalien force-pushed the steb/refactor-consistent-tipset-methods branch from 6d1ac1e to b78b9a4 Compare December 9, 2020 19:29
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.

Would be nice to finally land this.

Not entirely sure why, but tests fail on latest master with this

return act.Nonce, nil
for _, msg := range msgs {
vmmsg := msg.VMMessage()
if vmmsg.From != keyAddr {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think msg.From can technically also be an ID address, though I don't see messages like that on-chain

Copy link
Member Author

Choose a reason for hiding this comment

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

You sure? It looks like message verification will fail if the from address is not a public key.

if err := sigs.Verify(&m.Signature, m.Message.From, m.Message.Cid().Bytes()); err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Uff we should fix it.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, you should abosolutely be able to send a message with an ID address as the from

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we can add that to the network upgrade. Note: we'll need to carefully check the VM to make sure we resolve everything as expected before passing it off to the actors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: we currently implicitly resolve the from address when sending. Should we not be doing that?

node/modules/mpoolnonceapi.go Outdated Show resolved Hide resolved
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.

Looks good. Also did some manual testing and everything appears to work well

@magik6k magik6k merged commit ec58099 into master Feb 5, 2021
@magik6k magik6k deleted the steb/refactor-consistent-tipset-methods branch February 5, 2021 20:57
bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Feb 22, 2021
…efactor-consistent-tipset-methods

Make state tipset usage consistent in the API
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.

5 participants