-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix getLog
API to use log_cids
table
#92
Conversation
@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:
|
pkg/graphql/graphql.go
Outdated
log: l.Log, | ||
cid: l.CID, | ||
receiptCID: l.RctCID, | ||
ipldBlock: l.RctData, |
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.
ipldBlock
should be the log, not the receipt.
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.
Done
pkg/graphql/schema.go
Outdated
@@ -72,6 +72,9 @@ const schema string = ` | |||
|
|||
# IPLD block data for the Receipt this Log exists in. | |||
ipldBlock: Bytes! | |||
|
|||
# Status of the Receipt IPLD block this Log exists in. |
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.
Nit: Indentation is different from above.
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.
Done
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.
LGTM, added some minor comments.
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, |
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.
Just a heads up, this is not NOT NULL
in the migration that is in go-ethereum
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 added a note in this task cerc-io/go-ethereum#73
I will update the schema once we have a single repo for the DB schema.
pkg/eth/api.go
Outdated
extractMiner = true | ||
} | ||
|
||
fields := RPCMarshalHeader(header, extractMiner) |
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.
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.
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.
Done
pkg/eth/ipld_fetcher.go
Outdated
@@ -167,8 +167,95 @@ func (f *IPLDFetcher) FetchRcts(tx *sqlx.Tx, cids []eth.ReceiptModel) ([]ipfs.Bl | |||
return rctIPLDs, nil | |||
} | |||
|
|||
// FetchLogs fetches logs. | |||
func (f *IPLDFetcher) FetchLogs(logCIDs []logResult) ([]*types.Log, error) { |
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.
This isn't fetching anything, it's decomposing the already fetched logResults.
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.
Updated the function name.
pkg/eth/ipld_fetcher.go
Outdated
} | ||
|
||
// FetchGQLLogs fetches logs for graphql. | ||
func (f *IPLDFetcher) FetchGQLLogs(logCIDs []logResult) ([]logsCID, error) { |
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.
Same here, not actually fetching IPLDs
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.
Updated the function name.
pkg/graphql/schema.go
Outdated
@@ -67,11 +67,17 @@ const schema string = ` | |||
# Transaction is the transaction that generated this log entry. | |||
transaction: Transaction | |||
|
|||
# CID for the Receipt IPLD block this Log exists in. | |||
# CID for the Receipt IPLD block of leaf node. |
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.
Receipt => Log in the comment?
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.
Done
ipldBlock: Bytes! | ||
|
||
# Status of the Receipt IPLD block this Log exists in. | ||
status: Int! |
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 will this return for pre-byzantium fork records?
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.
// 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
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.
PostState
which is the root
Root of what? Is there some notion of success/failure by inspecting the PostState
?
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.
Intermediate state root when the transaction is being applied if the transaction fails
Source: ethereum/EIPs#98 (comment)
But then they removed the root
field and add status
in EIP-658 this field was replaced by status
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.
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?
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 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.
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.
Oh I misread, you meant for pre-byzantium blocks. So I just stated the obvious.
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.
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.
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.
Done
@@ -67,11 +67,17 @@ const schema string = ` | |||
# Transaction is the transaction that generated this log entry. | |||
transaction: Transaction | |||
|
|||
# CID for the Receipt IPLD block this Log exists in. | |||
# CID for the leaf node IPLD block of the log. | |||
cid: String! |
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.
This is always returning an empty string.
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.
Done
0491cab
to
70ccc51
Compare
70ccc51
to
65411e7
Compare
65411e7
to
458aae1
Compare
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.
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.
pkg/eth/api.go
Outdated
@@ -524,12 +524,18 @@ func (pea *PublicEthAPI) localGetTransactionReceipt(ctx context.Context, hash co | |||
"logsBloom": receipt.Bloom, | |||
} | |||
|
|||
// Assign receipt status or post state. | |||
if len(receipt.PostState) > 0 { | |||
if blockNumber <= pea.B.Config.ChainConfig.ByzantiumBlock.Uint64() { |
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.
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.
pkg/eth/api.go
Outdated
} else { | ||
fields["status"] = hexutil.Uint(types.ReceiptStatusSuccessful) | ||
} | ||
} else if len(receipt.PostState) > 0 { |
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.
We still want to fill in the fields["root"]
if receipt.PostState is present, whether or not we can also assume the status above.
pkg/eth/api.go
Outdated
// Assign receipt status or post state. | ||
if len(receipt.PostState) > 0 { | ||
if blockNumber <= pea.B.Config.ChainConfig.ByzantiumBlock.Uint64() { | ||
if receipt.GasUsed > tx.Gas() { |
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'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)?
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.
Added an issue to track this #102
@@ -10,6 +10,7 @@ CREATE TABLE eth.transaction_cids ( | |||
src VARCHAR(66) NOT NULL, | |||
tx_data BYTEA, | |||
tx_type BYTEA, | |||
gas INTEGER NOT NULL, |
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.
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.
This reverts commit 04a0f9a.
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.
Ran watcher-ts smoke tests against this, tests passed.
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.
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-indexer
use statediff geth instead.Fixes: Update
getLogs
to work with new log_cids table #87 and Lift code used from here into statediffing geth and ipld-eth-server vulcanize/ipld-eth-indexer#84