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

Agressive Channeld Restart Testing #7083

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Feb 16, 2024

Repeatedly STFU and then TX_ABORT a channel to trigger the socket/fd flake issue.

Changelog-None

@ddustin ddustin requested a review from cdecker as a code owner February 16, 2024 04:03
@rustyrussell
Copy link
Contributor

This ran 50 times in a row for me without any error?

@ddustin ddustin force-pushed the ddustin/abort_test branch 2 times, most recently from d19dcb3 to acd9419 Compare February 19, 2024 20:16
@ddustin
Copy link
Collaborator Author

ddustin commented Feb 19, 2024

It passed through CI no problem too -- it must be a Mac only problem

@ddustin ddustin added this to the v24.05 milestone May 20, 2024
@cdecker
Copy link
Member

cdecker commented May 27, 2024

Shall we merge this, or would you like to dig deeper and see what is going wrong with Mac? In the latter case I'd remove the milestone.

@ddustin
Copy link
Collaborator Author

ddustin commented May 29, 2024

Seems net positive to have it merged into the CI as a regression test

@ddustin
Copy link
Collaborator Author

ddustin commented May 29, 2024

Although this does bundle in the stfu and abort RPC commands which @niftynei had some design objections to in my Splice Script PR 🤔.

#6980

@rustyrussell rustyrussell modified the milestones: v24.05, v24.08 Jun 9, 2024
@rustyrussell rustyrussell self-assigned this Jun 28, 2024
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.

Weird one. You wrote this batch API and only ever use it for one channel. Maybe just have a simpler, single-channel API? Seems clearer, too...

"Put {channel_ids} channels into stfu mode and return their available"
" balance."
};
AUTODATA(json_command, &stfu_channels_command);
Copy link
Contributor

Choose a reason for hiding this comment

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

These are definitely dev commands!!

Copy link
Contributor

Choose a reason for hiding this comment

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

And since you only ever call it with one channel, dev_quiesce would work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤔 Discussing the trade offs of exposing stfu / abort in this comment over here: #7083 (comment)

"channel_ids": channel_ids,
}
return self.call("abort_channels", payload)

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need these wrappers, esp. for dev commands. It automatically handles unknown RPCs (we mainly add them so you get nicer documentation and error detection).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very cool!

tests/test_restart.py Outdated Show resolved Hide resolved
@rustyrussell rustyrussell modified the milestones: v24.08, v24.11 Aug 9, 2024
@ddustin
Copy link
Collaborator Author

ddustin commented Nov 11, 2024

Weird one. You wrote this batch API and only ever use it for one channel. Maybe just have a simpler, single-channel API? Seems clearer, too...

The bulk commands were needed & implemented in #6980, this PR should remove its implementation and instead depend on #6980

@ddustin
Copy link
Collaborator Author

ddustin commented Nov 11, 2024

I removed the stfu_channels and abort_channels RPCs with the idea we can rebase off master once #6980 is merged.

I see two ways forward for these commands:

  1. Expose low level splice commands to user: stfu_channels, abort_channels, splice_init, splice_update, splice_signed etc
  2. Hide all these low level commands from user and expose a singular simple RPC like the dev-splice script command.

Doing something in between these two options doesn't make sense because a user doing a cross-splice of two channels needs a way to stfu both channels and potentially abort them. Prior to #6980 there was no way for a user to do so because splice_init entered stfu automatically.

@ddustin ddustin force-pushed the ddustin/abort_test branch 5 times, most recently from 17d129d to e38f259 Compare November 15, 2024 21:03
@rustyrussell
Copy link
Contributor

I removed the stfu_channels and abort_channels RPCs with the idea we can rebase off master once #6980 is merged.

I see two ways forward for these commands:

1. Expose low level splice commands to user: `stfu_channels`, `abort_channels`, `splice_init`, `splice_update`, `splice_signed` etc

I think this is the way. Though make sftu/abort singular (you can simply call it multiple times).

@rustyrussell
Copy link
Contributor

Trivial rebase...

@rustyrussell rustyrussell merged commit 4500661 into ElementsProject:master Nov 17, 2024
39 checks passed
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