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

sweep/txgenerator: fix input witness ordering #4838

Merged

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Dec 7, 2020

This commit fixes an issue that would arise if inputs without required TxOuts would be swept together with inputs with required TxOuts. In this case we would add the required ones first to the transaction, but did not change the order we signed the inputs, resulting in signing the wrong input index.

A unit test that triggers the scenario is added.

Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch!

This commit fixes an issue that would arise if inputs without required
TxOuts would be swept together with inputs with required TxOuts. In this
case we would add the required ones first to the transaction, but did
not change the order we signed the inputs, resulting in signing the
wrong input index.
This add a test for inputs that gets re-ordered because the inputs with
required TxOuts must be added first.

We add a new step to the test that checks that all inputs were signed at
the correct tx input index.

This test would fail without the previous commit.
@halseth halseth force-pushed the sweeper-input-script-ordering branch from 257bbfb to 74eb728 Compare December 8, 2020 10:00
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🎷

@Roasbeef Roasbeef added this to the 0.12.0 milestone Dec 9, 2020
@Roasbeef Roasbeef merged commit c95c423 into lightningnetwork:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants