feat: compute gas outputs #67

iand · 2020-10-06T14:09:12Z

This change adds a gas outputs processing task. The results of the task are written to
a new table derived_gas_outputs. The task fetches batches of messages that have receipts
and calls the lotus vm ComputeGasOutputs method, persisting the results.

This change also performs a significant refactor of the visor processing tables. Previously
the chain indexers would write duplicate rows to visor_processing_statechanges and
visor_processing_messages. Each of those tables contained claimed_until, completed_at and
errors_detected columns. They were used by the ActorStateChange and Message tasks. However
if we were to add another task that processed tipsets we would need to add another processing
table and then reindex the entire chain to populate it.

To avoid this, the visor_processing_statechanges and visor_processing_messages tables have
been combined into a single visor_processing_tipsets table. Each task now has a set of three
columns in this table to track its work: statechange_claimed_until/statechange_completed_at/
statechange_errors_detected and message_claimed_until/message_completed_at/message_errors_detected.

Adding a new tipset-oriented task is then a matter of adding three columns to this table.

The visor_processing_messages table has been repurposed to track work that needs to be performed
on each message seen. The GasOutputs task is the first task to use this table. It uses the
gas_outputs_claimed_until, gas_outputs_completed_at and gas_outputs_errors_detected columns.

Additional message-oriented tasks can be supported by adding new columns to this table, avoiding
the need to read every message in the chain again.

fixes #63

iand · 2020-10-06T14:59:07Z

storage/sql.go

+}
+
+// LeaseGasOutputsMessages leases a set of messages that have receipts for gas output processing. minHeight and maxHeight define an inclusive range of heights to process.
+func (d *Database) LeaseGasOutputsMessages(ctx context.Context, claimUntil time.Time, batchSize int, minHeight, maxHeight int64) (derived.GasOutputsList, error) {


This function contains a query that joins messages to receipts to only return messages that have corresponding receipts in the database. It also joins to block_headers to get the parent_base_fee. Is this the correct value to use?

This function contains a query that joins messages to receipts to only return messages that have corresponding receipts in the database

It should never be the case that a message without a receipt exists in the tables produced by visor since it operates on valid blocks, I believe this join on block_headers is unnecessary if its only for ensuring messages are returned that have a receipt. (spec-ref)

It also joins to block_headers to get the parent_base_fee. Is this the correct value to use?

"The ParentBaseFee is used when calculating how much a sender burns when executing a message. Burning simply refers to sending attoFIL to a dedicated, unreachable account. A message causes ParentBaseFee * GasUsed attoFIL to be burnt" - (lotus-ref).

The ParentBaseFee value being extracted looks right to me, here is where lotus gets it from: https://github.com/filecoin-project/lotus/blob/2bf42d5318d089741719a213c32e8d3fd03db139/chain/stmgr/call.go#L135

I think the join between messages and receipts is necessary so as to ensure we only process messages that have received a receipt, which happens at some time in the future. The indexer will add messages and then some time later will find the receipts for those messages so there will be a period of time where we have messages with no receipts in the database.

Plus we need the gas_used value that is only in the receipt. The join to block headers it to retrieve the parent_base_fee. Both of these are needed for the call to ComputeGasOutputs.

@frrist Thanks for the response. The join on block_headers is for returning parent_base_fee.

BTW, this query looks good to me.

iand · 2020-10-06T14:59:58Z

model/derived/gasoutputs.go

+	"golang.org/x/xerrors"
+)
+
+type GasOutputs struct {


This is what is persisted in the new derived_gas_outputs table

Any reason for duplicating the message and receipt data in this table? Is it to avoid complicating our ingestion strategy?

This is intended to be query optimized (indexed, normalized) format for supporting analysis and monitoring. This (query-optimized) schema may eventually live in another place so we shouldn't assume the presence of our ingestion schema. It also cleanly isolates our ingestion concerns from our analysis concerns.

Columns are as requested in #63

frrist · 2020-10-06T17:16:51Z

tasks/message/gasoutputs.go

+		return xerrors.Errorf("parse gas premium: %w", err)
+	}
+
+	outputs := vm.ComputeGasOutputs(item.GasUsed, item.GasLimit, baseFee, feeCap, gasPremium)


Can we express this within the Lens abstraction? This will keep our lotus deps within that package and make it easy to diversify our sources later.

placer14

A few changes and concerns to address. 🤝

placer14 · 2020-10-06T15:10:16Z

commands/run.go

+		},
+		&cli.IntFlag{
+			Name:    "gasoutputs-batch",
+			Aliases: []string{"gob"},


placer14 · 2020-10-06T15:11:50Z

commands/run.go

+		for i := 0; i < cctx.Int("gasoutputs-workers"); i++ {
+			scheduler.Add(schedule.TaskConfig{
+				Name:                fmt.Sprintf("GasOutputsProcessor%03d", i),
+				Task:                message.NewGasOutputsProcessor(rctx.db, rctx.api, cctx.Duration("gasoutputs-lease"), cctx.Int("gasoutputs-batch"), heightFrom, heightTo),


Thanks for applying --to and --from here.

placer14 · 2020-10-06T15:13:47Z

model/derived/gasoutputs.go

+	ExitCode      int64  `pg:",use_zero"`
+	GasUsed       int64  `pg:",use_zero"`
+	Return        []byte
+	ParentBaseFee string


We don't need Params, Idx, or Return. ~~I don't really mind them being present, but they do take up space.~~ I think I mind them. Let's remove. I'd be willing to consider including them if they were parsable, but we can follow up with that later.

placer14 · 2020-10-06T19:14:38Z

model/derived/gasoutputs.go

+	"golang.org/x/xerrors"
+)
+
+type GasOutputs struct {


This is intended to be query optimized (indexed, normalized) format for supporting analysis and monitoring. This (query-optimized) schema may eventually live in another place so we shouldn't assume the presence of our ingestion schema. It also cleanly isolates our ingestion concerns from our analysis concerns.

placer14 · 2020-10-06T19:24:26Z

storage/migrations/6_tipset_processing_tables.go

+	"refund" text,
+	"gas_refund" bigint,
+	"gas_burned" bigint,
+	PRIMARY KEY ("cid")


Indexes to include here:

from (using Hash)

to (using Hash)

method(using BTree)

exit_code (using BTree)

placer14 · 2020-10-06T19:26:13Z

storage/sql.go

-// LeaseActors leases a set of tipsets containing messages to process. minHeight and maxHeight define an inclusive range of heights to process.
-func (d *Database) LeaseTipSetMessages(ctx context.Context, claimUntil time.Time, batchSize int, minHeight, maxHeight int64) (visor.ProcessingMessageList, error) {
-	var messages visor.ProcessingMessageList
+// LeaseTipSetMessages leases a set of tipsets containing messages to process. minHeight and maxHeight define an inclusive range of heights to process.


Nice catch.

placer14 · 2020-10-06T19:28:10Z

storage/sql.go

+}
+
+// LeaseGasOutputsMessages leases a set of messages that have receipts for gas output processing. minHeight and maxHeight define an inclusive range of heights to process.
+func (d *Database) LeaseGasOutputsMessages(ctx context.Context, claimUntil time.Time, batchSize int, minHeight, maxHeight int64) (derived.GasOutputsList, error) {


@frrist Thanks for the response. The join on block_headers is for returning parent_base_fee.

placer14 · 2020-10-06T19:31:14Z

model/derived/gasoutputs.go

+
+	GasRefund int64
+	GasBurned int64
+}


I think all of these columns should be notnull (even the cases which use_zero).

placer14 · 2020-10-06T19:37:37Z

storage/sql.go

+        gas_outputs_completed_at = ?,
+        gas_outputs_errors_detected = ?
+    WHERE cid = ?
+`, completedAt, useNullIfEmpty(errorsDetected), cid)


useNullIfEmpty

Love it.

placer14 · 2020-10-06T19:48:46Z

tasks/message/gasoutputs.go

+		return xerrors.Errorf("parse gas premium: %w", err)
+	}
+
+	outputs := vm.ComputeGasOutputs(item.GasUsed, item.GasLimit, baseFee, feeCap, gasPremium)


Can we express this within the Lens abstraction? This will keep our lotus deps within that package and make it easy to diversify our sources later.

placer14

@iand In the interest of allowing this to move along before I'm up, I'll put a ✅ now. I know you'll see the feedback is applied appropriately. 🤝

placer14 · 2020-10-07T03:58:50Z

FYI, restarts are very aggressive.

2020-10-07T03:56:03.596Z        INFO    schedule        schedule/scheduler.go:124       task exited cleanly     {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.596Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.663Z        INFO    schedule        schedule/scheduler.go:124       task exited cleanly     {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.663Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.728Z        INFO    schedule        schedule/scheduler.go:124       task exited cleanly     {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.728Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.792Z        INFO    schedule        schedule/scheduler.go:124       task exited cleanly     {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.792Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.856Z        INFO    schedule        schedule/scheduler.go:124       task exited cleanly     {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.856Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.920Z        INFO    schedule        schedule/scheduler.go:124       task exited cleanly     {"task": "ChainHistoryIndexer"}
2020-10-07T03:56:03.920Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "ChainHistoryIndexer"}

Additionally, the higher worker counts on a pretty beefy box end up resulting in a lot of this, which is also exacerbated by the aggressive restart.

2020-10-07T03:54:32.995Z        ERROR   schedule        schedule/scheduler.go:117       task exited with failure        {"task": "GasOutputsProcessor013", "error": "FATAL #53300 sorry, too many clients al
ready"}
2020-10-07T03:54:32.995Z        INFO    schedule        schedule/scheduler.go:106       restarting task {"task": "GasOutputsProcessor013"}

This change adds a gas outputs processing task. The results of the task are written to a new table derived_gas_outputs. The task fetches batches of messages that have receipts and calls the lotus vm ComputeGasOutputs method, persisting the results. This change also performs a significant refactor of the visor processing tables. Previously the chain indexers would write duplicate rows to visor_processing_statechanges and visor_processing_messages. Each of those tables contained claimed_until, completed_at and errors_detected columns. They were used by the ActorStateChange and Message tasks. However if we were to add another task that processed tipsets we would need to add another processing table and then reindex the entire chain to populate it. To avoid this, the visor_processing_statechanges and visor_processing_messages tables have been combined into a single visor_processing_tipsets table. Each task now has a set of three columns in this table to track its work: statechange_claimed_until/statechange_completed_at/ statechange_errors_detected and message_claimed_until/message_completed_at/message_errors_detected. Adding a new tipset-oriented task is then a matter of adding three columns to this table. The visor_processing_messages table has been repurposed to track work that needs to be performed on each message seen. The GasOutputs task is the first task to use this table. It uses the gas_outputs_claimed_until, gas_outputs_completed_at and gas_outputs_errors_detected columns. Additional message-oriented tasks can be supported by adding new columns to this table, avoiding the need to read every message in the chain again.

iand · 2020-10-07T11:56:00Z

Moved ComputeGasOutputs into the node abstraction as suggested by @placer14 but github won't let me reply to that comment

iand force-pushed the iand/gas-processing branch 5 times, most recently from 28f683c to c6b5923 Compare October 6, 2020 14:45

iand marked this pull request as ready for review October 6, 2020 14:50

iand requested review from placer14 and frrist October 6, 2020 14:50

iand commented Oct 6, 2020

View reviewed changes

iand force-pushed the iand/gas-processing branch from c6b5923 to ae4e8ae Compare October 6, 2020 15:23

frrist approved these changes Oct 6, 2020

View reviewed changes

placer14 suggested changes Oct 6, 2020

View reviewed changes

placer14 approved these changes Oct 6, 2020

View reviewed changes

iand force-pushed the iand/gas-processing branch from ae4e8ae to 387bb05 Compare October 7, 2020 11:01

iand force-pushed the iand/gas-processing branch from 387bb05 to 8ff8682 Compare October 7, 2020 11:19

iand merged commit 3b98708 into master Oct 7, 2020

iand deleted the iand/gas-processing branch October 7, 2020 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: compute gas outputs #67

feat: compute gas outputs #67

iand commented Oct 6, 2020 •

edited

Loading

iand Oct 6, 2020

frrist Oct 6, 2020 •

edited

Loading

iand Oct 6, 2020

placer14 Oct 6, 2020

placer14 Oct 6, 2020

iand Oct 6, 2020

frrist Oct 6, 2020

placer14 Oct 6, 2020

iand Oct 6, 2020

frrist Oct 6, 2020

placer14 Oct 6, 2020

placer14 left a comment

placer14 Oct 6, 2020

placer14 Oct 6, 2020

placer14 Oct 6, 2020 •

edited

Loading

iand Oct 7, 2020

placer14 Oct 6, 2020

placer14 Oct 6, 2020

iand Oct 7, 2020

placer14 Oct 6, 2020

placer14 Oct 6, 2020

placer14 Oct 6, 2020

iand Oct 7, 2020

placer14 Oct 6, 2020

placer14 Oct 6, 2020

placer14 left a comment

placer14 commented Oct 7, 2020 •

edited

Loading

iand commented Oct 7, 2020

feat: compute gas outputs #67

feat: compute gas outputs #67

Conversation

iand commented Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

frrist Oct 6, 2020 • edited Loading

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

placer14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

placer14 Oct 6, 2020 • edited Loading

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

placer14 left a comment

Choose a reason for hiding this comment

placer14 commented Oct 7, 2020 • edited Loading

iand commented Oct 7, 2020

iand commented Oct 6, 2020 •

edited

Loading

frrist Oct 6, 2020 •

edited

Loading

placer14 Oct 6, 2020 •

edited

Loading

placer14 commented Oct 7, 2020 •

edited

Loading