Skip to content
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

More BOLT updates, to latest. #5592

Merged
merged 13 commits into from
Sep 24, 2022

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Sep 14, 2022

Includes the 12-block delay, removing non-htlc_maximum_msat updates, and setting the dont_forward bit!

Currently breaks lnprototest as expected, I'll have to fix that!

Closes: #5548
Closes #5562

gossipd/routing.h Outdated Show resolved Hide resolved
- Always check the major version number! We will increment it if the format
changes in a way that breaks readers.
- Ignore unknown flags in the header.
- Ignore unknown messages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was reading this as "Ignore any message of unknown type." Is there any other validation a reader should be expected to perform? If so, we should be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's it, so I made it more explicit.

Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

This was great - I learned a few things. I like the gossip_store versioning and new documentation.
ACK 6e4e104

@endothermicdev
Copy link
Collaborator

I should note this predictably upsets lnprototest during test_bolt7-01-channel_announcement-success.py. Will need to bring the test up to speed with a 12 block delay after closing tx.

@cdecker
Copy link
Member

cdecker commented Sep 20, 2022

Some things that made CI upset are fixed on master so a rebase will address at least the missing mako issue.

@cdecker
Copy link
Member

cdecker commented Sep 21, 2022

Seems there is a mismatch between bolts and string in lnprototest:

        test += [
            Block(blockheight=103, number=3, txs=[funding_tx()]),
>           ExpectMsg(
                "channel_ready",
                channel_id=channel_id_v2(local_keyset),
                second_per_commitment_point=remote_per_commitment_point(1),
            ),
            # Ignore unknown odd messages
            TryAll([], RawMsg(bytes.fromhex("270F"))),
        ]

tests/test_bolt2-20-open_channel_accepter.py:1951: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = ExpectMsg:test_bolt2-20-open_channel_accepter.py:1951
msgtypename = 'channel_ready'
if_match = <function ExpectMsg._default_if_match at 0x7f5e32b7b9a0>
ignore = None, connprivkey = None
kwargs = {'channel_id': <function channel_id_v2.<locals>._channel_id_v2 at 0x7f5e302e5510>, 'second_per_commitment_point': functools.partial(<function remote_per_commitment_point.<locals>._remote_per_commitment_point at 0x7f5e302e55a0>, 1)}

    def __init__(
        self,
        msgtypename: str,
        if_match: Callable[["ExpectMsg", Message, "Runner"], None] = _default_if_match,
        ignore: Optional[Callable[[Message], Optional[List[Message]]]] = None,
        connprivkey: Optional[str] = None,
        **kwargs: Union[str, Resolvable],
    ):
        super().__init__(connprivkey)
        self.msgtype = namespace().get_msgtype(msgtypename)
        if not self.msgtype:
>           raise SpecFileError(self, "Unknown msgtype {}".format(msgtypename))
E           lnprototest.errors.SpecFileError: (ExpectMsg:test_bolt2-20-open_channel_accepter.py:1951, 'Unknown msgtype channel_ready')

@rustyrussell
Copy link
Contributor Author

OK, so this now depends on new lnprototest, which requires #5621...

@rustyrussell
Copy link
Contributor Author

OK, I folded the #5621 into this (at a point before we add the 12 block delay), so this PR will actually pass.

@rustyrussell
Copy link
Contributor Author

Trivial rebase for minor test conflict.

@cdecker
Copy link
Member

cdecker commented Sep 23, 2022

Seems some of the blockheights are off by 12 blocks, so this seems to be either the wrong commit or we need to adjust heights in tests.

I'll be ignoring failures in the lnprototest config in CI until this PR is merged.

@rustyrussell
Copy link
Contributor Author

Seems some of the blockheights are off by 12 blocks, so this seems to be either the wrong commit or we need to adjust heights in tests.

I'll be ignoring failures in the lnprototest config in CI until this PR is merged.

Wtf... Ok, I'll try to get to my laptop today and figure out what I did.

I meant to only add the lnprototest changes :(

Which I disagreed with, and has been fixed.

Signed-off-by: Rusty Russell <[email protected]>
If they really upgrade directly from 0.9.2, it will simply delete the
store and re-fetch it.

We still update from v9 (which could be v0.11), since it's a noop.

Signed-off-by: Rusty Russell <[email protected]>
We rewrite the file to compact it, but as a side effect we
recalculate the checksums!

Signed-off-by: Rusty Russell <[email protected]>
Many changes to gossmap (including the pending ones!) don't actually
concern readers, as long as they obey certain rules:

1. Ignore unknown messages.
2. Treat all 16 upper bits of length as flags, ignore unknown ones.

So now we split the version byte into MAJOR and MINOR, and you can
ignore MINOR changes.

We don't expose the internal version (for creating the map)
programmatically: you should really hardcode what major version you
understand!

Signed-off-by: Rusty Russell <[email protected]>
And in the next patch, gossipd will no longer put new ones in.

Signed-off-by: Rusty Russell <[email protected]>
We will now simply reject old-style ones as invalid.  Turns out the
only trace we could find is a channel between two nodes unconnected to
the rest of the network.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: Protocol: We now require all channel_update messages include htlc_maximum_msat (as per latest BOLTs)
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Protocol: We now set the `dont_forward` bit on private channel_update's message_flags (as per latest BOLTs).
rustyrussell and others added 5 commits September 24, 2022 08:36
It's a bit more optimal, and tells gossipd exactly what height the
spend occurred at (with multiple blocks, it's not always the current
height!).  It will need that next patch.

Signed-off-by: Rusty Russell <[email protected]>
This adds a new "chan_dying" message to the gossip_store, but since we
already changed the minor version in this PR, we don't bump it again.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Protocol: We now delay forgetting funding-spent channels for 12 blocks (as per latest BOLTs, to support splicing in future).
Simple quote update.

Signed-off-by: Rusty Russell <[email protected]>
Mainly for @vincenzopalazzo who is thinking of writing a Rust version!

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author

Ah, it really was a conflict! That multichan test did need fixing, done now.

@rustyrussell rustyrussell merged commit 3f5ff0c into ElementsProject:master Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to latest bolt which delays channel close 12 blocks after spend seen
4 participants