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

Remove support for push_amount #697

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Remove support for push_amount #697

merged 2 commits into from
Oct 22, 2024

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Sep 9, 2024

With the updated on-the-fly funding protocol, we're using liquidity ads instead of an unofficial push_amount TLV. We can thus remove usage of push_amounts everywhere, which simplifies the codebase.

@t-bast t-bast marked this pull request as ready for review October 8, 2024 07:43
@t-bast t-bast requested a review from pm47 October 8, 2024 07:43
With the updated on-the-fly funding protocol, we're using liquidity ads
instead of an unofficial `push_amount` TLV. We can thus remove usage of
`push_amount`s everywhere, which simplifies the codebase.
pm47
pm47 previously approved these changes Oct 22, 2024
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

LGTM

Tested against eclair

Comment on lines +73 to +74
readNumber()
readNumber()
Copy link
Member

Choose a reason for hiding this comment

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

At this state, the initial LocalCommit/RemoteCommit objects were already fully generated, and we don't need local/remote push_msat to validate signatures.

We would only need local/remote push_msat if we were to do a rbf, which we are not doing in Phoenix due to zeroconf.

So we can safely discard those values.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand what you're saying at all...if we update the app and read data that was generated by the previous version of the app, we have to skip over the local_push_amount and remote_push_amount to be able to read the rest of the state data? If I remove those two lines, the rest of the decoding will fail?

Copy link
Member

Choose a reason for hiding this comment

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

Those are just review notes confirming that it is okay to discard those two fields after reading, which is not obvious. In particular it wasn't obvious that we wouldn't need this information to validate the signatures received in that state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, I thought you were saying I could remove those two lines!

Comment on lines +103 to +104
readNumber()
readNumber()
Copy link
Member

Choose a reason for hiding this comment

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

Same remark as above, those are safe to discard.

@t-bast t-bast merged commit a7e4dad into master Oct 22, 2024
2 checks passed
@t-bast t-bast deleted the remove-push-amount branch October 22, 2024 12:38
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.

2 participants