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: prepare for sweeper #1978

Merged
merged 6 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@joostjager
Collaborator

joostjager commented Sep 26, 2018

This PR makes changes that prepare for the new Sweeper struct.

  • Create new subsystem for sweeping
  • Move sweep tx generation code to sweep subsystem.
  • Clean up SpendableOutput and CsvSpendableOutput structs.
  • Make Cltv expiry part of a sweep input.
  • Remove duplicated sweep tx generation code from Success and Commit resolvers

A follow PR is #1960, which builds the sweeper around the sweep tx generation and uses that from utxonursery.

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch 3 times, most recently from 49685bf to b569192 Sep 26, 2018

@joostjager joostjager changed the title from Sweeper prep [DO NOT REVIEW] to sweep: prepare for sweeper [DO NOT REVIEW] Sep 26, 2018

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch 11 times, most recently from 9c1cca0 to 158b97a Sep 26, 2018

@joostjager joostjager changed the title from sweep: prepare for sweeper [DO NOT REVIEW] to sweep: prepare for sweeper Sep 27, 2018

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch 3 times, most recently from a168546 to 58c8813 Sep 27, 2018

@Roasbeef

Solid PR! The commit structure makes the set of changes very easy to review. The only comment re the commits, is that we typically like to add additional context to the commits themselves using the message body provided. We find that this helps to make the git history more detailed as each commit has a small body that explains the changes/rationale within it, and may also extend to the commits proceeding it as well (assuming we always work to retain commit order).

@@ -849,10 +849,6 @@ func (bo *breachedOutput) BuildWitness(signer lnwallet.Signer, txn *wire.MsgTx,
return bo.witnessFunc(txn, hashCache, txinIdx)
}
// Add compile-time constraint ensuring breachedOutput implements

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

What's the rationale behind this change? Seems better from the PoV of unit tests, that we can feed in this interface into various methods in his file. After all, we may eventually extract this into the contractcourt package.

This comment has been minimized.

@joostjager

joostjager Oct 3, 2018

Collaborator

I removed the interface because it was not used anywhere. But indeed, I can also see the breacharbiter use an (its own) instance of the sweeper. Converted to Input types.

Signer lnwallet.Signer
}
// NewSweeper returns a new Sweeper instance.

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Can instead be sweep.New as it's the primary external facing struct within the package (atm).

This comment has been minimized.

@joostjager

joostjager Oct 3, 2018

Collaborator

Changed. Not completely sure if I like it, because it'll be another rename when we found out we need another important external facing struct.

Show resolved Hide resolved sweep/sweeper.go Outdated
cfg *SweeperConfig
}
// SweeperConfig contains dependencies of Sweeper

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Missing a period at the end of the sentence.

// Store provides access to and modification of the persistent state
// maintained about the utxo nursery's incubating outputs.
Store NurseryStore
// Sweeper manages the sweeping of unlocked outputs for nursery.

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Well at this point it actually just constructs the sweeping transactions.

return nil, err
}
log.Debugf("%T(%v): using %v sat/kw for sweep tx", c,

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

With this change, we've lost the debug logging here. Also just realized that we don't have a way currently to signal to the sweeper what conf timing, or fee rate that we'd like when sweeping.

This comment has been minimized.

@joostjager

joostjager Oct 3, 2018

Collaborator

Moved logging to sweeper.

This comment has been minimized.

@joostjager

joostjager Oct 3, 2018

Collaborator

Added TODO for configurable fee rate.

This comment has been minimized.

@Roasbeef

Roasbeef Oct 8, 2018

Member

Before we merge we should be able to specify the fee rate. In distinct areas in the codebase, we use 3 or 6 depending on how quickly we want the transaction to enter the chain. Some cases (CLTV timeouts) are more time sensitive than others.

This comment has been minimized.

@joostjager

joostjager Oct 8, 2018

Collaborator

Added

return nil, err
}
log.Debugf("%T(%x): using %v sat/kw to sweep htlc"+

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Similar comment here re losing the debug logging.

// BaseInput contains all the information needed to sweep a output.
type BaseInput struct {
// InputKit contains the base information needed to sweep an output.
type InputKit struct {

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Does this need to be public seeing as it's mostly used as a mix-in struct (via composition) now?

@@ -72,28 +70,61 @@ func MakeBaseInput(outpoint *wire.OutPoint,
}
// Amount returns the number of satoshis contained in the breached output.
func (bi *BaseInput) Amount() btcutil.Amount {
func (bi *InputKit) Amount() btcutil.Amount {

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

I think this might cause vet on Go 1.11 to complain re the name of the method target...

return &bi.signDesc
}
// AbsoluteMaturity returns the absolute timelock.

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Do the absolute and relative maturity need to be a part of this struct? With the changes in this commit, it now serves more or less as the base class (via composition tho), and most "base" inputs don't require a CSV or CLTV semantics at all. Also if we go with my suggestion to always use non-final sequence locks inputs (or even randomize them as eventually we can make all transactions look like commitment transactions being broadcast) and use the next block height as the lock time value.

This comment has been minimized.

@joostjager

joostjager Oct 3, 2018

Collaborator

So you propose:

  • Always set tx locktime to current height + 1
  • Remove AbsoluteMaturity() from the interface completely, as it is no longer needed on the input level
  • Remove relativeMaturity from BaseInput
  • Implement BlockToMaturity of BaseInput as always returning 0
  • Create a new struct CsvInput that allows setting the relative lock time
    ?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 8, 2018

Member

Remove them only on this new baseInput struct. Yes to the rest!

This comment has been minimized.

@joostjager

joostjager Oct 8, 2018

Collaborator

Okay, got it. Didn't create CsvInput yet, as it is not needed in this PR.

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch 9 times, most recently from 22e8545 to d42c27a Oct 3, 2018

@Roasbeef Roasbeef requested a review from cfromknecht Oct 8, 2018

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch 5 times, most recently from 19deaee to 608a9db Oct 8, 2018

// SpendableOutput an interface which can be used by the breach arbiter to
// construct a transaction spending from outputs we control.
type SpendableOutput interface {

This comment has been minimized.

@halseth

halseth Oct 11, 2018

Collaborator

Nice commit structure 👍

Show resolved Hide resolved breacharbiter.go Outdated
Show resolved Hide resolved server.go
Show resolved Hide resolved server.go Outdated
Show resolved Hide resolved sweep/log.go Outdated
// MakeBaseInput assembles a new BaseInput that can be used to construct a
// sweep transaction.
func MakeBaseInput(outpoint *wire.OutPoint,
func makeInputKit(outpoint *wire.OutPoint,

This comment has been minimized.

@halseth

halseth Oct 11, 2018

Collaborator

nit: can remove makeInputKit and just populate the struct manually where needed?

This comment has been minimized.

@joostjager

joostjager Oct 11, 2018

Collaborator

First I didn't like the suggestion, because I'd need to duplicate the amount assignment in makeInputKit. But then I realized that whole field is redundant. Removed it completely.

Show resolved Hide resolved breacharbiter.go
Show resolved Hide resolved breacharbiter.go
// spends from them. This method also makes an accurate fee estimate before
// generating the required witnesses.
//
// The value of currentBlockHeight argument will be set as the tx locktime. This

This comment has been minimized.

@halseth

halseth Oct 11, 2018

Collaborator

Should the parameter be called locktime instead? Since we are currently not passing the correct block height.

This comment has been minimized.

@joostjager

joostjager Oct 11, 2018

Collaborator

The expected value is currentBlockHeight, otherwise the comments would not be valid. Because we are currently not passing the block height in the contract resolvers, I added todo comments there.

sweepTx.AddTxIn(&wire.TxIn{
PreviousOutPoint: *input.OutPoint(),
Sequence: input.BlocksToMaturity(),
})
}
for _, input := range cltvInputs {
sweepTx.AddTxIn(&wire.TxIn{
PreviousOutPoint: *input.OutPoint(),

This comment has been minimized.

@halseth

halseth Oct 11, 2018

Collaborator

I think it is only set FF if NewTxIn is used, so I don't think this will change the existing behavior.

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch 2 times, most recently from d0aebc4 to 121d030 Oct 11, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Oct 11, 2018

Comments addressed.

Show resolved Hide resolved sweep/input.go
Show resolved Hide resolved sweep/input.go
return nil, err
}
log.Debugf("Using %v sat/kw for sweep tx", int64(feePerKw))

This comment has been minimized.

@cfromknecht

cfromknecht Oct 17, 2018

Collaborator

should we consider making this info?

This comment has been minimized.

@joostjager

joostjager Oct 17, 2018

Collaborator

I added a commit that restructures the functions in a imo more logical way and unites the log statements.

joostjager added some commits Sep 21, 2018

utxonursery: move spendable output structs to sweep package
This commit moves the output structs to a new package as a
preparation for moving more logic into that package.
sweep: move sweep tx generation into sweep package
Sweep txes are currently generated in multiple locations. Moving
the sweep tx generation into a shared package will allow us to
reuse that logic.
sweep: create new Input interface
This commit introduces a common interface for sweep
inputs. It eliminates the type checking from UtxoSweeper.

Also the formerly present Amount() getter is removed. It was redundant
because this value is present in SignDesc().Output.Value as well.
cnct: reuse sweep tx logic for commit resolver
Removes duplicate sweep tx code from commit resolver
and delegates generation to UtxoSweeper in the sweep
package.

@joostjager joostjager force-pushed the joostjager:sweeper-prep branch from c431e4b to 95bf858 Oct 17, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Oct 17, 2018

Comments addressed

@Roasbeef

LGTM 🦎

@Roasbeef Roasbeef merged commit c9e42a6 into lightningnetwork:master Oct 17, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.04%) to 55.211%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
)
default:
log.Warnf("kindergarten output in nursery store "+

This comment has been minimized.

@cfromknecht

cfromknecht Oct 17, 2018

Collaborator

i think this is consistent with the current behavior, but currently we won't add any weight estimate for unknown witness types

This comment has been minimized.

@cfromknecht

cfromknecht Oct 17, 2018

Collaborator

i take that back, the prior version would properly filter unknown output types and not include them in the transaction. the new version will include them

weightEstimate.AddWitnessInput(
lnwallet.OfferedHtlcSuccessWitnessSize,
)

This comment has been minimized.

@cfromknecht

cfromknecht Oct 17, 2018

Collaborator

missing cltvCount++

This comment has been minimized.

@cfromknecht

cfromknecht Oct 17, 2018

Collaborator

ah wait, this is a new type that wasn't included before, nvm

}
log.Infof("Creating sweep transaction for %v inputs (%v CSV, %v CLTV) "+
"using %v sat/kw", len(inputs), csvCount, cltvCount,

This comment has been minimized.

@cfromknecht

cfromknecht Oct 17, 2018

Collaborator

first argument should be len(inputs)-csvCount-cltvCount, or len(inputs)-len(csvInputs)-len(cltvOutputs) if getWeightEstimate is modified to return slices of filtered inputs.

@cfromknecht

This comment has been minimized.

Collaborator

cfromknecht commented Oct 17, 2018

Added a small commit to restore prior behavior that filters unknown witness types #2062

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Oct 18, 2018

Oh, good catch. I introduced that already in a commit before that last refactor commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment