-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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: les/4 minimalistic version #21909
Conversation
les/peer.go
Outdated
disableTxLookup := server.handler.blockchain.TxLookupLimit() != 0 | ||
if disableTxLookup && p.version < lpv4 { | ||
recentTx := server.handler.blockchain.TxLookupLimit() | ||
if recentTx > 0 { // 0 means no limit (all txs available) |
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.
Can we put these number in the protocol file and use the constants? It's a bit weird to hardcode the number here
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.
Sorry one more nitpicks, can we replace the 1
with a constant? It's better to define in the protocol that 1
means disabled.
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.
Just one nitpick ,otherwise LGTM. Also test it, it works.
les/peer.go
Outdated
disableTxLookup := server.handler.blockchain.TxLookupLimit() != 0 | ||
if disableTxLookup && p.version < lpv4 { | ||
recentTx := server.handler.blockchain.TxLookupLimit() | ||
if recentTx > 0 { // 0 means no limit (all txs available) |
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.
Sorry one more nitpicks, can we replace the 1
with a constant? It's better to define in the protocol that 1
means disabled.
This PR allows tx unindexing in les/4 light server mode.
A few more upgrades might be added to les/4 but the goal is to keep the number of changes low while satisfying the requirements stated in #21876
Most of the upgrades needed for incentivization (including UDP communication) should go into les/5.
Features that might be added to the les/4 upgrade (up for debate):
Note: the metainfo channel upgrade is needed for incentivization and therefore could also go into les/5. The reason I added it here is because it can be added separately from everything else and it would make the next upgrade easier (less changes at once). I think with the rest of the changes postponed this upgrade would still be relatively low risk if we add the metainfo and it would help my progress but if there is a good reason then we can leave it out.