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

Ashanti/connect state transition #454

Merged
merged 35 commits into from
Jun 2, 2020

Conversation

StaticallyTypedAnxiety
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Looks like the channel was being used for concurrency, didn't implement due to rust safety guarantees
    -a bit of a translation of tipset_state in lotus
    -missing a few functions that call tipset_state
    -Changes statemanager in sync to an arc readlock due to the need of shared reference in asynchronous context

Reference issue to close (if applicable)

Closes #405

Other information and links

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

You also need to revert the changes to the submodule you committed

blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/blocks/src/header.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
@ec2
Copy link
Member

ec2 commented May 27, 2020

Was there a reason why you removed the concurrent validation of block headers?

@austinabell
Copy link
Contributor

austinabell commented May 27, 2020

Was there a reason why you removed the concurrent validation of block headers?

I was going to come back to this in a bit but might as well now, before it was spawning a blocking task which is just synconous (I didn't look too much in detail at the last PR just assumed you guys had verified it) Think it just needs to be changed to task::spawn from what it was

(I actually don't know how the FuturesUnordered handles these boxed async functions being passed in or when they are executed but I can look into it)

@StaticallyTypedAnxiety
Copy link
Contributor Author

StaticallyTypedAnxiety commented May 27, 2020

tually don't know how the FuturesUnordered handles these boxed async functions being passed in or when they are executed but I can look into it)

Yeah they have to be boxed because each future has a different type even though in essence it looks the same. So adding different types is according to the future trait rather than a concrete future type.

It looked like it was returning join handles with futures inside of them and using spawn_blocking so in essence didn't see much of a difference between handling them where they are vs handling them at the end where the errors are being collected.

Changed to spawn right now

blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

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

Should add some comments to the public methods if possible.

blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
blockchain/state_manager/src/lib.rs Outdated Show resolved Hide resolved
@StaticallyTypedAnxiety StaticallyTypedAnxiety merged commit 2ad2399 into master Jun 2, 2020
@StaticallyTypedAnxiety StaticallyTypedAnxiety deleted the ashanti/connect_state_transition branch June 2, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connecting state transition to chain system
4 participants