-
Notifications
You must be signed in to change notification settings - Fork 374
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
Make htlc_maximum_msat
a required field.
#1519
Make htlc_maximum_msat
a required field.
#1519
Conversation
Given we can use this to clean up the scoring as well (we'd no longer need a "default" channel capacity), I think we should do this sooner rather than later. Will do a real review this week. |
3fd4fdc
to
b24f081
Compare
AFAICT, benchmark CI check is failing due to said compat breakage. |
Right, so I think compat is actually fine here, the issue is we have some channels in our kinda-checked-in gossip store that are missing the field. So we should update the |
I think, in this PR or a followup, we can drop |
Grr, after playing around with it a bit more, it seems just skipping on decoding errors isn't a very good option here, since this messes with the alignment in the reader. Also, I think that we would discard a lot of updates received via gossip and/or during deserialization when we just switch to the new layout. Maybe the best way forward would be to introduce a |
Oh? I'm not sure why this is an issue? The |
Ah, I believe this could work in order to solve the alignment issue: we could ensure that we don't error-out mid decoding, i.e., the required number of bytes is always read, even if the Also, are we positive we simply want to drop legacy channel updates when there is no |
Yep, exactly, read the whole thing either way, but set
Yes, but
I kinda assume so? Like, the theory of making it required is that its ~fully deployed everywhere and that we don't need to accept gossip where its not set. If that's true, we should be happy to not use gossip when its not set :) |
b24f081
to
3e85de4
Compare
Rebased and just pushed an intermediary step towards implementing I played around with utilizing pre-existing macros, but most of them are full of FWIW, I'm considering implementing |
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.
Yea, it does become a good bit more verbose...not a lot we can do about it, at least not without adding more support to the macros.
Alright, thanks for the feedback, will proceed. Another conundrum to be solved is that even when we make sure we do not stop decoding, we will do so according to the new layout, i.e., will probably still hit an alignment issue. Will try to read and discard the appropriate number of bytes after we fail decoding the Btw. wouldn't such a change of the serialization format warrant to increase |
It shouldn't be an issue as long as you call through the tlv stream read macro. It should read all available TLVs.
No, I don't think so, I think that's basically just when we want to break TLV compat, but this should still be fully backwards and forwards compatible. |
Ah, right, one more reason why it could pay off to go the macro route. 👍
Thanks, makes sense. |
I believe as written currently it works just fine and reads all available bytes. |
Looks like this needs rebase as well now. Feel free to squash fixups when you do so. |
As of #1553 we actually now must ensure we ignore announcements that fail to deserialize as we've "soft-forked" serialization and if a node has an existing, but invalid, hostname field we'll refuse to read our entire network graph. Thus, we'll want to do that here and will need to land this in the next release. |
Yes, sorry for the delay here. I made some progress and hope to push some updates by the end of this week, early next week latest. |
3e85de4
to
e3fe348
Compare
Codecov Report
@@ Coverage Diff @@
## main #1519 +/- ##
==========================================
+ Coverage 90.82% 91.13% +0.30%
==========================================
Files 80 80
Lines 44643 46470 +1827
Branches 44643 46470 +1827
==========================================
+ Hits 40547 42350 +1803
- Misses 4096 4120 +24
Continue to review full report at Codecov.
|
Rebased and just pushed some progress. |
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.
Not your bug, but due to #1553, whatever we do for ChannelUpdate we have to also do for NodeAnnouncement.
@@ -715,16 +797,55 @@ impl fmt::Display for ChannelInfo { | |||
} | |||
} | |||
|
|||
impl_writeable_tlv_based!(ChannelInfo, { |
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 not just make ignorable
work for impl_writeable_tlv_based
?
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.
Hum, it may have been possible before, but I think now that we handle reading ChannelUpdateInfo
s via ChannelUpdateInfoDeserWrap
, we're needing a custom implementation of Readable
for ChannelInfo
anyways?
@TheBlueMatt If you don't mind, I'll do this in a follow-up PR. |
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.
This basically LGTM, can you clean up the git history and I'll give it a proper review?
6d6ffd4
to
3d37831
Compare
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.
ACK 7b637a3
This LGTM. Only did a light review of gossip.rs
though.
Oops needs rebase now as well. |
7b637a3
to
24315e4
Compare
Rebased on main. |
c83955c
to
a979694
Compare
Yes, I have a fix ready that overrides with the default if |
Ah, right. Yea, let's just move forward with this and merge it with the benchmark CI failing, IMO. We can fix it pre-release (or not, even, really). |
In any case, assigning @arik-so as a "please generate a new snapshot" |
Did my comment regarding the discussion with Rusty not send? I'm not seeing it in this thread, when I could have sworn I sent it. Either way, I sent @tnull a new snapshot for testing this morning. |
lightning/src/routing/gossip.rs
Outdated
// Check we can decode legacy ChannelInfo, even if the `two_to_one`/`one_to_two` fields | ||
// fail to decode. |
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.
Let's add a similar test for NodeInfo
missing announcement_info
?
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.
Ah, yes, good catch, was coming back to that. I now added the test 04b677c, but it's indeed currently still failing, as not all bytes of the invalid NodeAnnouncementInfo
are directly read when it fails, they are only cleaned up by s.eat_remaining()
, which however still leads to the read_tlv_fields!
macro returning an InvalidValue
I can't catch (see here).
Currently not entirely sure how I'll about it. As it's in fact the NetAddress
that is failing to decode, I probably could implement MaybeReadable
for Vec<NetAddress>
which only adds the successful decodes to the resulting vector.
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.
I now worked around this by introducing another NetAddressVecDeserWrapper
in 285cb27, but this is really not pretty. It might be easier to switch NetAddress
directly to MaybeReadable
, since it is the part that may fail to decode. However, I think this would mess with serialization in other parts of the code, e.g., the Init
message...
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.
After some offline discussion with @TheBlueMatt I now went to ignore all errors arising from trying to decode NodeAnnouncementInfo
in c1bfa3f, which eliminates the need for an additional NetAddressVecDeserWrapper
.
Can confirm that benchmark passes with the new snapshot at least locally. Can we upload it, I'd then update the URL in benchmark CI and here before we merge this? |
Oh, oops, right, can you also add a test (and fix, I think its broken) reading a |
As discussed offline, we now eat all errors arising from trying to read |
lightning/src/routing/gossip.rs
Outdated
|
||
impl MaybeReadable for NodeAnnouncementInfoDeserWrapper { | ||
fn read<R: io::Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> { | ||
loop { |
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.
Oh lol, uhhh, hmm, I don't think this quite works, but could. Calling read on a "normal' object may try to read several bytes at a time, which technically the Read
implementation may refuse to do if it doesn't have enough bytes. Instead, if we fail to read once we should fall back to just calling reader.read()
on an, eg, 4KB buffer repeatedly until it returns zero.
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.
Ah, I had thought the loop would always make at least a u8
worth of progress since it would try to read the tlv_len: BigSize
fields on every iteration. That said, I now adopted the copy
approach from eat_remaining
, which seems like the cleaner way to do it anyways.
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.
Patch otherwise LGTM.
c58be01
to
896f47b
Compare
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.
Looks good, feel free to squash.
Fixes a deserialization incompatibility introduced with lightningdevkit#1553.
896f47b
to
8b86ed7
Compare
Squashed! |
As of lightning/bolts#996,
htlc_maximum_msat
will soon be a required field.This PR implements this change, i.e., it removes the special de-/serialisation logic needed before.
Not sure if we want to do this right away though, since it breaks serialisation backwards compat. 🤷♂️