feat(gas outputs): Add Height and ActorName #270

hsanjuan · 2020-11-26T16:30:28Z

Gas outputs are very important for many base-fee and gas-usage related views.

The table has no height, which means it needs to be joined with blocks all the
time on something like state_root to get a timestamp. This is likely
innefficient.

Additionally, it needs to be joined with actors to get the actor code. Since
we are "deriving" and already include redundant info, we might also get this
one. This allows easy filtering of messages by actor type and method.

Gas outputs are very important for many base-fee and gas-usage related views. The table has no height, which means it needs to be joined with blocks all the time on something like state_root to get a timestamp. This is likely innefficient. Additionally, it needs to be joined with actors to get the actor code. Since we are "deriving" and already include redundant info, we might also get this one. This allows easy filtering of messages by actor type and method.

model/derived/gasoutputs.go

hsanjuan · 2020-11-26T16:32:12Z

model/derived/gasoutputs.go

@@ -12,6 +12,7 @@ import (

 type GasOutputs struct {
 	tableName          struct{} `pg:"derived_gas_outputs"` //nolint: structcheck,unused
+	Height             int64    `pg:",pk,use_zero,notnull"`


We will need to backfill the value for this column.

The value corresponds to the height of the message I think (not that of the receipts).

Yes, it's the height of the message. Backfilling is a problem. With the current version it will take some time. With the new in-order processing it takes <1s per tipset, but that's still ~211000 seconds = ~2.4 days unless we shard the processing.

An alternative is to figure out a query that can update this table by joining with messages. That will still be slow to run, several hours at least.

Custom in-order processing that just does messages and just updates this should be faster (?). That said, 2.4 days should be ok.

hsanjuan · 2020-11-26T16:34:25Z

tasks/message/gasoutputs.go

+	child, err := node.ChainGetTipSetByHeight(ctx, abi.ChainEpoch(item.Height+1), types.NewTipSetKey())
+	if err != nil {
+		return xerrors.Errorf("Failed to load child tipset: %w", err)
+	}
+
+	st, err := state.LoadStateTree(node.Store(), child.ParentState())
+	if err != nil {
+		return xerrors.Errorf("load state tree when gas outputs for %s: %w", item.Cid, err)
+	}
+
+	dstAddr, err := address.NewFromString(item.To)
+	if err != nil {
+		return xerrors.Errorf("parse to address failed for gas outputs in %s: %w", item.Cid, err)
+	}
+
+	var dstActorCode cid.Cid
+	dstActor, err := st.GetActor(dstAddr)
+	if err != nil {
+		if !errors.Is(err, types.ErrActorNotFound) {
+			return xerrors.Errorf("get destination actor for gas outputs %s failed: %w", item.Cid, err)
+		}
+	} else {
+		dstActorCode = dstActor.Code
+	}
+


I wish there was a better way, but this should be ok if the state is cached at some layer.

#249 is much more efficient but will be a few days before it's ready. Currently checking that the data it produces matches the existing data.

yeah, still need to support it in the "standard" way which I trust will become way faster with store improvement and block caching at some point.

storage/migrations/22_gasoutputs_height_code.go

iand · 2020-11-27T12:20:35Z

Can we drop the message size column? Is it useful?

hsanjuan · 2020-11-30T17:01:55Z

Can we drop the message size column? Is it useful?

I haven't used it for anything, but it seems like it could give an interesting metric.

frrist · 2020-11-30T19:06:20Z

storage/migrations/22_gasoutputs_height_code.go

+	ALTER TABLE public.derived_gas_outputs ADD COLUMN height bigint;
+	ALTER TABLE public.derived_gas_outputs ADD COLUMN actor_name text;


I believe constraints need to be added for these new columns since neither are null-able, and height is a primary key.

Do you mean height should be made primary key along with cid?

Please have a look to the latest commits because I am not sure I know what I'm doing with the migrations..

frrist · 2020-11-30T19:16:38Z

tasks/message/gasoutputs.go

+	// Note: this item will only be processed if there are receipts for
+	// it, which means there should be a tipset at height+1.  This is only
+	// used to get the destination actor code, so we don't care about side
+	// chains.


Perhaps its worth adding a check to ensure child != item? Alternatively, null rounds can be handled via: https://github.com/filecoin-project/sentinel-visor/pull/214/files#diff-a771f3c826cfe5ebe05c1a003873af47456ee3e16188c1e5d0a1162c0b3e27acR40

Given that I just want to extract the dstActorCode, is it a problem if child == parent? The actor code will be the same even if it comes from a slightly wrong state. But maybe I'm missing something?

I believe the destination actor may no exist in child (when child == parent) if this is the first message being sent to the destination actor. But based on the below code maybe that isn't an issue -- We just won't know the designation actor code if the actor is created in a block preceding a null round.

…puts.

frrist · 2020-12-02T17:32:28Z

storage/migrations/24_gasoutputs_height_code.go

+	up := batch(`
+	ALTER TABLE public.derived_gas_outputs ADD COLUMN height bigint NOT NULL;
+	ALTER TABLE public.derived_gas_outputs ADD COLUMN actor_name text NOT NULL;
+	ALTER TABLE public.derived_gas_outputs ADD PRIMARY KEY (height);


frrist · 2020-12-02T17:37:22Z

tasks/message/gasoutputs.go

+	// Note: this item will only be processed if there are receipts for
+	// it, which means there should be a tipset at height+1.  This is only
+	// used to get the destination actor code, so we don't care about side
+	// chains.


I believe the destination actor may no exist in child (when child == parent) if this is the first message being sent to the destination actor. But based on the below code maybe that isn't an issue -- We just won't know the designation actor code if the actor is created in a block preceding a null round.

…lumns

hsanjuan · 2020-12-03T11:49:28Z

storage/migrations/25_gasoutputs_height_code.go

+	ALTER TABLE public.derived_gas_outputs ADD PRIMARY KEY (cid, height);
+`)


@iand can shall we leave like this, or add state_root too?

let's add state_root - I'll do it

hsanjuan · 2020-12-03T11:51:50Z

storage/migrations/25_gasoutputs_height_code.go

+	up := batch(`
+	ALTER TABLE public.derived_gas_outputs ADD COLUMN height bigint NOT NULL;
+	ALTER TABLE public.derived_gas_outputs ADD COLUMN actor_name text NOT NULL;
+	ALTER TABLE public.derived_gas_outputs DROP CONSTRAINT derived_gas_outputs_pkey;


I also see in other migrations we do _pk instead of _pkey, should I change that?

pkey is the convention used by postgres when you create a primary key

hsanjuan requested review from iand and frrist November 26, 2020 16:30

willscott reviewed Nov 26, 2020

View reviewed changes

model/derived/gasoutputs.go Show resolved Hide resolved

hsanjuan commented Nov 26, 2020

View reviewed changes

feat(gas outputs): Migrations: add height and actor_name columns

545c9d9

iand reviewed Nov 27, 2020

View reviewed changes

storage/migrations/22_gasoutputs_height_code.go Outdated Show resolved Hide resolved

Migration 22: set height to bigint

746d903

frrist reviewed Nov 30, 2020

View reviewed changes

hsanjuan added 4 commits December 1, 2020 12:49

feat(migrations): add constrains to new derived_gas_outputs columns

6e5079c

feat(migrations): add height column as primary key in derived_gas_out…

c9dbcdd

…puts.

Merge branch 'master' into feat/gas-outputs-columns

a361a33

feat(migrations): re-index gasoutputs_height_code migration to 24.

f3161ac

frrist approved these changes Dec 2, 2020

View reviewed changes

frrist assigned hsanjuan Dec 2, 2020

frrist added the kind/enhancement Improvement to an existing feature label Dec 2, 2020

hsanjuan added 2 commits December 3, 2020 12:30

Merge remote-tracking branch 'origin/master' into feat/gas-outputs-co…

4f8e2d3

…lumns

feat(migrations): fix gas_output_migration.

943a68b

hsanjuan commented Dec 3, 2020

View reviewed changes

hsanjuan requested a review from iand December 3, 2020 11:49

hsanjuan commented Dec 3, 2020

View reviewed changes

Add state_root to derived_gas_outputs index

f83a401

iand changed the title ~~Feat(gas outputs): Add Height and ActorName~~ feat(gas outputs): Add Height and ActorName Dec 3, 2020

iand merged commit 627e829 into master Dec 3, 2020

iand deleted the feat/gas-outputs-columns branch December 3, 2020 12:51

iand mentioned this pull request Dec 8, 2020

Add height to derived_gas_outputs #251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gas outputs): Add Height and ActorName #270

feat(gas outputs): Add Height and ActorName #270

hsanjuan commented Nov 26, 2020

hsanjuan Nov 26, 2020

hsanjuan Nov 26, 2020

iand Nov 26, 2020

hsanjuan Nov 26, 2020

hsanjuan Nov 26, 2020

iand Nov 26, 2020

hsanjuan Nov 26, 2020

iand commented Nov 27, 2020

hsanjuan commented Nov 30, 2020

frrist Nov 30, 2020

hsanjuan Dec 1, 2020

hsanjuan Dec 1, 2020

frrist Nov 30, 2020

hsanjuan Dec 1, 2020

frrist Dec 2, 2020

frrist Dec 2, 2020

frrist Dec 2, 2020

hsanjuan Dec 3, 2020

iand Dec 3, 2020

hsanjuan Dec 3, 2020

iand Dec 3, 2020

		ALTER TABLE public.derived_gas_outputs ADD COLUMN height bigint;
		ALTER TABLE public.derived_gas_outputs ADD COLUMN actor_name text;

feat(gas outputs): Add Height and ActorName #270

feat(gas outputs): Add Height and ActorName #270

Conversation

hsanjuan commented Nov 26, 2020

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

iand commented Nov 27, 2020

hsanjuan commented Nov 30, 2020

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