Skip to content

les: fix TxStatus message format#15956

Closed
zsfelfoldi wants to merge 1 commit into
ethereum:masterfrom
zsfelfoldi:tx-status-fix
Closed

les: fix TxStatus message format#15956
zsfelfoldi wants to merge 1 commit into
ethereum:masterfrom
zsfelfoldi:tx-status-fix

Conversation

@zsfelfoldi
Copy link
Copy Markdown
Contributor

@zsfelfoldi zsfelfoldi commented Jan 23, 2018

Replaced by #15974

This PR fixes #15943
Problem: LES/2 clients dropped LES/2 servers after receiving a TxStatusMsg in response to SendTxsV2 because the message format of les.txStatus was broken (unable to RLP decode).
Solution: the message format has been changed back to the original format implemented in the original LES/2 PR before the cleanup. Changing the message format at this point is fine because it never worked until now anyway.

@karalabe
Copy link
Copy Markdown
Member

Why doens't the current code work? Can't we fix it instead of reintroducing the weird combo field?

@karalabe
Copy link
Copy Markdown
Member

I.e. if we change the error to string, doesn't that "just fix it"?

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

No, there is a problem with the Lookup field too. If I leave it as a pointer then encoding/decoding is inconsistent. If I change it to core.TxLookupEntry then the entire struct (including an empty hash) is always encoded every time I just want to poll a pending tx, which I'd hate a lot more than the combo field.

@karalabe
Copy link
Copy Markdown
Member

karalabe commented Jan 23, 2018

Hmm, does this not work by design, or perhaps it's an issue with our RLP encoder? @fjl?

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

I think by design. There has to be something telling the decoder whether to expect a struct there or not. One thing we could do is change it to []core.TxLookupEntry which has either 0 or 1 elements. Not sure if it is nicer than the combo field either though.

@karalabe
Copy link
Copy Markdown
Member

The eth protocol has very complex types as part of its wire protocol https://github.com/ethereum/go-ethereum/blob/master/eth/protocol.go#L171, so it shouldn't choke on a this. Perhaps we just need to add the RLP encoder to the core.TxLookupEntry.

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

Are any of those pointer fields ever nil? Because here it is, and I believe that causes the problem. I don't want to add an empty struct there.

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.

Remote LES drops connection after local client submits transaction

2 participants