cerc-io/ipld-eth-server
18
0
Fork 1
Code Issues 10 Pull Requests 1 Actions Packages Projects Releases 37 Wiki Activity

Fix getLog API to use log_cids table #92

Merged
arijitAD merged 14 commits from fix-get-logs into master 2021-09-16 12:13:25 +00:00
Conversation 3 Commits 14 Files Changed 37 +1274 -1709
arijitAD commented 2021-08-16 05:41:55 +00:00 (Migrated from github.com)
Copy Link
* Remove `ipld-eth-indexer` use statediff geth instead. * Fix getLog API to use log_cids table Fixes: #87 and https://github.com/vulcanize/ipld-eth-indexer/issues/84
arijitAD commented 2021-08-31 12:46:29 +00:00 (Migrated from github.com)
Copy Link

@ashwinphatak Can you specify the CLI flags we use to run to the server? Wanted to cross-check if the CLI is not broken by my changes.

@ashwinphatak Can you specify the CLI flags we use to run to the server? Wanted to cross-check if the CLI is not broken by my changes.
ashwinphatak commented 2021-08-31 12:49:22 +00:00 (Migrated from github.com)
Copy Link

@ashwinphatak Can you specify the CLI flags we use to run to the server? Wanted to cross-check if the CLI is not broken by my changes.

I'm using:

./ipld-eth-server serve --config=./environments/config.toml --eth-server-graphql
> @ashwinphatak Can you specify the CLI flags we use to run to the server? Wanted to cross-check if the CLI is not broken by my changes. I'm using: ``` ./ipld-eth-server serve --config=./environments/config.toml --eth-server-graphql ```
👍 1
ashwinphatak (Migrated from github.com) reviewed 2021-08-31 13:33:37 +00:00
pkg/graphql/graphql.go
ashwinphatak (Migrated from github.com) commented 2021-08-31 13:33:37 +00:00
Copy Link

ipldBlock should be the log, not the receipt.

`ipldBlock` should be the log, not the receipt.
ashwinphatak (Migrated from github.com) reviewed 2021-08-31 13:34:26 +00:00
pkg/graphql/schema.go
@ -69,3 +69,3 @@
# CID for the Receipt IPLD block this Log exists in.
# CID for the leaf node IPLD block of the log.
cid: String!
ashwinphatak (Migrated from github.com) commented 2021-08-31 13:34:26 +00:00
Copy Link

Nit: Indentation is different from above.

Nit: Indentation is different from above.
i-norden approved these changes 2021-08-31 15:04:25 +00:00
i-norden left a comment
Member
Copy Link

LGTM, added some minor comments.

LGTM, added some minor comments.
db/migrations/00016_create_eth_log_cids_table.sql
@ -0,0 +4,4 @@
leaf_cid TEXT NOT NULL,
leaf_mh_key TEXT NOT NULL REFERENCES public.blocks (key) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
receipt_id INTEGER NOT NULL REFERENCES eth.receipt_cids (id) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
address VARCHAR(66) NOT NULL,
i-norden commented 2021-08-31 12:53:10 +00:00
Member
Copy Link

Just a heads up, this is not NOT NULL in the migration that is in go-ethereum

Just a heads up, this is not `NOT NULL` in the migration that is in go-ethereum
pkg/eth/api.go
@ -107,3 +109,4 @@
if pea.ethClient != nil {
if header, err := pea.ethClient.HeaderByHash(ctx, hash); header != nil && err == nil {
go pea.writeStateDiffFor(hash)
i-norden commented 2021-08-31 13:00:11 +00:00
Member
Copy Link

We can just pass RPCMarshalHeader(header, pea.B.Config.ChainConfig.Clique != nil) instead of allocating this bool (here and on line 1092 and 1113), super insignificant optimization but I actually think it makes the code cleaner/less cluttered. Up to you.

We can just pass `RPCMarshalHeader(header, pea.B.Config.ChainConfig.Clique != nil)` instead of allocating this bool (here and on line 1092 and 1113), super insignificant optimization but I actually think it makes the code cleaner/less cluttered. Up to you.
pkg/eth/ipld_fetcher.go
@ -168,7 +166,7 @@ func (f *IPLDFetcher) FetchRcts(tx *sqlx.Tx, cids []eth.ReceiptModel) ([]ipfs.Bl
}
i-norden commented 2021-08-31 15:01:40 +00:00
Member
Copy Link

This isn't fetching anything, it's decomposing the already fetched logResults.

This isn't fetching anything, it's decomposing the already fetched logResults.
i-norden commented 2021-08-31 15:03:02 +00:00
Member
Copy Link

Same here, not actually fetching IPLDs

Same here, not actually fetching IPLDs
arijitAD (Migrated from github.com) reviewed 2021-09-01 06:52:48 +00:00
db/migrations/00016_create_eth_log_cids_table.sql
@ -0,0 +4,4 @@
leaf_cid TEXT NOT NULL,
leaf_mh_key TEXT NOT NULL REFERENCES public.blocks (key) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
receipt_id INTEGER NOT NULL REFERENCES eth.receipt_cids (id) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
address VARCHAR(66) NOT NULL,
arijitAD (Migrated from github.com) commented 2021-09-01 06:52:48 +00:00
Copy Link

I have added a note in this task https://github.com/vulcanize/go-ethereum/issues/73
I will update the schema once we have a single repo for the DB schema.

I have added a note in this task https://github.com/vulcanize/go-ethereum/issues/73 I will update the schema once we have a single repo for the DB schema.
arijitAD (Migrated from github.com) reviewed 2021-09-01 12:57:09 +00:00
pkg/graphql/graphql.go
arijitAD (Migrated from github.com) commented 2021-09-01 12:57:09 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-09-01 12:57:46 +00:00
pkg/graphql/schema.go
@ -69,3 +69,3 @@
# CID for the Receipt IPLD block this Log exists in.
# CID for the leaf node IPLD block of the log.
cid: String!
arijitAD (Migrated from github.com) commented 2021-09-01 12:57:45 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-09-01 12:57:58 +00:00
pkg/eth/api.go
@ -107,3 +109,4 @@
if pea.ethClient != nil {
if header, err := pea.ethClient.HeaderByHash(ctx, hash); header != nil && err == nil {
go pea.writeStateDiffFor(hash)
arijitAD (Migrated from github.com) commented 2021-09-01 12:57:57 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-09-01 12:58:03 +00:00
pkg/eth/ipld_fetcher.go
@ -168,7 +166,7 @@ func (f *IPLDFetcher) FetchRcts(tx *sqlx.Tx, cids []eth.ReceiptModel) ([]ipfs.Bl
}
arijitAD (Migrated from github.com) commented 2021-09-01 12:58:03 +00:00
Copy Link

Updated the function name.

Updated the function name.
arijitAD (Migrated from github.com) reviewed 2021-09-01 12:58:23 +00:00
pkg/eth/ipld_fetcher.go
@ -168,7 +166,7 @@ func (f *IPLDFetcher) FetchRcts(tx *sqlx.Tx, cids []eth.ReceiptModel) ([]ipfs.Bl
}
arijitAD (Migrated from github.com) commented 2021-09-01 12:58:23 +00:00
Copy Link

Updated the function name.

Updated the function name.
ashwinphatak (Migrated from github.com) reviewed 2021-09-02 05:10:56 +00:00
pkg/graphql/schema.go
ashwinphatak (Migrated from github.com) commented 2021-09-02 05:10:06 +00:00
Copy Link

Receipt => Log in the comment?

Receipt => Log in the comment?
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
ashwinphatak (Migrated from github.com) commented 2021-09-02 05:10:44 +00:00
Copy Link

What will this return for pre-byzantium fork records?

https://ethereum.stackexchange.com/questions/28889/what-is-the-exact-meaning-of-a-transactions-new-receipt-status-field

What will this return for pre-byzantium fork records? https://ethereum.stackexchange.com/questions/28889/what-is-the-exact-meaning-of-a-transactions-new-receipt-status-field
ashwinphatak (Migrated from github.com) reviewed 2021-09-03 10:40:48 +00:00
pkg/graphql/schema.go
@ -70,3 +70,3 @@
# CID for the Receipt IPLD block this Log exists in.
# CID for the leaf node IPLD block of the log.
cid: String!
ashwinphatak (Migrated from github.com) commented 2021-09-03 10:40:48 +00:00
Copy Link

This is always returning an empty string.

This is always returning an empty string.
arijitAD (Migrated from github.com) reviewed 2021-09-08 10:12:14 +00:00
pkg/graphql/schema.go
@ -70,3 +70,3 @@
# CID for the Receipt IPLD block this Log exists in.
# CID for the leaf node IPLD block of the log.
cid: String!
arijitAD (Migrated from github.com) commented 2021-09-08 10:12:14 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-09-08 10:16:38 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
arijitAD (Migrated from github.com) commented 2021-09-08 10:16:38 +00:00
Copy Link
// Assign receipt status or post state.
if len(receipt.PostState) > 0 {
    fields["root"] = hexutil.Bytes(receipt.PostState)
} else {
    fields["status"] = hexutil.Uint(receipt.Status)
}

@ashwinphatak
As per my understanding pre-byzantium fork, we used to return PostState which is the root and if it is not present then we return the status.
For GQL we need to change the type of status to Bytes! so it can accommodate both. Or should I add two separate fields root and status

``` // Assign receipt status or post state. if len(receipt.PostState) > 0 { fields["root"] = hexutil.Bytes(receipt.PostState) } else { fields["status"] = hexutil.Uint(receipt.Status) } ``` @ashwinphatak As per my understanding pre-byzantium fork, we used to return `PostState` which is the root and if it is not present then we return the `status`. For GQL we need to change the type of status to `Bytes!` so it can accommodate both. Or should I add two separate fields `root` and `status`
ashwinphatak (Migrated from github.com) reviewed 2021-09-08 10:25:37 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
ashwinphatak (Migrated from github.com) commented 2021-09-08 10:25:36 +00:00
Copy Link

PostState which is the root

Root of what? Is there some notion of success/failure by inspecting the PostState?

> `PostState` which is the root Root of what? Is there some notion of success/failure by inspecting the `PostState`?
arijitAD (Migrated from github.com) reviewed 2021-09-08 11:01:22 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
arijitAD (Migrated from github.com) commented 2021-09-08 11:01:22 +00:00
Copy Link

Intermediate state root when the transaction is being applied if the transaction fails
Source: https://github.com/ethereum/EIPs/issues/98#issue-151094725
But then they removed the root field and add status in EIP-658 this field was replaced by status

Intermediate state root when the transaction is being applied if the transaction fails Source: https://github.com/ethereum/EIPs/issues/98#issue-151094725 But then they removed the `root` field and add `status` in [EIP-658](https://eips.ethereum.org/EIPS/eip-658) this field was replaced by `status`
ashwinphatak (Migrated from github.com) reviewed 2021-09-08 11:13:03 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
ashwinphatak (Migrated from github.com) commented 2021-09-08 11:13:02 +00:00
Copy Link

As per https://eips.ethereum.org/EIPS/eip-658, "With the introduction of the REVERT opcode in EIP140, it is no longer possible for users to assume that a transaction failed iff it consumed all gas".

So, we may be able to return 1 or 0 based on whether the tx consumed all gas for pre-byzantium blocks?

@i-norden what do you think?

As per https://eips.ethereum.org/EIPS/eip-658, "With the introduction of the REVERT opcode in EIP140, it is no longer possible for users to assume that a transaction failed iff it consumed all gas". So, we may be able to return 1 or 0 based on whether the tx consumed all gas for pre-byzantium blocks? @i-norden what do you think?
i-norden reviewed 2021-09-13 14:17:24 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
i-norden commented 2021-09-13 14:17:24 +00:00
Member
Copy Link

I don't think we can assume whether or not a tx failed based on how much of the gas limit was used.

If a tx is reverted early on in its execution, due to some programmatic failure of an OPCODE execution, then it can be reverted early on and the amount of gas used can be much lower than the limit allocated. But, if a tx reverts because there is not enough gas allocated, then it will only revert after the miner has executed all of the OPCODEs it can afford to before bumping up against the limit and realizing there is not enough left to finish execution. In this case the reverted tx may use all or most of the gas limit.

Another edge cases that make this approach infeasible is, for example, if someone vastly overestimated the gas limit needed for a tx. The tx is properly executed but done so far below the allocated limit, so using this heuristic the tx would look like it failed when it didn't.

I don't think we can assume whether or not a tx failed based on how much of the gas limit was used. If a tx is reverted early on in its execution, due to some programmatic failure of an OPCODE execution, then it can be reverted early on and the amount of gas used can be much lower than the limit allocated. But, if a tx reverts *because* there is not enough gas allocated, then it will only revert after the miner has executed all of the OPCODEs it can afford to before bumping up against the limit and realizing there is not enough left to finish execution. In this case the reverted tx may use all or most of the gas limit. Another edge cases that make this approach infeasible is, for example, if someone vastly overestimated the gas limit needed for a tx. The tx is properly executed but done so far below the allocated limit, so using this heuristic the tx would look like it failed when it didn't.
i-norden reviewed 2021-09-13 14:21:06 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
i-norden commented 2021-09-13 14:21:06 +00:00
Member
Copy Link

Oh I misread, you meant for pre-byzantium blocks. So I just stated the obvious.

Oh I misread, you meant for pre-byzantium blocks. So I just stated the obvious.
i-norden reviewed 2021-09-13 14:24:52 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
i-norden commented 2021-09-13 14:24:52 +00:00
Member
Copy Link

Theoretically it is possible that a tx uses up the entire gas limit during normal, successful execution- no? But the statement in that EIP makes it sound like it is a safe assumption.

Theoretically it is possible that a tx uses up the entire gas limit during normal, successful execution- no? But the statement in that EIP makes it sound like it is a safe assumption.
arijitAD (Migrated from github.com) reviewed 2021-09-14 12:15:33 +00:00
pkg/graphql/schema.go
@ -75,2 +79,4 @@
# Status of the Receipt IPLD block this Log exists in.
status: Int!
}
arijitAD (Migrated from github.com) commented 2021-09-14 12:15:33 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-09-14 12:15:40 +00:00
pkg/graphql/schema.go
arijitAD (Migrated from github.com) commented 2021-09-14 12:15:40 +00:00
Copy Link

Done

Done
i-norden requested changes 2021-09-14 21:23:01 +00:00
i-norden left a comment
Member
Copy Link

Left some comments, I need some clarification on some things and some changes need to be done. Also, my main hangup is that we are breaking compatibility with the regular eth json rpc endpoint with these changes, so I think that requires some further discussion.

Left some comments, I need some clarification on some things and some changes need to be done. Also, my main hangup is that we are breaking compatibility with the regular eth json rpc endpoint with these changes, so I think that requires some further discussion.
db/migrations/00006_create_eth_transaction_cids_table.sql
@ -9,6 +9,7 @@ CREATE TABLE eth.transaction_cids (
dst VARCHAR(66) NOT NULL,
i-norden commented 2021-09-14 21:15:09 +00:00
Member
Copy Link

Let's call this gas_limit, it's more accurate to how things are defined in EIPs/specs and more explicit about what it is/how it is used.

Let's call this gas_limit, it's more accurate to how things are defined in EIPs/specs and more explicit about what it is/how it is used.
pkg/eth/api.go
i-norden commented 2021-09-14 21:10:08 +00:00
Member
Copy Link

A problem with this is that this deviates from the normal, expected behavior of this eth json-rpc endpoint. The regular endpoint only fills in the status if it is explicitly known: https://github.com/ethereum/go-ethereum/blob/master/internal/ethapi/api.go#L1650. So this breaks compatibility with the regular endpoint.

A problem with this is that this deviates from the normal, expected behavior of this eth json-rpc endpoint. The regular endpoint only fills in the status if it is explicitly known: https://github.com/ethereum/go-ethereum/blob/master/internal/ethapi/api.go#L1650. So this breaks compatibility with the regular endpoint.
i-norden commented 2021-09-14 21:10:50 +00:00
Member
Copy Link

We still want to fill in the fields["root"] if receipt.PostState is present, whether or not we can also assume the status above.

We still want to fill in the `fields["root"]` if receipt.PostState is present, whether or not we can also assume the status above.
i-norden commented 2021-09-14 21:12:08 +00:00
Member
Copy Link

I'm missing something. When will receipt.GasUsed ever be more than tx.Gas()? When a tx fails for whatever reason is it's receipt.GasUsed simply set to something above the tx.Gas() to indicate it failed? Seems odd but I'm not super familiar with this pre-byzantium behavior- wouldn't it need to track/report how much gas was actually used during the failed execution/reversion (because the tx sender still has to pay for whatever execution was performed prior to it reverting/failing)?

I'm missing something. When will receipt.GasUsed ever be more than tx.Gas()? When a tx fails for whatever reason is it's receipt.GasUsed simply set to something above the tx.Gas() to indicate it failed? Seems odd but I'm not super familiar with this pre-byzantium behavior- wouldn't it need to track/report how much gas was actually used during the failed execution/reversion (because the tx sender still has to pay for whatever execution was performed prior to it reverting/failing)?
arijitAD (Migrated from github.com) reviewed 2021-09-15 11:47:43 +00:00
pkg/eth/api.go
arijitAD (Migrated from github.com) commented 2021-09-15 11:47:43 +00:00
Copy Link

Added an issue to track this https://github.com/vulcanize/ipld-eth-server/issues/102

Added an issue to track this https://github.com/vulcanize/ipld-eth-server/issues/102
ashwinphatak (Migrated from github.com) approved these changes 2021-09-15 13:39:10 +00:00
ashwinphatak (Migrated from github.com) left a comment
Copy Link

Ran watcher-ts smoke tests against this, tests passed.

Ran watcher-ts smoke tests against this, tests passed.
i-norden approved these changes 2021-09-16 11:10:42 +00:00
i-norden left a comment
Member
Copy Link

LGTM. We might want to go ahead and update the schema.sql, or not since we want to move to using ipld-eth-db for migrations/schema. Depends how soon you think we will remove the migrations/schema from here and rely on ipld-eth-dn. If soon then don't worry about it, if it will be some time (weeks) then go ahead and update the schema file here.

LGTM. We might want to go ahead and update the schema.sql, or not since we want to move to using ipld-eth-db for migrations/schema. Depends how soon you think we will remove the migrations/schema from here and rely on ipld-eth-dn. If soon then don't worry about it, if it will be some time (weeks) then go ahead and update the schema file here.
arijitAD commented 2021-09-16 12:12:53 +00:00 (Migrated from github.com)
Copy Link

LGTM. We might want to go ahead and update the schema.sql, or not since we want to move to using ipld-eth-db for migrations/schema. Depends how soon you think we will remove the migrations/schema from here and rely on ipld-eth-dn. If soon then don't worry about it, if it will be some time (weeks) then go ahead and update the schema file here.

I am picking up the task for moving the schema to ipld-eth-db next. Should be done by next week.

> LGTM. We might want to go ahead and update the schema.sql, or not since we want to move to using ipld-eth-db for migrations/schema. Depends how soon you think we will remove the migrations/schema from here and rely on ipld-eth-dn. If soon then don't worry about it, if it will be some time (weeks) then go ahead and update the schema file here. I am picking up the task for moving the schema to `ipld-eth-db` next. Should be done by next week.
Sign in to join this conversation.
Reviewers
No reviewers
i-norden
ashwinphatak
Labels
Clear labels   
bug

Something isn't working

  
critical

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
Epic

   
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
Integration tests

   
invalid

This doesn't seem right

  
question

Further information is requested

  
v5

   
wontfix

This will not be worked on

  
Copied from Github

An issue or PR manually copied from GitHub.

  
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
bug
critical
documentation
duplicate
enhancement
Epic
good first issue
help wanted
Integration tests
invalid
question
v5
wontfix
Copied from Github
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/ipld-eth-server#92
No description provided.
Delete Branch "fix-get-logs"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?