- 
                Notifications
    You must be signed in to change notification settings 
- Fork 47
Replace ExecutedAndBlockMessages with individual methods #1040
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
Conversation
813d043    to
    252ee71      
    Compare
  
    | Codecov Report❌ Patch coverage is  Additional details and impacted files@@           Coverage Diff            @@
##           master   #1040     +/-   ##
========================================
- Coverage    34.4%   34.4%   -0.1%     
========================================
  Files          44      44             
  Lines        2925    2930      +5     
========================================
  Hits         1008    1008             
- Misses       1821    1826      +5     
  Partials       96      96             🚀 New features to boost your workflow:
 | 
24648f0    to
    72909f1      
    Compare
  
    72909f1    to
    0490708      
    Compare
  
            
          
                lens/interface.go
              
                Outdated
          
        
      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.
| func (mri *MessageReceiptIterator) Next() (types.ChainMsg, int, *types.MessageReceipt) { | |
| func (mri *MessageReceiptIterator) Next() (types.ChainMsg, *types.MessageReceipt) { | 
is the index necessary as part of the return value? i did a quick scan of where this is being called and only saw it once here.
could the index also be accessed as:
msg, rec := itr.Next()
idx := itr.exeIdx[msg]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.
Ah, I will comment this, idx is the messages execution index in the tipset - the order the message was executed in relative to other messages in the tipset.
        
          
                lens/lily/impl.go
              
                Outdated
          
        
      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.
What does pts mean? parent tipset?
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.
Parent tipset yes. aka ts.Parent()
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 didn't see any glaring logical errors in any of the tasks nor in the TipSetMessageReceipts() function. My previous comments are not blockers.
        
          
                chain/datasource/datasource.go
              
                Outdated
          
        
      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.
| func (t *DataSource) ShouldBrunFn(ctx context.Context, ts *types.TipSet) (lens.ShouldBurnFn, error) { | |
| func (t *DataSource) ShouldBurnFn(ctx context.Context, ts *types.TipSet) (lens.ShouldBurnFn, error) { | 
        
          
                lens/lily/impl.go
              
                Outdated
          
        
      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.
| // so we only load the receipt array one | |
| // so we only load the receipt array once | 
        
          
                lens/lily/impl.go
              
                Outdated
          
        
      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.
Is the VM very "heavy" in size? Should vmi be memoized within LilyNodeAPI and have the appropriate statetree applied at the time of request within the wrapper?
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.
Great question. Currently, this method is only called by the GasOuptut task, so until another task needs to call this method we can leave out caching.
0490708    to
    0aa04d3      
    Compare
  
            
          
                tasks/messages/gaseconomy/task.go
              
                Outdated
          
        
      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.
#1045 fixed here, this calls the lotus ComputeBaseFee method directly: https://github.com/filecoin-project/lotus/blob/master/chain/store/basefee.go#L50
        
          
                tasks/messages/message/task.go
              
                Outdated
          
        
      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.
fixes #1043 by correctly calculating the message size on chain:
https://github.com/filecoin-project/lotus/blob/v1.17.0/chain/types/message.go#L81
https://github.com/filecoin-project/lotus/blob/v1.17.0/chain/types/signedmessage.go#L81
        
          
                lens/lily/impl.go
              
                Outdated
          
        
      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 have implemented this method in lotus: filecoin-project/lotus#9186, initial review (walked through with the lotus team together) suggests this is correct - as does the comparison with existing data in the production database. Once filecoin-project/lotus#9186 merges I will replace this with a direct call to the lotus method.
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.
These tasks were simplified to require a single tipset to operate, resulting from the removal of ExecutedAndBlockMessages which requires a child and a parent tipset.
        
          
                lens/lily/impl.go
              
                Outdated
          
        
      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.
Parent tipset yes. aka ts.Parent()
        
          
                lens/interface.go
              
                Outdated
          
        
      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.
Ah, I will comment this, idx is the messages execution index in the tipset - the order the message was executed in relative to other messages in the tipset.
        
          
                lens/lily/impl.go
              
                Outdated
          
        
      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.
Great question. Currently, this method is only called by the GasOuptut task, so until another task needs to call this method we can leave out caching.
What
This PR removes the
ExecutedAndBlockMessagesmethod and replaces it smaller more specific methods:TipSetMessageReceipts,TipSetBlockMessages, andComputeGasOutputs.While implementing, I noticed a few bugs in the now replaced
ExecutedAndBlockMessagesmethod, which are tracked in #1045 and #1043 and fixed in this PR. Furthermore, I took the opportunity to address #1041 while implementingTipSetMessageReceipts.