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

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

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

Conversation

ziggie1984
Copy link
Collaborator

@ziggie1984 ziggie1984 commented Feb 13, 2023

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 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
Contributor

Torakushi commented Feb 14, 2023

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
Copy link
Collaborator Author

ziggie1984 commented Feb 14, 2023

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 ?

This commit tests the new input selection logic and therefore
fixing the fee calculation for specific input/output combinations.
@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.

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.

psbt funding: cannot create 1 in, 1 out tx
3 participants