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

Persist feature bits across restarts #6308

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

instagibbs
Copy link
Collaborator

On channel creation with a peer, we can persist feature bits of that peer and load them on restart.

This allows more sane behavior after a restart before we've reconnected with a peer, f.e. when deciding if a peer has opted in to anysegwit when creating taproot outputs. Needed for #6035

This PR doesn't persist for each connection, only on new channel creation.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Some comments, but this looks good to me

lightningd/peer_control.c Outdated Show resolved Hide resolved
lightningd/peer_control.h Show resolved Hide resolved
wallet/db.c Outdated
@@ -949,6 +949,7 @@ static struct migration dbmigrations[] = {
{NULL, migrate_invalid_last_tx_psbts},
{SQL("ALTER TABLE channels ADD channel_type BLOB DEFAULT NULL;"), NULL},
{NULL, migrate_fill_in_channel_type},
{SQL("ALTER TABLE peers ADD feature_bits BLOB DEFAULT NULL;"), NULL},
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, indent is wrong.

@instagibbs instagibbs force-pushed the persist_featurebits branch from a5cdece to 0aecbdf Compare June 5, 2023 19:34
@instagibbs
Copy link
Collaborator Author

anecdotally seems to fix me test issues in #6035 locally, if I get a concept ACK I'll rebase to make sure on the other PR

@instagibbs instagibbs force-pushed the persist_featurebits branch from 0aecbdf to 1464a63 Compare June 5, 2023 20:41
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Pretty good: it aimed for minimal rather than maximally clean though, so a bit more rewriting is desirable I think.

db stuff looks good.

lightningd/peer_control.c Show resolved Hide resolved
@@ -1403,7 +1407,7 @@ void peer_connected(struct lightningd *ld, const u8 *msg)
peer = peer_by_id(ld, &id);
if (!peer)
peer = new_peer(ld, 0, &id, &hook_payload->addr,
hook_payload->incoming);
NULL, hook_payload->incoming);
Copy link
Contributor

Choose a reason for hiding this comment

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

... use their_features here.

And

...
else 
    peer_updated_features(peer, their_features).

Though frankly that function is only called once and is going to be clearer open-coded IMHO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did it in the open, removed peer_update_features

@instagibbs instagibbs force-pushed the persist_featurebits branch 3 times, most recently from e2a70cb to f890be6 Compare June 6, 2023 14:23
@instagibbs
Copy link
Collaborator Author

took all feedback, pushed a basic test that shows peer feature persistence. Does this deserve a changelog?

@instagibbs instagibbs force-pushed the persist_featurebits branch 3 times, most recently from 973f891 to a37a4c2 Compare June 7, 2023 15:26
@instagibbs
Copy link
Collaborator Author

Unrelated(?) CI timeout. Would like to see a proper postgres run in CI since it caught one logic issue prior.

@rustyrussell rustyrussell added this to the v23.08 milestone Jun 9, 2023
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

I'll push the trivial fixes myself to reduce RTT...

Comment on lines 81 to 77
{
tal_free(peer->their_features);
peer->their_features = tal_dup_talarr(peer, u8, their_features);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This accidentally added "tal_free(peer->their_features)" to destroy_peer. Which is harmless, but is weird, since it's a child which is freed anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops yeah was trying to figure out memory issue and forgot to remove

wallet/db.c Outdated
@@ -949,6 +949,7 @@ static struct migration dbmigrations[] = {
{NULL, migrate_invalid_last_tx_psbts},
{SQL("ALTER TABLE channels ADD channel_type BLOB DEFAULT NULL;"), NULL},
{NULL, migrate_fill_in_channel_type},
{SQL("ALTER TABLE peers ADD feature_bits BLOB DEFAULT NULL;"), NULL},
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, indent is wrong.

[ Whitespace fix and remove gratuitous tal_free(peer->their_features) -- RR ]
@rustyrussell rustyrussell force-pushed the persist_featurebits branch from a37a4c2 to fee7cbe Compare June 15, 2023 01:33
@rustyrussell
Copy link
Contributor

Ack fee7cbe

@rustyrussell rustyrussell merged commit e125640 into ElementsProject:master Jun 20, 2023
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.

3 participants