Replace ExecutedAndBlockMessages with individual methods #1040

frrist · 2022-08-18T20:52:23Z

What

This PR removes the ExecutedAndBlockMessages method and replaces it smaller more specific methods: TipSetMessageReceipts, TipSetBlockMessages, and ComputeGasOutputs.

While implementing, I noticed a few bugs in the now replaced ExecutedAndBlockMessages method, which are tracked in #1045 and #1043 and fixed in this PR. Furthermore, I took the opportunity to address #1041 while implementing TipSetMessageReceipts.

codecov-commenter · 2022-08-23T01:40:29Z

Codecov Report

Merging #1040 (f3d0d2e) into master (593dad6) will decrease coverage by 0.0%.
The diff coverage is 25.0%.

@@           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

kasteph · 2022-08-24T20:08:47Z

lens/interface.go

+	return false
+}
+
+func (mri *MessageReceiptIterator) Next() (types.ChainMsg, int, *types.MessageReceipt) {


Suggested change

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]

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.

kasteph · 2022-08-24T20:12:47Z

lens/lily/impl.go

+	return out, nil
+}
+
+// TipSetMessageReceipts returns the blocks and messages in `pts` and their corresponding receipts from `ts` matching block order in tipset (`pts`).


What does pts mean? parent tipset?

Parent tipset yes. aka ts.Parent()

kasteph

I didn't see any glaring logical errors in any of the tasks nor in the TipSetMessageReceipts() function. My previous comments are not blockers.

placer14 · 2022-08-25T16:12:11Z

chain/datasource/datasource.go


-		return data, innerErr
-	})
+func (t *DataSource) ShouldBrunFn(ctx context.Context, ts *types.TipSet) (lens.ShouldBurnFn, error) {


Suggested change

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) {

placer14 · 2022-08-25T19:18:10Z

lens/lily/impl.go

+			return nil, fmt.Errorf("loading message receipts %w", err)
+		}
+	}
+	// so we only load the receipt array one


Suggested change

// so we only load the receipt array one

// so we only load the receipt array once

placer14 · 2022-08-25T19:24:10Z

lens/lily/impl.go

+			CircSupplyCalc: m.StateManager.GetVMCirculatingSupply,
+			NetworkVersion: util.DefaultNetwork.Version(ctx, ts.Height()),
+			BaseFee:        ts.Blocks()[0].ParentBaseFee,
+		})


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?

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.

…essors

frrist · 2022-08-23T23:35:03Z

tasks/messages/gaseconomy/task.go

 	}

-	newBaseFee := store.ComputeNextBaseFee(executed.Blocks()[0].ParentBaseFee, totalUniqGasLimit, len(executed.Blocks()), executed.Height())
-	baseFeeRat := new(big.Rat).SetFrac(newBaseFee.Int, new(big.Int).SetUint64(build.FilecoinPrecision))
+	currentBaseFee, err := t.node.ComputeBaseFee(ctx, current)


#1045 fixed here, this calls the lotus ComputeBaseFee method directly: https://github.com/filecoin-project/lotus/blob/master/chain/store/basefee.go#L50

frrist · 2022-08-23T23:36:39Z

tasks/messages/message/task.go

+				GasFeeCap:  msg.VMMessage().GasFeeCap.String(),
+				GasPremium: msg.VMMessage().GasPremium.String(),
+				GasLimit:   msg.VMMessage().GasLimit,
+				SizeBytes:  msg.ChainLength(),


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

frrist · 2022-08-23T23:39:17Z

lens/lily/impl.go

+}
+
+// TipSetMessageReceipts returns the blocks and messages in `pts` and their corresponding receipts from `ts` matching block order in tipset (`pts`).
+func (m *LilyNodeAPI) TipSetMessageReceipts(ctx context.Context, ts, pts *types.TipSet) ([]*lens.BlockMessageReceipts, error) {


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.

frrist · 2022-08-23T23:42:42Z

chain/indexer/integrated/processor/state.go

+			out.TipsetProcessors[t] = messagetask.NewTask(api)
+		case tasktype.BlockMessage:
+			out.TipsetProcessors[t] = bmtask.NewTask(api)
+		case tasktype.MessageGasEconomy:
+			out.TipsetProcessors[t] = gasecontask.NewTask(api)
+


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.

frrist · 2022-08-24T20:23:40Z

lens/lily/impl.go

+	return out, nil
+}
+
+// TipSetMessageReceipts returns the blocks and messages in `pts` and their corresponding receipts from `ts` matching block order in tipset (`pts`).


Parent tipset yes. aka ts.Parent()

frrist · 2022-08-24T20:24:38Z

lens/interface.go

+	return false
+}
+
+func (mri *MessageReceiptIterator) Next() (types.ChainMsg, int, *types.MessageReceipt) {


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.

frrist · 2022-08-25T22:07:47Z

lens/lily/impl.go

+			CircSupplyCalc: m.StateManager.GetVMCirculatingSupply,
+			NetworkVersion: util.DefaultNetwork.Version(ctx, ts.Height()),
+			BaseFee:        ts.Blocks()[0].ParentBaseFee,
+		})


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.

frrist self-assigned this Aug 19, 2022

frrist linked an issue Aug 22, 2022 that may be closed by this pull request

Compute message receipts when necessary #1041

Closed

frrist force-pushed the frrist/message-api branch from 813d043 to 252ee71 Compare August 23, 2022 01:33

This was linked to issues Aug 23, 2022

computed message size incorrect for message task #1043

Closed

message gas economy computed base fee incorrect #1045

Closed

frrist force-pushed the frrist/message-api branch 3 times, most recently from 24648f0 to 72909f1 Compare August 23, 2022 23:31

frrist marked this pull request as ready for review August 23, 2022 23:43

frrist requested review from placer14 and kasteph August 23, 2022 23:43

frrist changed the title ~~Frrist/message api~~ Replace ExecutedAndBlockMessages with simpler methods Aug 23, 2022

frrist changed the title ~~Replace ExecutedAndBlockMessages with simpler methods~~ Replace ExecutedAndBlockMessages with individual methods Aug 23, 2022

frrist force-pushed the frrist/message-api branch from 72909f1 to 0490708 Compare August 24, 2022 19:27

kasteph reviewed Aug 24, 2022

View reviewed changes

kasteph approved these changes Aug 24, 2022

View reviewed changes

placer14 approved these changes Aug 25, 2022

View reviewed changes

frrist added 10 commits August 25, 2022 15:02

refactor: remove block messages from TipSetMessages and refactor proc…

cab83e8

…essors

chore: TipSetMessageReceipts implemented and wired up

673fcb9

chore: update miner post extractor

b40a473

polish: TipSetMessageReceipts iterator

e1d032a

refactor: update msapproaval extractor

2f0f4cb

refactor: update parsed messages task

62382e9

refactor: memoize TipSetMessageReceipts

dd42630

refactor: update gas economy

3fd0eb2

refactor: update gas outs task

f9cb788

refactor: remove ExecutedAndBlockMessages method

0aa04d3

frrist force-pushed the frrist/message-api branch from 0490708 to 0aa04d3 Compare August 25, 2022 22:02

frrist commented Aug 25, 2022

View reviewed changes

address review feedback

f3d0d2e

frrist merged commit ab09cca into master Aug 30, 2022

frrist deleted the frrist/message-api branch August 30, 2022 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ExecutedAndBlockMessages with individual methods #1040

Replace ExecutedAndBlockMessages with individual methods #1040

frrist commented Aug 18, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading

kasteph Aug 24, 2022

frrist Aug 24, 2022

kasteph Aug 24, 2022

frrist Aug 24, 2022

kasteph left a comment

placer14 Aug 25, 2022

placer14 Aug 25, 2022

placer14 Aug 25, 2022

frrist Aug 25, 2022

frrist Aug 23, 2022

frrist Aug 23, 2022

frrist Aug 23, 2022

frrist Aug 23, 2022

frrist Aug 24, 2022

frrist Aug 24, 2022

frrist Aug 25, 2022

	func (mri MessageReceiptIterator) Next() (types.ChainMsg, int, types.MessageReceipt) {
	func (mri MessageReceiptIterator) Next() (types.ChainMsg, types.MessageReceipt) {

	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) {

	// so we only load the receipt array one
	// so we only load the receipt array once

Replace ExecutedAndBlockMessages with individual methods #1040

Replace ExecutedAndBlockMessages with individual methods #1040

Conversation

frrist commented Aug 18, 2022 • edited Loading

What

codecov-commenter commented Aug 23, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasteph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frrist commented Aug 18, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading