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

Fix Fee-Calculation when Selecting Inputs #844

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ziggie1984
Copy link
Contributor

@ziggie1984 ziggie1984 commented Feb 10, 2023

The old NewUnsignedTransaction (txauthor package) fell short when calculating the transaction fees in scenarios where no change output was required. It always calculated the fees as if there would be a change output overpaying and making it impossible to create a transaction which pays for example only for 1 Input and 1 Output.

The new implementation is almost the same as in the github.com/lightningnetwork/lnd/sweep package. It adds an input until the target-amount is satisfied. This leads to a proper fee-calculation.

Things which need to be discussed:

  1. Do we want to deprecate the use of the CoinSelectionStrategy (wallet/wallet.go) and signal the replacement is now in package txauthor: InputSelectionStrategy
  2. Is it sufficient to test NewUnsignedTransaction or should we also test the single functions in the file txSelection.go
  3. Is my idea with a more detailed error to signal the missing amount overkill?

Happy to receive your thoughts. Thanks for your input so far it helped path the way @Torakushi

@ziggie1984
Copy link
Contributor Author

This PR will fix lightningnetwork/lnd#5739

Copy link

@Torakushi Torakushi 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 PR! good job

wallet/txauthor/author.go Outdated Show resolved Hide resolved
// sure the initial add of the first input
// succeeds and does not fail with negative
// yielding.
Value: -int64(targetAmount),

Choose a reason for hiding this comment

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

I understand the logic and should work indeed.
But why not simply have targetAmount and currentAmount (fee included) instead?

I tried a bit and I think it is feasible without a lot of refactoring. The logic will remain the same w/o the negative change output (from the start)

Because having a negative changeOutpoint from the start is kinda misleading (for example totalOutput() function)

Maybe I am missing something!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your thought, could you elaborate more, not sure if we can compare to the currentAmount, because we have to make sure we look at the fees differently, but happy if you can tell me more details how you envisioned it.

I found another solution currently, keeping the changeOutput at 0 but checking in the totalOutput calculation whether we have already some funds in your input array if not returen 0.

// totalOutput is the total amount left for us after paying fees.
//
// NOTE: This might be dust.
func (t *inputState) totalOutput() btcutil.Amount {
	// We return an output amount of 0 so that the first
	// input is added successfully. Otherwise adding the
	// first amount would fails as long its smaller than
	// the target amount.
	if len(t.inputs) == 0 {
		return 0
	}

	return t.targetAmount + btcutil.Amount(t.changeOutpoint.Value)
}

Choose a reason for hiding this comment

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

What I meant is that:

in addToState instead of:

tempInputState.changeOutpoint.Value = int64(tempInputState.inputTotal - tempInputState.targetAmount - fee)

we can have something like

currentAmount = tempInputState.inputTotal - fee(with change) + Check if yield > 0

You can create a new function getChangeOutput (that will be currentAmount-targetAmount) to get the theoretical changeOutput every time you need (to check if dust or not,...).

We can argue however that the changeOutput is still negative 😄 But for the yield, we can only check
currentAmount - currentAmount_old (?)
Finally, that's what you did with the totalOutput because targetAmount is always the same.
So, when you check the yield, you simply check (inputTotal-fee) - (inputTotal-fee)_old AKA currentAmount - currentAmount_old.

Maybe the name currentAmount is misleading and could be something more explicit.

However, I do agree that it could be a good idea to have a behaviour similar to lnd (so using a changeOutput field like you did) but without it being explicitly negative from start. (This logic has proven itself in lnd) 😄

That's why maybe your solution can be a good idea (I haven't check in details your new solution though)

Maybe it's only me that doesn't like the change output being explicitly negative from start! (only to solve a problem when adding the first input)

Copy link

@Torakushi Torakushi Feb 13, 2023

Choose a reason for hiding this comment

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

(you just changed your solution, will check again)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for laying out your reasoning. I think you solution might also be good, to have a kind of a theoretical change. At the end all boils down to the initial addition of the first output.

I think just capping the totalAmount at 0 when there is still no input in our state might be the easier solution regarding the amount of source code haha. But your solution might be more verbose lets see what the other reviewers say and then go from there ?

// RandomSelection means there could still be some inputs
// which are larger than the previous one therefore we
// will loop through all the inputs.
RandomSelection

Choose a reason for hiding this comment

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

I understand, but why not using the already existing CoinSelectionStrategy? (Kinda similar?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we need to generalize it, but I cannot use it from the wallet dir because than I would run into the risk of creating import cycles.

Choose a reason for hiding this comment

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

ahh yes good catch

wallet/txauthor/author.go Outdated Show resolved Hide resolved
@ziggie1984 ziggie1984 force-pushed the input-selection branch 3 times, most recently from fab40bd to 16ea904 Compare February 13, 2023 08:41
@ziggie1984
Copy link
Contributor Author

I also added a new error to explicitly say why the selection failed.
For example when used with lncli which should make creating a transaction which pay exactly 1 input and 1 output way easier to construct.

[lncli] rpc error: code = Unknown desc = wallet couldn't fund PSBT: error creating funding TX: insufficient funds available to construct transaction: amount: 0.0000947 BTC, minimum fee: 0.00000248 BTC, available amount: 0.00009714 BTC

@ziggie1984 ziggie1984 force-pushed the input-selection branch 2 times, most recently from 1272414 to 43599de Compare February 13, 2023 13:27
@ziggie1984 ziggie1984 changed the title [WIP] First Approach in fixing the Fee-Calculation when Selecting Inputs [WIP] Fix Fee-Calculation when Selecting Inputs Feb 13, 2023
@ziggie1984 ziggie1984 changed the title [WIP] Fix Fee-Calculation when Selecting Inputs Fix Fee-Calculation when Selecting Inputs Feb 13, 2023
Copy link

@Torakushi Torakushi left a comment

Choose a reason for hiding this comment

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

It begins to look good!
Essentially nits for this iteration

wallet/createtx.go Outdated Show resolved Hide resolved
wallet/createtx.go Outdated Show resolved Hide resolved
wallet/txauthor/author.go Show resolved Hide resolved
wallet/txauthor/author.go Outdated Show resolved Hide resolved
// InputSourceError is returned.
//
// BUGS: Fee estimation may be off when redeeming non-compressed P2PKH outputs.
// BUGS: Fee estimation is off when redeeming inputs from a uncompressed P2PKH.
// Because one cannot evaluate whether an uncrompressed or compressed public

Choose a reason for hiding this comment

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

Suggested change
// Because one cannot evaluate whether an uncrompressed or compressed public
// Because one cannot evaluate whether an uncompressed or compressed public

Choose a reason for hiding this comment

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

I don't really understand this sentence:

[...] knowing the detailed signature of such an input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to clarify, basically we cannot solve this problem with the new approach, that a bit of a pity. We could only solve the problem when also knowing the ScriptSig at this point, not sure if its worth the effort though, because shouldn't be used that much in the wild?

wallet/txauthor/txselection.go Outdated Show resolved Hide resolved
wallet/txauthor/txselection.go Show resolved Hide resolved
wallet/txauthor/txselection.go Outdated Show resolved Hide resolved
wallet/txauthor/author_test.go Outdated Show resolved Hide resolved
wallet/txauthor/author_test.go Show resolved Hide resolved
func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount,
fetchInputs InputSource, changeSource *ChangeSource) (*AuthoredTx, error) {
inputs []wtxmgr.Credit, selectionStrategy InputSelectionStrategy,

Choose a reason for hiding this comment

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

Question, can the signature change be a breaking change for the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure the NewUnsignedTransaction does not need any signatures for the Inputs, it estimates the ScriptSig/RedeemScript given worst case scenarios for differrent input types. So don't think it would be a problem

Choose a reason for hiding this comment

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

Ah sorry, I was unclear, I meant the function signature :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah that's for sure, hmm I guess we need to introduce a NewUnsignedTransaction and slowly deprecate the old I guess ?

Choose a reason for hiding this comment

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

Let's wait for other reviewers

@ziggie1984 ziggie1984 force-pushed the input-selection branch 2 times, most recently from c047e9a to d44971c Compare February 14, 2023 11:44
@ziggie1984
Copy link
Contributor Author

I am not rebasing on master for now, because seems like this new change by Roasbeef conflicts with the current lnd master branch, meaning that I could not compile my version including the new change with the latest master lnd branch.

cmd/sweepaccount/main.go Outdated Show resolved Hide resolved
func NewUnsignedTransaction(outputs []*wire.TxOut, feeRatePerKb btcutil.Amount,
fetchInputs InputSource, changeSource *ChangeSource) (*AuthoredTx, error) {
inputs []wtxmgr.Credit, selectionStrategy InputSelectionStrategy,

Choose a reason for hiding this comment

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

Let's wait for other reviewers

wallet/txauthor/author.go Outdated Show resolved Hide resolved
destinationSource := makeDestinationScriptSource(rpcClient, opts.DestinationAccount)
// We are only selecting postive yieling outputs.
tx, err := txauthor.NewUnsignedTransaction(nil, opts.FeeRate.Amount,

Choose a reason for hiding this comment

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

Did you test the sweep action to see if you have any regression? (as there is no test for it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

When creating an unsigned transaction inputs are selected
one at a time which allows us creating transactions which
do not overpay fees for change outputs when no change output
is needed.
The new NewUnsignedTransaction function requires all inputs
and the input selection logic for the input selection to work.
The new NewUnsignedTransaction function requires different input
values. Now a constant input slice and the input selection
strategy is needed.
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