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

Checks to-address for finality before adding message to mempool #7795

Closed
wants to merge 1 commit into from

Conversation

geoff-vball
Copy link
Contributor

Closes issue #4368

@geoff-vball geoff-vball requested a review from a team as a code owner December 15, 2021 00:55
@geoff-vball geoff-vball force-pushed the feat/mempool-to-address-finality branch from ff83c7c to a5253fb Compare December 15, 2021 01:51
@geoff-vball geoff-vball force-pushed the feat/mempool-to-address-finality branch from a5253fb to c4e3222 Compare December 15, 2021 02:22
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

So this doesn't quite work, unfortunately. StateAccountKeyAtFinality will fail if called with something that isn't a secp / bls account -- something like a miner actor doesn't have an "account key", although it does have a robust address.

I think we need to do the following:

  • check if To is an ID address. If it is,
  • check if it existed 900 epochs ago. if it didn't,
  • check if the To actor is an account. if it is,
  • fail: they should be using the pubkey address instead.

Really we should be failing even if the To actor isnt' an account, but that's gonna break a looot of tooling, so I'd rather just see this land first.

@jennijuju
Copy link
Member

jennijuju commented Jan 11, 2022

@arajasek feel like we should include this in v1.14.0?

@jennijuju jennijuju marked this pull request as draft April 19, 2022 15:19
@jennijuju jennijuju modified the milestone: Network v18 Feb 4, 2023
@arajasek arajasek closed this Feb 20, 2023
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.

3 participants