Skip to content
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

les, light: implement ODR transaction lookup by hash #19069

Merged
merged 5 commits into from
May 13, 2019

Conversation

zsfelfoldi
Copy link
Contributor

@zsfelfoldi zsfelfoldi commented Feb 14, 2019

This PR implements an ODR request for retrieving transaction status and adds it to the API backend so that it is used by GetTransactionByHash and GetRawTransactionByHash. From the user perspective this means that eth.getTransaction and eth.getRawTransaction will now work in light mode too.
Solves #19063

les/odr_requests.go Outdated Show resolved Hide resolved
light/odr.go Outdated Show resolved Hide resolved
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -174,6 +174,10 @@ func (b *EthAPIBackend) GetPoolTransaction(hash common.Hash) *types.Transaction
return b.eth.txPool.Get(hash)
}

func (b *EthAPIBackend) GetTransactionWithOdr(ctx context.Context, txHash common.Hash) (*types.Transaction, common.Hash, uint64, uint64, error) {
return nil, common.Hash{}, 0, 0, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return a not implemented - error here?

@@ -62,6 +62,7 @@ type Backend interface {

// TxPool API
SendTx(ctx context.Context, signedTx *types.Transaction) error
GetTransactionWithOdr(ctx context.Context, txHash common.Hash) (*types.Transaction, common.Hash, uint64, uint64, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls rename to GetTransaction and leave the ODR decision handling internally.

@holiman
Copy link
Contributor

holiman commented Apr 30, 2019

PR triage summary: the user/caller doesn't care about where the transaction comes from, he just wants a transaction. So it would be better not to expose GetTransaction/GetTransactionODR , but instead just GetTransaction and the node should figure it out internally whether it exists locally or should be fetched from the network

@zsfelfoldi
Copy link
Contributor Author

@holiman @karalabe @rjl493456442 I added a commit that renames GetTransactionWithOdr to GetCanonicalTransaction which is now also implemented in eth/api_backend.go. I also made the error handling consistent between GetTransactionByHash and GetRawTransactionByHash (now both of them return an error if the retrieval from the backend has failed and we have no information on whether the given transaction is in the chain or not).

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I haven't executed the code. With this change, will existing web3.js code utilize odr-requests under the hood, or do they need to invoke a separate method? It will just work, right?

@zsfelfoldi
Copy link
Contributor Author

It will "just work". No extra function is provided, eth.getTransaction works in light mode.

@@ -174,6 +175,11 @@ func (b *EthAPIBackend) GetPoolTransaction(hash common.Hash) *types.Transaction
return b.eth.txPool.Get(hash)
}

func (b *EthAPIBackend) GetCanonicalTransaction(ctx context.Context, txHash common.Hash) (*types.Transaction, common.Hash, uint64, uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCanonicalTransaction -> GetTransaction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -227,3 +228,18 @@ func GetBloomBits(ctx context.Context, odr OdrBackend, bitIdx uint, sectionIdxLi
return result, nil
}
}

// GetTransaction retrieves a canonical transaction by hash and also returns its position in the chain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this dangle in the air? Why isn't this attached to a light chain or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is attached to the OdrBackend which is attached to the correct chain. Just like direct database read functions where the chain is selected by the supplied chain database.
It wasn't correct though because I forgot to check whether the header is available and canonical. It is added now.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, seems to work nicely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants