Skip to content

peer: always send latest block header as part of ping messages #5621

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

Merged
merged 11 commits into from
Aug 27, 2021

Conversation

Roasbeef
Copy link
Member

In this commit, we implement a long discussed mechanism to use the
Lightning Network as a redundant block header source. By sending our
latest block header with each ping message, we give peers another source
(outside of the Bitcoin P2P network) they can use to spot check their
chain state. Peers can also use this information to detect if they've
been eclipsed from the traditional Bitcoin P2P network itself.

As is, we only send this data in Ping messages (which are periodically
sent), in the future we could also move to send them as the partial
payload for our pong messages, and also randomize the payload size
requested as well.

@Roasbeef Roasbeef added p2p Code related to the peer-to-peer behaviour networking labels Aug 11, 2021
@Roasbeef Roasbeef added this to the v0.14.0 milestone Aug 11, 2021
@Roasbeef Roasbeef requested a review from ellemouton August 11, 2021 23:15
@Crypt-iQ
Copy link
Collaborator

what do you do if you find out you are behind for significant time? call addpeer?

@Crypt-iQ
Copy link
Collaborator

bitcoind has stale block detect logic, and will connect out to Peers if stale. I'm not sure how this could be used with bitcoind -- not sure that you can feed it headers. I like extra headers relay sources but not sure how this can be used atm

@Roasbeef
Copy link
Member Author

what do you do if you find out you are behind for significant time?

Just added another commit to expose it over the call to ListPeers. Application logic can use this (or it can be somewhere else within neutrino using some sort of redundant block source interface), to do a spot check to compare the work/timestaps of the headers their peers are sending.

We can also use this new return value to write an integration test on our end tighten things up a bit.

Def some more work needed on a higher level, but this is an easy win to get the ball rolling. One other enhancement here would be to always send back our current header in the pong message as well.

Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Cool PR! Really enjoyed diving into the chainntfns package a bit!

Left some spec related questions.

Also, similar to @Crypt-iQ's question: would this purely be for application (or neutrino) logic? ie, Should LND ever react in a certain way if it sees that it's peers are getting blocks that it itself isnt?

@Roasbeef
Copy link
Member Author

would this purely be for application (or neutrino) logic? ie, Should LND ever react in a certain way if it sees that it's peers are getting blocks that it itself isnt?

rn, it's setup mainly for interpretation by a higher level application/system, the first step is just to make the data available before attempting to act on it in an automated fashion.

Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Added 2 comments for build fix and also to ensure the LastPingPayload rpc response gets populated (currently it is not populated). With those 2 changes, tACK, LGTM!

In this commit, we implement a long discussed mechanism to use the
Lightning Network as a redundant block header source. By sending our
latest block header with each ping message, we give peers another source
(outside of the Bitcoin P2P network) they can use to spot check their
chain state. Peers can also use this information to detect if they've
been eclipsed from the traditional Bitcoin P2P network itself.

As is, we only send this data in Ping messages (which are periodically
sent), in the future we could also move to send them as the partial
payload for our pong messages, and also randomize the payload size
requested as well.
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

Straightforward pull, just a question

peer/brontide.go Outdated
@@ -1368,8 +1375,14 @@ out:
atomic.StoreInt64(&p.pingTime, delay)

case *lnwire.Ping:
// TODO(roasbeef): get from buffer pool somewhere? ends
// up w/ lots of small allocations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need to include this in an earlier commit if it gets deleted?

/*
The last ping payload the peer has sent to us.
*/
bytes last_ping_payload = 15;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this get encoded as hex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

@Roasbeef Roasbeef merged commit c93824e into lightningnetwork:master Aug 27, 2021
@jnewbery
Copy link
Contributor

I'm not sure how this could be used with bitcoind -- not sure that you can feed it headers.

You can send an individual header to bitcoind over p2p using the headers message and over rpc using the submitheader method.

@Crypt-iQ
Copy link
Collaborator

I'm not sure how this could be used with bitcoind -- not sure that you can feed it headers.

You can send an individual header to bitcoind over p2p using the headers message and over rpc using the submitheader method.

Thanks - I wasn't aware of the submitheader rpc, will look into that. Given bitcoind makes various types of outbound connections periodically, checks the stale tip, etc - I would think it unlikely to get eclipsed? If the addrman is compromised though, I could see the use of feeding it headers via submitheader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking p2p Code related to the peer-to-peer behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants