-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
policy: restrict all TRUC (v3) transactions to 10kvB #29873
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
without focusing too much on the LN use-case, anytime you wanted to use something larger, you'd have to make sure you do so, f.e. when sweeping a max size commitment tx with penalty path. Basically you'll have to check if your tx is larger than 25k and switch if so. |
Just wanted to add -- I think the case I expect more often would be that users would just create multiple transactions if they had a need to construct a transaction greater than 25kvb (similar to what I think everyone must do today to work around the current 100kvb standardness limit). The additional transaction overhead from constructing four 25kvb transactions instead of one 100kvb transaction seems quite small, so if there's not some clear need for 100kvb transactions I think this would be great to try. Maybe in the long run the network moves towards a lower cap on transaction sizes and this can be a way to test the waters; on the other hand, if we get this wrong and there is demand for bigger TRUC transactions for some use case, we can always relax the limit later if we need to. |
Check BOLT2’s
What is / are TRUCs ? |
Ah so (900 + 172 * 483 * 2 + 224) / 4 = 41819vB? So maybe a better number is 42KvB or 50KvB. |
That's correct, that is the maximum size of a commitment transaction today. This size was chosen to ensure that signatures for all HTLCs could be transmitted in a single 65kB lightning p2p message. But we will likely need to transmit more data when we support PTLCs, which means we'll probably reduce the maximum number of allowed PTLCs (IIRC we'll have to divide the current limits roughly by two). That would reduce the maximum size of a commitment transaction, which is IMO a good thing and would likely work with your proposed maximum of 25KvB! Regardless of that, I think you shouldn't care about the size of today's lightning transactions: whenever we start using TRUCs, we can cut down the maximum number of allowed HTLCs to ensure that the transaction size stays below 25KvB. Lightning should adapt to what's best for bitcoin more generally, and I agree that smaller transactions are desirable. |
Yes, don’t forget the 2 * for anchor outputs.
Still, what are TRUCs ? Do you have documentation ? |
TRUC = Topologically Restricted Until Confirmation, see bitcoin/bips#1541 |
For historical lightning channel who have already negotiated parameters above
Yes if you have sane negotiated |
I think the 100kB value for transaction relay was adopted as follows:
So I don't think there was any deep thought put into that figure. 25kvB sounds fine to me. Given the suggestion in the OP that it's easier to increase the value than decrease it, I'm tempted to suggest it should initially be as small as possible, perhaps as low as 10kvB? However I think increasing it is also likely to be hard in practice -- if someone designs their protocol assuming an attacker can only attach 9.9kvB of garbage to their 100vB tx, then suddenly finding out that an attacker can actually attach 24.9kvB of garbage instead might cause them unexpected problems ("now I have to pay 2.5x as much as I'd planned??") which might break software that had been designed based on the initial parameters. |
From a more general wallet perspective if we're assuming ~110vbytes of overhead per additional transaction required to match an input/output set, picking 10kvB as the new limit vs 100kvB results in 10*110=1,100vb, or ~1% additional overhead vs 100kvB. At 5kvB you'd be receiving ~2% penalty, and so on. FWIW 10kvB also seems like plenty of headroom for practical LN channels with >60-HTLCs-per-direction, but below today's spec limit. |
I've changed the limit to 10KvB. This means the maximum v3 package vsize is 11KvB. The |
Concept ACK making it 10kvb. Assuming we stick with that, then the commit message in the second commit should be updated (1d95eee). |
I think 10kvb is okay as a limit though pray for someone telling it more to LN folks. There is very likely 2018 / 2019 channels open with In the (far) future, 10kvb could be dynamic at the transaction-relay level, though this would assume LN upgrading its channel policy settings first to have something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed commit message, thanks 👍
Concept ACK on making the TRUC transaction vsize limit 10 kvB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 0583aee
Nit in PR title and second commit message:
Uppercase K kilo refers to 1024¹ per the JEDEC standard, the symbol of the metric prefix kilo for 1000¹ is lowercase k. Since you are limiting TRUC transactions to 10,000 vB, I think you mean "kvB" instead of "KvB".
This carve out is intended to allow a second child under restricted circumstances, but this topology is not allowed for v3 transactions. As CPFP carve out does not explicitly require a second child to actually exist, it has the effect of granting a free +10KvB descendant size limit when a single child is enough to bust the descendant limit.
Thanks @murchandamus, addressed nits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 154b2b2
I think a concept re-ack from protocol devs outside this repo is needed for merge
edit: fuzzer ran clean
Concept ACK on restricting to 10kvB. Lightning will need an upgrade path for existing channels to start using TRUC txs, and we're working on several options to upgrade existing channels. We will make sure the upgrade mechanism lowers the maximum number of HTLCs to fit inside the 10kvB limit. |
ACK 154b2b2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 154b2b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK 154b2b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 154b2b2
ACK 154b2b2 |
Opening for discussion / conceptual review.
We like the idea of a smaller maximum transaction size because:
History for
MAX_STANDARD_TX_WEIGHT=100KvB
(copied from #29873 (comment)):Lowering
MAX_STANDARD_TX_WEIGHT
for all txns is not being proposed, as there are existing apps/protocols that rely on large transactions. However, it's been brought up that we should consider this for TRUCs (which is especially designed to avoid Rule 3 pinning).This reduction should be ok because using nVersion=3 isn't standard yet, so this wouldn't break somebody's existing use case. If we find that this is too small, we can always increase it later. Decreasing would be much more difficult.
Expected size of a commitment transaction is within (900 + 172 * 483 + 224) / 4 = 21050vBEDIT: this is incorrect, but perhaps not something that should affect how we choose this number.