Uh oh!
There was an error while loading. Please reload this page.
Add ability to broadcast our own node_announcement - #435
Conversation
1261639 to
198b089Compare
|
c7f02e3 to
c372439Comparec372439 to
91f4e5dCompare
ariard
left a comment
There was a problem hiding this comment.
Just to fix few comments otherwise looks good.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
91f4e5d to
9c8165dCompare| let mut channel_state = self.channel_state.lock().unwrap(); | ||
| channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastNodeAnnouncement { | ||
| msg: msgs::NodeAnnouncement { | ||
| signature: self.secp_ctx.sign(&msghash, &self.our_network_key), |
There was a problem hiding this comment.
we may wanna provide an interface for external signers later
There was a problem hiding this comment.
Right. For now the external signing support hasn't even attempted to think about moving the node_id private key out, given its used in a ton of places. Eventually it'll need to be (well, probably after splitting it up more), but for now it is what it is.
| } | ||
| } | ||
| /// A "set" of addresses which enforces that there can be only up to one of each net address type. |
There was a problem hiding this comment.
how does this actually enforce that the underlying NetAddress objects behave are what's expected?
There was a problem hiding this comment.
Because the public API only allows you to add one address per type (replacing the previous one if there is one already).
There was a problem hiding this comment.
see the set_address methods now. Also, hadn't realized NetAddress wasn't a standard library.
There was a problem hiding this comment.
@arik-so FWIW, there is SocketAddrV4 and SocketAddrV6.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| }, | ||
| MessageSendEvent::BroadcastNodeAnnouncement { ref msg } => { |
There was a problem hiding this comment.
if Rust had a way of distributing match arms across multiple files, this would be the place
There was a problem hiding this comment.
I'm not sure what you mean here?
There was a problem hiding this comment.
I was referring to the fact that we're adding yet another match arm to a match that's hundreds of lines long.
| features: NodeFeatures, | ||
| last_update: u32, | ||
| last_update: Option<u32>, |
There was a problem hiding this comment.
what's the rationale for moving last_update into an Option as part of this diff? Also, there should be a comment somewhere saying it's a timestamp.
And also, this will break 18 years from now 😛
There was a problem hiding this comment.
As otherwise we'll reject a 0 value (as we only accept anything newer than the previous value). Also note that it is not a timestamp, the spec only recommends that you use a timestamp. See-also #493. I added a comment noting that None implies we've not heard any updates yet.
9c8165d to
646acb8CompareAs requested by Arik at lightningdevkit#435 (comment)
646acb8 to
cba0e24CompareAs requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
TheBlueMatt
commented
Feb 27, 2020
Addressed Jeff's feedback and added a commit which resolves #493. |
As requested by Arik at lightningdevkit#435 (comment)
2340839 to
b727bafCompareAs requested by Arik at lightningdevkit#435 (comment)
As requested by Arik at lightningdevkit#435 (comment)
valentinewallace
left a comment
There was a problem hiding this comment.
Nice, very key feature!
Uh oh!
There was an error while loading. Please reload this page.
| let encoded_msg = encode_msg!(msg); | ||
| for (ref descriptor, ref mut peer) in peers.peers.iter_mut() { | ||
| if !peer.channel_encryptor.is_ready_for_encryption() || peer.their_features.is_none() || |
There was a problem hiding this comment.
Why would a Peer not have their_features set?
There was a problem hiding this comment.
Before we've received their Init message (which will be the first message we receive, or we'll disconnect them), it should be None.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| let announcement = msgs::UnsignedNodeAnnouncement { | ||
| features: NodeFeatures::supported(), | ||
| timestamp: self.last_node_announcement_serial.fetch_add(1, Ordering::AcqRel) as u32, |
There was a problem hiding this comment.
AcqRel is a good default - it ensures we always have the latest value here, without acting as a lock (as SeqCst does). If we're not relying on any other data to be consistent, but want consistency ourselves, AcqRel it is. Note that, on x86, AcqRel compiles down to nothing, whereas SeqCst is relatively expensive.
| node_id: self.get_our_node_id(), | ||
| rgb, alias, | ||
| addresses: addresses.into_vec(), | ||
| excess_address_data: Vec::new(), |
There was a problem hiding this comment.
Since this is our node announcement, we'll never have any excess address data, right? this is just for remote peer NodeAnnouncements that may randomly have excess address data? is this a common problem...?
There was a problem hiding this comment.
That is correct. And, indeed, it is not a common thing. We have to have it as otherwise the signatures of things we relay will fail, but, in general, we anticipate almost never having anything in there, or if we do, a very small thing.
| loop { | ||
| // Just in case we end up in a race, we loop until we either successfully update | ||
| // last_node_announcement_serial or decide we don't need to. | ||
| let old_serial = self.last_node_announcement_serial.load(Ordering::Acquire); | ||
| if old_serial < header.time as usize { | ||
| if self.last_node_announcement_serial.compare_exchange(old_serial, header.time as usize, Ordering::AcqRel, Ordering::Relaxed).is_ok() { | ||
| break; | ||
| } | ||
| } else { break; } | ||
| } |
There was a problem hiding this comment.
Confused about this loop -- we don't broadcast a NodeAnnouncement in this function, so what's the point of updating last_node_announcement_serial?
There was a problem hiding this comment.
The compare_exchange updates it - the goal here is to update the value iff the block timestamp is >= the current latest value. I updated the comment to note that. Rust does have a method for this, but sadly its nightly-only (and it should compile down to something similar, just maybe with more optimal/effecient Orderings.
There was a problem hiding this comment.
Ah, so the purpose is to have a "fresh" timestamp for the next time we do broadcast a node announcement
As requested by Arik at lightningdevkit#435 (comment)
b727baf to
4334755CompareCodecov Report
@@ Coverage Diff @@## master #435 +/- ##
==========================================
+ Coverage 89.75% 90.11% +0.35%
==========================================
Files 34 34 Lines 18991 19054 +63 ==========================================
+ Hits 17046 17170 +124 + Misses 1945 1884 -61
Continue to review full report at Codecov.
|
As requested by Arik at lightningdevkit#435 (comment)
4334755 to
b8a4c45CompareUh oh!
There was an error while loading. Please reload this page.
| excess_address_data: Vec::new(), | ||
| excess_data: Vec::new(), | ||
| }; | ||
| let msghash = hash_to_message!(&Sha256dHash::hash(&announcement.encode()[..])[..]); |
There was a problem hiding this comment.
just to be sure -- spec says the signature should be over the double hash, and this seems to be a single hash?
There was a problem hiding this comment.
Right, bitcoin_hashes types are confusing. That tiny little d that easy to miss means double :).
There was a problem hiding this comment.
Ah, it is easy to miss.. 😅
| per_peer_state.insert(peer_pubkey, Mutex::new(peer_state)); | ||
| } | ||
| let last_node_announcement_serial: u32 = Readable::read(reader)?; |
There was a problem hiding this comment.
Hm, in theory could this cause a user migrating error?
There was a problem hiding this comment.
Yea, we currently do no version checking for serialized types and break things all the time. We'll need to fix this come 0.1, but for now, no reason to slow down to build compatibility with 0.0.X.
| let a_events = nodes[a].node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(a_events.len(), 1); | ||
| let a_node_announcement = match a_events[0] { | ||
| MessageSendEvent::BroadcastNodeAnnouncement { ref msg } => { | ||
| (*msg).clone() | ||
| }, | ||
| _ => panic!("Unexpected event"), | ||
| }; | ||
| nodes[b].node.broadcast_node_announcement([1, 1, 1], [1; 32], Vec::new()); | ||
| let b_events = nodes[b].node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(b_events.len(), 1); | ||
| let b_node_announcement = match b_events[0] { | ||
| MessageSendEvent::BroadcastNodeAnnouncement { ref msg } => { | ||
| (*msg).clone() | ||
| }, | ||
| _ => panic!("Unexpected event"), | ||
| }; |
Uh oh!
There was an error while loading. Please reload this page.
As requested by Arik at lightningdevkit#435 (comment)
b8a4c45 to
295bde3Compare
jkczyz
left a comment
There was a problem hiding this comment.
Looks good modulo some minor comments mostly around magic numbers. I had to dig through the BOLTs to discern their meaning. Using constants would prevent posterity from needing to do the same thing.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| #[allow(dead_code)] | ||
| const HALF_MESSAGE_IS_ADDRS: u32 = 64*1024 / (msgs::NetAddress::MAX_LEN as u32 + 1) / 2; |
There was a problem hiding this comment.
It wasn't readily apparent to me that that the + 1 was for the address type. Could you make a constant for this using ::std::mem::size_of::<u8>?
There was a problem hiding this comment.
Its explicit in the docs for MAX_LEN - there is a 1-byte type. I don't think mem::size_of makes that any clearer.
There was a problem hiding this comment.
I only suggested to use mem::size_of when defining a constant. Fine to use a literal as well.
The point of defining a constant for 1 is to make its meaning explicit. Same goes for ::std::u16::MAX.
There was a problem hiding this comment.
Right, but the 1 is explicit in the documentation for the len() method as well as the MAX_LEN docs, if we want to change the definition of that stuff we can, but any future users will see pretty clearly whats up. Using u16::MAX is fine, and I agreed it wasn't clear, so a comment was added indicating that its a message length.
Uh oh!
There was an error while loading. Please reload this page.
| const HALF_MESSAGE_IS_ADDRS: u32 = 64*1024 / (msgs::NetAddress::MAX_LEN as u32 + 1) / 2; | ||
| #[deny(const_err)] | ||
| #[allow(dead_code)] | ||
| const STATIC_ASSERT: u32 = Self::HALF_MESSAGE_IS_ADDRS - 500; // This will fail to compile if we use half of the message with 500 addresses |
There was a problem hiding this comment.
Use a constant for 500 since it is used in two places.
There was a problem hiding this comment.
Its also written in the docs, so a comment confusingly indicates you may be able to change it freely (which you are not). The new comment I added to describe this const check a bit more explicitly calls it out as our public contract, is that fine?
There was a problem hiding this comment.
I'm assuming you meant "a constant confusingly indicates you may be able to change it freely".
Isn't the point of a constant that it can't change? Saying that if a number is used in documentation we can't make it a constant seems rather silly. Just reference the constant in the documentation then. The actual value 500 seems rather unimportant in this case.
There was a problem hiding this comment.
I tend to presume that if I see a constant I can change it, and the code will change appropriately. Same goes for a user who sees a constant referenced in documentation - it may change in a future version and code should handle it. That isn't the case here, so having the number appear three times across ten lines of code where one is in documentation seems like a better approach to me.
295bde3 to
bd69f0dCompareUnlike channel_update messages, node_announcement messages have no requirement that the timestamp is greater than 0.
lnd has been blatantly ignoring this line in the spec forever, so its somewhat of a lost cause trying to enforce it.
As requested by Arik at lightningdevkit#435 (comment)
bd69f0d to
48ee31fCompareTheBlueMatt
commented
Mar 5, 2020
Go travis fuzz checks, which caught a bug that would have been introduced here where we deduped addresses we read ending up writing only a subset of what we read which fails the strictness checks we need to apply on announcement messages. |
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| #[allow(dead_code)] | ||
| const HALF_MESSAGE_IS_ADDRS: u32 = 64*1024 / (msgs::NetAddress::MAX_LEN as u32 + 1) / 2; |
There was a problem hiding this comment.
I only suggested to use mem::size_of when defining a constant. Fine to use a literal as well.
The point of defining a constant for 1 is to make its meaning explicit. Same goes for ::std::u16::MAX.
jkczyz
left a comment
There was a problem hiding this comment.
I'm not gonna hold this up with the last few comments, but I just want to state there's a good rationale for them. :) They may seem trivial, but I make sure to fully understand code that I review. And if there is something that is not obvious, I assume it may not be obvious to others reading the code. Thus, I try to make suggestions that will save other readers time.
This is a somewhat-obvious oversight in the capabilities of rust-lightning, though not a particularly interesting one until we start relying on node_features (eg for variable-length-onions and Base AMP). Sadly its not fully automated as we don't really want to store the list of available addresses from the user. However, with a simple call to ChannelManager::broadcast_node_announcement and a sensible peer_handler, the announcement is made.
As requested by Arik at lightningdevkit#435 (comment)
Fixes issue lightningdevkit#493 and should resolve some issues where other nodes (incorrectly) reject channel_update/node_announcement messages which have a serial number that is not a relatively recent timestamp.
48ee31f to
78c48f7Compare| // Messages of up to 64KB should never end up more than half full with addresses, as that would | ||
| // be absurd. We ensure this by checking that at least 500 (our stated public contract on when |
There was a problem hiding this comment.
Hm, this isn't an actual spec rule, though, is it (mod the issue that this PR addresses)? >500 does seem extreme and I don't have an issue with enforcing it, just not sure the exact purpose of enforcing it...
There was a problem hiding this comment.
IIRC (and if it doesn't it should) if we went to serialize it we'd generate something >64KB, panicing trying to send a message that overflows the max message size.
| if addresses.len() > 500 { | ||
| panic!("More than half the message size was taken up by public addresses!"); | ||
| } |
This is a somewhat-obvious oversight in the capabilities of
rust-lightning, though not a particularly interesting one until we
start relying on node_features (eg for variable-length-onions and
Base AMP).
Sadly its not fully automated as we don't really want to store the
list of available addresses from the user. However, with a simple
call to ChannelManager::broadcast_node_announcement and a sensible
peer_handler, the announcement is made.