-
Notifications
You must be signed in to change notification settings - Fork 153
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
Conversation
560e8bb
to
8ac8289
Compare
1d04867
to
ed44807
Compare
There was a problem hiding this 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.
a5fadc2
to
3722bcd
Compare
There was a problem hiding this 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.
There was a problem hiding this 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?
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.
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.
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.
Yes, |
There was a problem hiding this 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.
Also, don't forget to make the linter happy! |
1a842f4
to
03e194e
Compare
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links