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

Semantic Validation for Messages #703

Merged
merged 3 commits into from
Sep 15, 2020
Merged

Semantic Validation for Messages #703

merged 3 commits into from
Sep 15, 2020

Conversation

ec2
Copy link
Member

@ec2 ec2 commented Sep 15, 2020

Summary of changes
Changes introduced in this pull request:

  • It's in the title!

Reference issue to close (if applicable)

Closes

Other information and links

@ec2 ec2 added the Blockchain label Sep 15, 2020
austinabell
austinabell reviewed Sep 15, 2020
View reviewed changes
vm/message/src/lib.rs
Comment on lines +49 to +50
/// semantic validation of the message
fn valid_for_block_inclusion(&self, min_gas: i64) -> Result<(), String>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this needs to be part of the message trait? Isn't this only needed for unsigned messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be used on signed messages in the Mpool

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason against putting it in the trait?

Copy link
Contributor

Choose a reason for hiding this comment

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

alrighty, but the check only needs the unsigned message, I'm fine with how it is, but consider just calling .message() on the signed message when you need this instead of expanding this trait (already getting bulky and this doesn't seem like it should be part of the trait).

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason against putting it in the trait?

not syntactically, but logically it doesn't make sense to me, might be opinionated

Copy link
Member Author

Choose a reason for hiding this comment

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

hehe, i like it in the trait more. im gunna leave it for now.

vm/message/src/unsigned_message.rs Outdated Show resolved Hide resolved
vm/message/src/unsigned_message.rs Outdated Show resolved Hide resolved
vm/message/src/unsigned_message.rs Show resolved Hide resolved
austinabell
austinabell approved these changes Sep 15, 2020
View reviewed changes
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.

LGTM, other change is pretty opinionated and can be altered later if needed

dutterbutter
dutterbutter approved these changes Sep 15, 2020
View reviewed changes
vm/message/Cargo.toml Outdated Show resolved Hide resolved
@ec2 ec2 merged commit dc0ff4c into main Sep 15, 2020
@ec2 ec2 deleted the ec2/msg-semantics branch September 15, 2020 18:43
timvermeulen added a commit that referenced this pull request Sep 17, 2020
@timvermeulen
Squashed commit of the following:
2b39da1 
commit 8ef5ae5
Author: Austin Abell <austinabell8@gmail.com>
Date:   Wed Sep 16 12:30:01 2020 -0400

    Peer stats tracking and selection (#701)

    * Added peer manager logic and started to move blocksync handling into network context

    * Migrate to using handled blocksync requests and remove a duplicate code

    * Fix test use

    * Fix duration subtract logic

    * Update peer manager updates from hello requests

    * Docs and cleanup

    * more cleanup

commit dc0ff4c
Author: Eric Tu <6364934+ec2@users.noreply.github.com>
Date:   Tue Sep 15 14:43:18 2020 -0400

    Semantic Validation for Messages (#703)

    * semantic validation

    * suggestions

    * suggestions

commit 3411459
Author: Austin Abell <austinabell8@gmail.com>
Date:   Mon Sep 14 15:14:48 2020 -0400

    ChainSync refactor (#693)

    * Switch chain store to threadsafe

    * Update genesis to arc reference

    * wip refactoring chain sync to workers in async tasks

    * Update network event handling and remove NetworkHandler

    * Update tipset scheduling logic

    * Update peer retrieval to take a random sample of available peers

    * Cleanup and enabling all existing tests

    * fix worker task spawn

    * Add TODO for emit event ignoring and change to error log

    * oops

    * Update comment

    * Fix typo

commit 66ca99e
Author: Eric Tu <6364934+ec2@users.noreply.github.com>
Date:   Mon Sep 14 13:48:22 2020 -0400

    Fix StateManager use in different components (#694)

    * wrap sm in arc

    * lint

    * fix tests

    * suggestions

    * clippy

commit 96b64cb
Author: nannick <rajarupans@gmail.com>
Date:   Mon Sep 14 10:33:27 2020 -0400

    Drand ignore env variable (#697)

    * First commit

    * Using enviromental vairable instead of cli flag

    * FIxing dumb mistakes

    * Renaming drand flag

    * UPdate latest_beacon_entry

    * Adding drand check

    * Adding doc

commit 548a464
Author: Austin Abell <austinabell8@gmail.com>
Date:   Thu Sep 10 13:22:22 2020 -0400

    Print out conformance results and add log for skips (#695)

commit 0d7b16c
Author: Stepan <s.s.naumov@gmail.com>
Date:   Tue Sep 8 09:01:05 2020 -0400

    Add CLI command to add Genesis Miner to Genesis Template (#644)

    * AddMinerGenesis cmd command

    * fixing tests

    * addressing comments

    * removing jsons
timvermeulen added a commit that referenced this pull request Sep 26, 2020
@timvermeulen @austinabell
Update Miner actor (#691)
f007210 
* WIP

* More changes

* Even more changes

* Amt docs

* Clippy & copyright

* Squashed commit of the following:

commit 8ef5ae5
Author: Austin Abell <austinabell8@gmail.com>
Date:   Wed Sep 16 12:30:01 2020 -0400

    Peer stats tracking and selection (#701)

    * Added peer manager logic and started to move blocksync handling into network context

    * Migrate to using handled blocksync requests and remove a duplicate code

    * Fix test use

    * Fix duration subtract logic

    * Update peer manager updates from hello requests

    * Docs and cleanup

    * more cleanup

commit dc0ff4c
Author: Eric Tu <6364934+ec2@users.noreply.github.com>
Date:   Tue Sep 15 14:43:18 2020 -0400

    Semantic Validation for Messages (#703)

    * semantic validation

    * suggestions

    * suggestions

commit 3411459
Author: Austin Abell <austinabell8@gmail.com>
Date:   Mon Sep 14 15:14:48 2020 -0400

    ChainSync refactor (#693)

    * Switch chain store to threadsafe

    * Update genesis to arc reference

    * wip refactoring chain sync to workers in async tasks

    * Update network event handling and remove NetworkHandler

    * Update tipset scheduling logic

    * Update peer retrieval to take a random sample of available peers

    * Cleanup and enabling all existing tests

    * fix worker task spawn

    * Add TODO for emit event ignoring and change to error log

    * oops

    * Update comment

    * Fix typo

commit 66ca99e
Author: Eric Tu <6364934+ec2@users.noreply.github.com>
Date:   Mon Sep 14 13:48:22 2020 -0400

    Fix StateManager use in different components (#694)

    * wrap sm in arc

    * lint

    * fix tests

    * suggestions

    * clippy

commit 96b64cb
Author: nannick <rajarupans@gmail.com>
Date:   Mon Sep 14 10:33:27 2020 -0400

    Drand ignore env variable (#697)

    * First commit

    * Using enviromental vairable instead of cli flag

    * FIxing dumb mistakes

    * Renaming drand flag

    * UPdate latest_beacon_entry

    * Adding drand check

    * Adding doc

commit 548a464
Author: Austin Abell <austinabell8@gmail.com>
Date:   Thu Sep 10 13:22:22 2020 -0400

    Print out conformance results and add log for skips (#695)

commit 0d7b16c
Author: Stepan <s.s.naumov@gmail.com>
Date:   Tue Sep 8 09:01:05 2020 -0400

    Add CLI command to add Genesis Miner to Genesis Template (#644)

    * AddMinerGenesis cmd command

    * fixing tests

    * addressing comments

    * removing jsons

* Delete old deadline tests

* Fix verify_block_signature

* Fix state_api

* Silent unused variable warnings

* clippy

* clippy

* clippy

* Fix miner worker addr function

* fix load_sectors_from_set

* Fix get_miner_info

* miner faults update

* Setup faults and recoveries logic

* Add todo for checking verification function

* Update to specs-actors v0.9.3

* Fix test

* Remove Cid.Undef check

* PR fixes

* Use the actor_error! macro in the miner actor

* Fix type mismatch

* PR changes

* Use return value

* Rename file

* More PR fixes

* Add assertions and error message

* Downcast errors originating from Deadlines::{load_deadline, update_deadline} and State::save_deadlines

* Rename Sectors::{new, load} to Sectors::{load, load_sector}

* Fix inverted logic

* Fix state manager

* Remove unused parameters

* clippy

* Replace function by const, remove duplication, remove comment

* Rename field, fix macro formatting, remove unnecessary vec

* Downcast errors

Co-authored-by: austinabell <austinabell8@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@austinabell austinabell austinabell approved these changes

@dutterbutter dutterbutter dutterbutter approved these changes

@ansermino ansermino Awaiting requested review from ansermino

@StaticallyTypedAnxiety StaticallyTypedAnxiety Awaiting requested review from StaticallyTypedAnxiety

@RajarupanSampanthan RajarupanSampanthan Awaiting requested review from RajarupanSampanthan

@timvermeulen timvermeulen Awaiting requested review from timvermeulen

Assignees
No one assigned
Labels
Blockchain Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ec2 @austinabell @dutterbutter