les: fix SendTx cost calculation and verify cost table #19437
Conversation
There was a problem hiding this comment.
In the handshake, could we also check the costtable length? In theory les/1 client should only receive les/1 costtable and les/2 client should only receive les/2 costtable. Now we have checked the existence of each cost. It would be better to also check the table length.
| peer.fcServer.QueueRequest(reqID, cost) | ||
| return func() { peer.SendTxs(reqID, cost, ll) } | ||
| return func() { peer.SendTxs(reqID, cost, enc) } |
There was a problem hiding this comment.
After modifying the TestTransactionStatusLes2 to this style, the RLP decode is failed.
|
I don't think we should strictly check the length of the cost table. We might want to add new requests later and in many cases it is doable without a new protocol version. Maybe we could add some protection against sending excessively large handshake packets in general but I don't think we should add it to 1.8. |
|
Even worse, currently the server does announce the cost of requests for all protocol versions. Now that you brought this up, this might not be ideal but expecting an exact size for the cost table would definitely break now. |
|
@zsfelfoldi You are right. We have to also consider the compatibility between the 1.8.26 light clients and old server. |
| case lpv2: | ||
| msgcode = SendTxV2Msg | ||
| default: | ||
| panic(nil) |
There was a problem hiding this comment.
Please add a message. panic(nil) is really bad style.
There was a problem hiding this comment.
these were all removed when LES/1 was removed so I really don't think we need to care about it in this hotfix
| case lpv2: | ||
| checkList = reqListV2 | ||
| default: | ||
| panic(nil) |
This PR backports the new transaction cost calculation (including the size cost correction for large transactions which is now used by latest servers too). It also adds a verification for required message types in the announced cost table.