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: improve txstatus retrieval #22349

Merged
merged 6 commits into from
Feb 25, 2021

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Feb 19, 2021

This PR introduces a retry mechanism in TxStatus retrieval.

In the LES protocol, there is no guarantee that the included transaction can be retrieved back seems the promise that the entire transaction history will be maintained is already gone away. The full node will drop the transaction index data older than a year by default(of course the full node operator can customize this value a bit).

What's more, the light client has no idea that the transaction with the given hash is still available from the network. So here only the weak guarantee the protocol can offer:

  • select the peers with the longest advertised transaction history
  • send the request to these peers
  • introduce the necessary retry if the peer is not aware of the request transaction

Copy link
Contributor

@zsfelfoldi zsfelfoldi left a comment

Choose a reason for hiding this comment

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

Are all changes pushed to this branch? I don't see the tx resend logic here (actually the tx sending logic is not touched at all).
Btw retrieving the tx status from multiple peers is good but maybe in the future we could do a majority check (expect more than half of the servers to give the same status, or at least a status different from "unknown"). For now it's kind of ok to believe the first server that says it has seen the tx, though maybe it's not good enough if it's the same server that we've sent the transaction to. It's possible that we send a tx and the server has problems with transaction propagation. Still, it's going to report it as pending but for everyone else it's unknown and we should resend it after some time. I'm not sure yet what is the best logic here but if we have multiple connected servers then it would be a good idea to send the tx to one server and ask the status from another.

les/odr_requests.go Show resolved Hide resolved
les/odr.go Outdated Show resolved Hide resolved
@rjl493456442 rjl493456442 changed the title les, light: resend if the transaction is not retrieved back les, light: improve txstatus retrieval Feb 24, 2021
@rjl493456442
Copy link
Member Author

rjl493456442 commented Feb 24, 2021

Are all changes pushed to this branch? I don't see the tx resend logic here (actually the tx sending logic is not touched at all).

Sorry, the PR description was confusing, it's not for the transaction resending, it's for the "tx status" retrieval.

Btw retrieving the tx status from multiple peers is good but maybe in the future we could do a majority check (expect more than half of the servers to give the same status, or at least a status different from "unknown")

Yes, we can do some majority checks. I don't check it right now is that the response itself is not verifiable, we can't ensure the status is correct even with the majority checks. And another big reason is for some transactions, we can only retrieve them back from a subset of peers, and the majority may never be reached(we have no idea what's the correct threshold since we only know the transaction hash).

So I think we can improve it a bit further with a better mechanism. In my current code, there is some randomness for selecting the peers for retrieval(offered by the distributor), so use the first seen result is kind of OK right now. It's definitely can be improved.

It's possible that we send a tx and the server has problems with transaction propagation.

I think it's more or less the problem of the transaction relayer. If we have multiple servers connected, the relayer should offer the guarantee that the local transaction is propagated well. And also, the "first seen result" is not perfect right now, we can improve it, but I think it's enough for the first version.

Copy link
Contributor

@zsfelfoldi zsfelfoldi left a comment

Choose a reason for hiding this comment

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

lgtm

@zsfelfoldi
Copy link
Contributor

I think it's more or less the problem of the transaction relayer. If we have multiple servers connected, the relayer should offer the guarantee that the local transaction is propagated well. And also, the "first seen result" is not perfect right now, we can improve it, but I think it's enough for the first version.

Sure, if the purpose of this pr is just the status query then it's definitely good enough for now.

@rjl493456442
Copy link
Member Author

Yes, because after 1.10, the transaction unindexing will be enabled by default, then all the tx status retrieval will be broken without this PR. So it's the main thing I want to fix.

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

Successfully merging this pull request may close these issues.

3 participants