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

client/asset/eth: RegFeeConfirmations #1317

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

buck54321
Copy link
Member

Implement RegFeeConfirmations. Add transactionConfirmations method
to nodeClient. Check local tx pool first, since non-zero confs will
always request from peers, and the tx we're looking for is ours.

Implement RegFeeConfirmations. Add transactionConfirmations method
to nodeClient. Check local tx pool first, since non-zero confs will
always request from peers.
Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

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

LGTM

if err != nil {
return 0, err
}
tx, _, blockHeight, _, err := n.leth.ApiBackend.GetTransaction(ctx, txHash)
Copy link
Member

@chappjc chappjc Nov 30, 2021

Choose a reason for hiding this comment

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

OK, but it's curious that internally they do this before GetPoolTransaction: https://github.com/ethereum/go-ethereum/blob/40cdcf8c47ff094775aca08fd5d94051f9cf1dbb/internal/ethapi/api.go#L1158-L1168
Could it be possible that there would be a result from GetPoolTransaction when GetTransaction would find it?

Copy link
Member

Choose a reason for hiding this comment

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

It makes more chronological sense to check the pool first, otherwise you might miss it checking "finalized" transactions first and it gets "finalized" before you check the pool where it was just removed.

Comment on lines +506 to +507
// We'll check the local tx pool first, since from what I can tell, a light
// client always requests tx data from the network for anything else.
Copy link
Member

@chappjc chappjc Nov 30, 2021

Choose a reason for hiding this comment

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

Curious what you discovered regarding network requests.

I started digging and it looks like it at least has a local db that it checks before sending off a request, like a cache.
From les/api_backend.go, (*LesApiBackend).GetTransaction > light.GetTransaction, then in light/odr_util.go:
GetTransaction > GetBody > GetBodyRLP > rawdb.ReadBodyRLP otherwise odr.Retrieve for the data, which stores the result for future read.

@chappjc chappjc added the ETH label Nov 30, 2021
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Ok.

I still find it curious that (*PublicTransactionPoolAPI) GetTransactionByHash does the checks in the opposite order.

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

Successfully merging this pull request may close these issues.

4 participants