-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
Conversation
a1e495a
to
0218064
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.
LGTM
eth/api_backend.go
Outdated
@@ -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 |
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.
Why not return a not implemented
- error here?
internal/ethapi/backend.go
Outdated
@@ -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) |
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.
Pls rename to GetTransaction
and leave the ODR decision handling internally.
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 |
@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). |
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, 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?
It will "just work". No extra function is provided, |
eth/api_backend.go
Outdated
@@ -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) { |
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.
GetCanonicalTransaction -> GetTransaction
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
@@ -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 |
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.
Why does this dangle in the air? Why isn't this attached to a light chain or something?
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.
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.
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, seems to work nicely!
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