Skip to content

Conversation

@sputn1ck
Copy link
Member

@sputn1ck sputn1ck commented Nov 4, 2024

This PR changes the loopin sweepless sweep
handling, by changing the hardcoded fee messages to
a map of fee versions.

@hieblmi hieblmi force-pushed the static-addr-staging branch 3 times, most recently from 60ff75b to ec3070f Compare November 5, 2024 18:22
@sputn1ck sputn1ck requested review from hieblmi and starius November 6, 2024 14:21
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

Thanks for the support @sputn1ck 🎖️
Code lgtm, I've done some reordering an naming changes.

@hieblmi hieblmi force-pushed the more_fee_versions branch 3 times, most recently from 8143266 to 356b673 Compare November 7, 2024 09:53
This commit changes the loopin sweepless sweep
handling, by changing the hardcoded fee messages to
a map of fee versions.
@sputn1ck sputn1ck requested a review from bhandras November 7, 2024 10:51
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, just two nits 🎉

// sigs.
ServerSweeplessSigningInfo extreme_fee_info = 4;
// A map of fee rate to the nonces.
map<uint64, ServerSweeplessSigningInfo> fee_rate_to_nonces = 2;
Copy link
Member

Choose a reason for hiding this comment

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

nit: i'd call this server_signing_info (and the one in the PushStaticAddressSweeplessSigsRequest client_signing_info.

// The info the client used to generate the extreme fee partial sweepless
// tx sigs.
ClientSweeplessSigningInfo extreme_fee_signing_info = 4;
// A map of fee rate to the nonces.
Copy link
Member

Choose a reason for hiding this comment

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

nit: comment could be updated as it's not just the nonces but also the sigs.

Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

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

LGTM! Great improvement!
I added a couple of comments.

// The info the server used to generate the extreme fee partial sweepless tx
// sigs.
ServerSweeplessSigningInfo extreme_fee_info = 4;
// A map of fee rate to the nonces.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to add units of fee rate to godoc and/or to the name of the field. Also in PushStaticAddressSweeplessSigsRequest client_signing_info.

err = fmt.Errorf("unable to sign sweepless sweep tx: %w", err)
return f.HandleError(err)
}
sessions, nonces, err = f.loopIn.createMusig2Sessions(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, resources represented by sessions leak in the signer. We should close them. It is not related to this PR, therefore let's add TODO here to clean sessions in the future.

if err != nil {
return f.HandleError(err)
}
clientResponse := make(map[uint64]*looprpc.ClientSweeplessSigningInfo)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to preallocate the space in the map:

clientResponse := make(
  map[uint64]*looprpc.ClientSweeplessSigningInfo,
  len(fetchResp.FeeRateToNonces),
)

@sputn1ck
Copy link
Member Author

closing, as superceeded by #844

@sputn1ck sputn1ck closed this Nov 14, 2024
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.

4 participants