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

input+sweep: make sure input with no fee rate is not added to cluster #7824

Merged
merged 7 commits into from Oct 17, 2023

Conversation

yyforyongyu
Copy link
Collaborator

@yyforyongyu yyforyongyu commented Jul 12, 2023

During a debugging process, an edge case is found when creating clusters for locked inputs. This PR makes sure an input is only added to the cluster when it has successfully estimated its fee rate. Previously, when an error is returned from feeRateForPreference, we'd still add this input to the cluster, resulting in a lower fee rate being used because when averaging the fee rates, we'd think this input has zero fee rate specified.

Several unit tests are patched to make the method clusterByLockTime more robust.


This change is Reviewable

@yyforyongyu yyforyongyu added fees Related to the fees paid for transactions (both LN and funding/commitment transactions) utxo sweeping labels Jul 12, 2023
@yyforyongyu yyforyongyu self-assigned this Jul 12, 2023
@yyforyongyu yyforyongyu force-pushed the sweeper-unit-test branch 2 times, most recently from bac3a9b to 85b030a Compare July 12, 2023 14:32
@yyforyongyu
Copy link
Collaborator Author

cc @ziggie1984

Copy link
Collaborator

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

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

Really cool new tests, learned how to use the mocks package 🤓. Had only minor nits.
:lgtm:

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yyforyongyu)


input/mocks.go line 9 at r1 (raw file):

)

// MockInput implement the `Input` interface and is used by other packages for

Nit: implements


sweep/sweeper.go line 250 at r1 (raw file):

// feeDeterminor defines an alias to the function signature of
// `DetermineFeePerKw`.
type feeDeterminor func(chainfee.Estimator,

Nit: s/Determinor/Determiner/g


sweep/sweeper.go line 970 at r1 (raw file):

		// Get the fee rate based on the fee preference. If an error is
		// returned, we'll skip sweeping this input for this round of

Nice catch 👏


sweep/sweeper_test.go line 2323 at r1 (raw file):

	}

	// Set the max fee rate to be 1000.

Nit: 1000 sat/kw


sweep/sweeper_test.go line 2335 at r1 (raw file):

	// testFeeRate is used to test the success case.
	testFeeRate := (s.cfg.MaxFeeRate + s.relayFeeRate) / 2

Nit: s/testFeeRate/validFeeRate ?


sweep/sweeper_test.go line 2405 at r1 (raw file):

			// Assert the expected feerate.
			require.Equal(t, tc.expectedFeeRate, feerate)

first check for error and maybe skip the fee check if an error is expected ?


sweep/sweeper_test.go line 2414 at r1 (raw file):

// TestClusterByLockTime tests the method clusterByLockTime works as expected.
func TestClusterByLockTime(t *testing.T) {

I like this test 👏


sweep/sweeper_test.go line 2488 at r1 (raw file):

	// Create a test sweeper.
	s := New(&UtxoSweeperConfig{
		// Set the max fee rate to be 1000.

Nit: 1000 sat/kw

@saubyk saubyk added the P1 MUST be fixed or reviewed label Aug 17, 2023
Copy link
Collaborator Author

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ziggie1984)


sweep/sweeper.go line 970 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

Nice catch 👏

🤓


sweep/sweeper_test.go line 2405 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

first check for error and maybe skip the fee check if an error is expected ?

I think it's more robust to check all the returned values.


sweep/sweeper_test.go line 2414 at r1 (raw file):

Previously, ziggie1984 (ziggieXXX) wrote…

I like this test 👏

Done.😈

@lightninglabs-deploy
Copy link

@Roasbeef: review reminder

This commit makes `DetermineFeePerKw` configurable on sweeper so it's
easier to write unit tests for it.
@yyforyongyu yyforyongyu force-pushed the sweeper-unit-test branch 2 times, most recently from 8ffcfd8 to 8817416 Compare October 13, 2023 07:14
This commit removes the map `inputFeeRates` inside `clusterByLockTime`
as the fee rate can already be access via `input.lastFeeRate`.
This commit makes sure an input is only added to the cluster when it has
successfully estimated its fee rate. Previously, when an error is
returned from `feeRateForPreference`, we'd still add this input to the
cluster, resulting a **lower** fee rates being used because when
averaging the fee rates, we'd think this input has zero fee rate
specified.

An unit test is patched to make the method `clusterByLockTime` more
robust.
@saubyk
Copy link
Collaborator

saubyk commented Oct 17, 2023

Concept Ack.
cc: @guggero this can be merged

@guggero guggero merged commit e8d865a into lightningnetwork:master Oct 17, 2023
21 of 25 checks passed
@yyforyongyu yyforyongyu deleted the sweeper-unit-test branch October 17, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fees Related to the fees paid for transactions (both LN and funding/commitment transactions) P1 MUST be fixed or reviewed utxo sweeping
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants