Skip to content

[Version bump] Fix fee calculation when funding a psbt. #7404

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[Version bump] Fix fee calculation when funding a psbt. #7404

wants to merge 1 commit into from

Conversation

ziggie1984
Copy link
Collaborator

@ziggie1984 ziggie1984 commented Feb 13, 2023 •
edited
Loading

Change Description

This PR tests the changes made in btcsuite/btcwallet#844 to make sure the lnd test suite passes.

The new versions of the btcwallet and the txauthor fix #5739

How to test the new funding for example on simnet (now we get an error when the available amount is too low which lets us construct a transaction which creates no change output:

slncli wallet psbt fund --sat_per_vbyte 1 --outputs='{"sb1pw75fhwyp55ad0ejxr8y65qxptn8af56z4956hhakn62nyy2gnars5dn4w2":9766 }'
[lncli] rpc error: code = Unknown desc = wallet couldn't fund PSBT: error creating funding TX: insufficient funds available to construct transaction: amount: 0.00009766 BTC, minimum fee: 0.00000112 BTC, available amount: 0.00009466 BTC

Available funding is 9466 - 112 (fee without a change output using the required feerate) = 9354 sathoshis

slncli wallet psbt fund --sat_per_vbyte 1 --outputs='{"sb1pw75fhwyp55ad0ejxr8y65qxptn8af56z4956hhakn62nyy2gnars5dn4w2":9354 }'
{
	"psbt": "cHNidP8BAF4CAAAAASTr+kWSpegHh9KXR7W4wt0SDsaVBdk1S9QWZg/T/c18AAAAAAD/////AYokAAAAAAAAIlEgd6ibuIGlOtfmRhnJqgDBXM/U00Kpaavftp6VMhFIn0cAAAAAAAEBK/okAAAAAAAAIlEgd6ibuIGlOtfmRhnJqgDBXM/U00Kpaavftp6VMhFIn0ciBgM+0TvBiXTKiXVW2IEU1AH2JW+sF33B8di2nNnCmxQBehgAAAAAVgAAgAAAAIAAAACAAAAAAAIAAAAhFj7RO8GJdMqJdVbYgRTUAfYlb6wXfcHx2Lac2cKbFAF6GQAAAAAAVgAAgAAAAIAAAACAAAAAAAIAAAAAAA==",
	"change_output_index": -1,
	"locks": [
		{
			"id": "ede19a92ed321a4705f8a1cccc1d4f6182545d4bb4fae08bd5937831b7e38f98",
			"outpoint": "7ccdfdd30f6616d44b35d90595c60e12ddc2b8b54797d28707e8a59245faeb24:0",
			"expiration": 1676303781,
			"pk_script": "512077a89bb881a53ad7e64619c9aa00c15ccfd4d342a969abdfb69e953211489f47",
			"value": 9466
		}
	]
}

slncli wallet psbt finalize cHNidP8BAF4CAAAAASTr+kWSpegHh9KXR7W4wt0SDsaVBdk1S9QWZg/T/c18AAAAAAD/////AYokAAAAAAAAIlEgd6ibuIGlOtfmRhnJqgDBXM/U00Kpaavftp6VMhFIn0cAAAAAAAEBK/okAAAAAAAAIlEgd6ibuIGlOtfmRhnJqgDBXM/U00Kpaavftp6VMhFIn0ciBgM+0TvBiXTKiXVW2IEU1AH2JW+sF33B8di2nNnCmxQBehgAAAAAVgAAgAAAAIAAAACAAAAAAAIAAAAhFj7RO8GJdMqJdVbYgRTUAfYlb6wXfcHx2Lac2cKbFAF6GQAAAAAAVgAAgAAAAIAAAACAAAAAAAIAAAAAAA==
{
	"psbt": "cHNidP8BAF4CAAAAASTr+kWSpegHh9KXR7W4wt0SDsaVBdk1S9QWZg/T/c18AAAAAAD/////AYokAAAAAAAAIlEgd6ibuIGlOtfmRhnJqgDBXM/U00Kpaavftp6VMhFIn0cAAAAAAAEBK/okAAAAAAAAIlEgd6ibuIGlOtfmRhnJqgDBXM/U00Kpaavftp6VMhFIn0cBCEIBQLurtloc/vyInUBrJwwsvrcbIa2Zm7l12l4J+7RJ/iFxr6SGqPUTzC1DcU+b4FoAae45at+SIwMTfM4s/FRpvnEAAA==",
	"final_tx": "0200000000010124ebfa4592a5e80787d29747b5b8c2dd120ec69505d9354bd416660fd3fdcd7c0000000000ffffffff018a2400000000000022512077a89bb881a53ad7e64619c9aa00c15ccfd4d342a969abdfb69e953211489f470140bbabb65a1cfefc889d406b270c2cbeb71b21ad999bb975da5e09fbb449fe2171afa486a8f513cc2d43714f9be05a0069ee396adf922303137cce2cfc5469be7100000000"
}

The resulting tx pays exactly 112 sats, and is off by 1 sat (virtualsize is 111) because we always account for the sighash byte in the taproot spend although the default has none. But I think 1 sat off is not that bad :)

@ziggie1984 ziggie1984 force-pushed the input-selection-btcwallet branch from 37cfb26 to 4d43a61 Compare February 13, 2023 15:10
@ziggie1984 ziggie1984 changed the title Fix fee calculation when funding a psbt. [Version bump] Fix fee calculation when funding a psbt. Feb 13, 2023
@Torakushi
Copy link

Torakushi commented Feb 14, 2023 •
edited
Loading

I've tested (especially with the example provided in #5739) and it worked like a charm 👍

The only thing I don't understand is your 112 in your example (shouldn't be 110?)

EDIT: didn't see that it's a 1-1 p2tr so should be 111

@ziggie1984 ziggie1984 force-pushed the input-selection-btcwallet branch from 4d43a61 to 90594db Compare February 14, 2023 11:06
@ziggie1984
Copy link
Collaborator Author

ziggie1984 commented Feb 14, 2023 •
edited
Loading

Just to clarify why 112 and not 111. I think studying this let me to a bug in the size calculation.

Basically we account for 2 additional weight-units, one here size of witness elements this shouldn't be here because every Witness Input already accounts for its number of elements: number of items. But only this one would also lead to 112. Moreover we account for an additional sighhash byte in the P2TR Weight, this I find not necessary because it was left out exactly to save weight but also fees (IMO). additional sighash wu

Thos 2 additonal weight units lead to 1vbyte more according to estimate

I think we should also fix this too, what are your opinions ?

@ziggie1984
mod: adding new btcwallet and txauthor version
65f079f 
This commit tests the new input selection logic and therefore
fixing the fee calculation for specific input/output combinations.
@ziggie1984 ziggie1984 force-pushed the input-selection-btcwallet branch from 90594db to 65f079f Compare February 16, 2023 14:23
@jharveyb
Copy link
Contributor

Just to clarify why 112 and not 111. I think studying this let me to a bug in the size calculation.

Basically we account for 2 additional weight-units, one here size of witness elements this shouldn't be here because every Witness Input already accounts for its number of elements: number of items. But only this one would also lead to 112. Moreover we account for an additional sighhash byte in the P2TR Weight, this I find not necessary because it was left out exactly to save weight but also fees (IMO). additional sighash wu

Thos 2 additonal weight units lead to 1vbyte more according to estimate

I think we should also fix this too, what are your opinions ?

I think this estimate of 66 wu for a P2TR input is correct, because EstimateVirtualSize intends to make a worst-case size estimate.

The P2TR input would only be 65 wu when using SIGHASH_DEFAULT, as explained in the BIP. So EstimateVirtualSize accounts for the extra wu that would be used by a non-default sighash flag. This leads to the overestimate for P2TR inputs using SIGHASH_DEFAULT, but as a worst-case estimate the value is correct.

@alexbosworth alexbosworth mentioned this pull request Jan 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

psbt funding: cannot create 1 in, 1 out tx
3 participants
@ziggie1984 @Torakushi @jharveyb