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

Implement optimal message selection #1086

Merged
merged 8 commits into from
Apr 28, 2021
Merged

Conversation

creativcoder
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

Other information and links

blockchain/message_pool/src/msg_chain.rs Outdated Show resolved Hide resolved
blockchain/message_pool/src/msg_chain.rs Outdated Show resolved Hide resolved
@creativcoder creativcoder force-pushed the creativcoder/opt_sel_fixed branch 2 times, most recently from 1d04867 to ed44807 Compare April 26, 2021 15:46
Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

slotmap looks really neat! However, I'm afraid I'm not able to provide review without further context.

Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

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

LGTM aside from the clippy issues and nightly build check.

Cargo.lock Outdated Show resolved Hide resolved
Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Thank you for the HackMD primer you made on Optimal Message Selection. It was very useful to understand the context of this PR.

I don't have specific comments on this PR, but I would like to ask a few general questions.

Do you have an idea of how many messages will be in the SlotMap each time?

Because the SlotMap is frequently cleared, I wonder if we shouldn't use the HopSlotMap, with a predefined (rough estimate) of capacity, using with_capacity_and_key, and then using the clear method to explicitly clear its contents.

Also, I noticed you removed the comment "TODO: Implement guess gas". Is that implemented now, say, via chain message? If so, are we checking against the new MIN_GAS constant anywhere the new, dynamic min_gas value should be used instead?

Finally, do you have any test cases? How have you observed that this new message selection algorithm works?

@creativcoder
Copy link
Contributor Author

creativcoder commented Apr 27, 2021

Thank you for the HackMD primer you made on Optimal Message Selection. It was very useful to understand the context of this PR.

I don't have specific comments on this PR, but I would like to ask a few general questions.

Do you have an idea of how many messages will be in the SlotMap each time?

Glad you found it helpful Hunter. Um, @ec2 might have a better answer on that. When I ran the tests for optimal selection (3 of them) I get about 1437 messages picked out of the selection method. It will usually be variable though.

Because the SlotMap is frequently cleared, I wonder if we shouldn't use the HopSlotMap, with a predefined (rough estimate) of capacity, using with_capacity_and_key, and then using the clear method to explicitly clear its contents.

We don't delete anything from the map anytime during the algorithm, it gets de-allocated at end of the method. We just remove, invalidate pointers to them.

Also, I noticed you removed the comment "TODO: Implement guess gas". Is that implemented now, say, via chain message? If so, are we checking against the new MIN_GAS constant anywhere the new, dynamic min_gas value should be used instead?

So gas_guess is actually referring to a module from lotus impl. As of now we only use two constants from it and we don't have the rest of entities being used anywhere in forest. Yeah, it's better to put the TODO comment back, actually at the top where MIN_GAS constant is defined.

Finally, do you have any test cases? How have you observed that this new message selection algorithm works?

Yes, selection.rs contains 3 test cases for optimal selection. Starts here: selection.rs They are direct ports of lotus' selection_test.go

Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

I feel comfortable with those answers! Fantastic work!

I think we should get this in before we merge #1033 due to our changes to how the keystore is created (Memory, Persistent, Encrypted). We'll handle those changes when we update our branch.

@cryptoquick
Copy link
Contributor

Also, don't forget to make the linter happy!

https://app.circleci.com/pipelines/github/ChainSafe/forest/4132/workflows/6168d5f9-bf19-40ad-a466-55f4d693c3c4/jobs/15493/parallel-runs/0/steps/0-111

@creativcoder creativcoder merged commit 946f451 into main Apr 28, 2021
@creativcoder creativcoder deleted the creativcoder/opt_sel_fixed branch April 28, 2021 14:07
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.

Implement a correct chain abstraction for optimal message selection
3 participants