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

policy: restrict all TRUC (v3) transactions to 10kvB #29873

Merged
merged 3 commits into from
May 23, 2024

Conversation

glozow
Copy link
Member

@glozow glozow commented Apr 15, 2024

Opening for discussion / conceptual review.

We like the idea of a smaller maximum transaction size because:

  • It lowers potential replacement cost (i.e. harder to do Rule 3 pinning via gigantic transaction)
  • They are easier to bin-pack in block template production
  • They equate to a tighter memory limit in data structures that are bounded by a number of transactions (e.g. orphanage and vExtraTxnForCompact). For example, the current memory bounds for orphanage is 100KvB * 100 = 40MB, and guaranteeing 1 tx per peer would require reserving a pretty large space.

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 = 21050vB EDIT: this is incorrect, but perhaps not something that should affect how we choose this number.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 15, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK instagibbs, sdaftuar, t-bast, murchandamus, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30072 (refactor prep for package rbf by instagibbs)
  • #29496 (policy: bump TX_MAX_STANDARD_VERSION to 3 by glozow)
  • #29325 (consensus: Store transaction nVersion as uint32_t by achow101)
  • #28984 (Cluster size 2 package rbf by instagibbs)

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.

@glozow
Copy link
Member Author

glozow commented Apr 15, 2024

cc @sdaftuar @instagibbs @t-bast

@instagibbs
Copy link
Member

Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

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.

@sdaftuar
Copy link
Member

Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

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.

@ariard
Copy link
Member

ariard commented Apr 16, 2024

Expected size of a commitment transaction is within (900 + 172 * 483 + 224) / 4 = 21050vB

Check BOLT2’s max_accepted_htlcs, it’s 483 HTLC outputs in both directions (inbounds / outbounds), so 25k virtual bytes doesn’t work (even if LN implems use more conservative limits in practice, they’re exposed to lightning node operators).

Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

What is / are TRUCs ?

@glozow glozow mentioned this pull request Apr 16, 2024
56 tasks
@glozow
Copy link
Member Author

glozow commented Apr 16, 2024

Ah so (900 + 172 * 483 * 2 + 224) / 4 = 41819vB?

So maybe a better number is 42KvB or 50KvB.

@t-bast
Copy link
Contributor

t-bast commented Apr 17, 2024

Ah so (900 + 172 * 483 * 2 + 224) / 4 = 41819vB?

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.

@ariard
Copy link
Member

ariard commented Apr 17, 2024

So maybe a better number is 42KvB or 50KvB.

Yes, don’t forget the 2 * for anchor outputs.

Regardless of that, I think you shouldn't care about the size of today's lightning transactions: whenever we start using TRUCs,

Still, what are TRUCs ? Do you have documentation ?

@ariard
Copy link
Member

ariard commented Apr 22, 2024

Still, what are TRUCs ? Do you have documentation ?

TRUC = Topologically Restricted Until Confirmation, see bitcoin/bips#1541

@glozow glozow marked this pull request as ready for review May 1, 2024 16:15
@ariard
Copy link
Member

ariard commented May 2, 2024

Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

For historical lightning channel who have already negotiated parameters above max_accepted_htlcs + witness spend size of legacy channel types. i.e anything before option_anchors_zero_fee_htlc_tx, I think. Legacy v2 commitment transactions are already opting to RBF, so shall be able to be replaced by v3. This is unclear for the splicing flow, where a counterparty could malleate one of the v2 transaction flow above v3 MAX_STANDARD_TX_WEIGHT (no guarantee that one counterparty contributes at least one input and as such sign the splice-in/out).

Is this useful for pinning, given that there are already tighter limits on commitment tx size through max_accepted_htlcs?

Yes if you have sane negotiated max_accepted_htlcs with your channel counterparties. On the other hand, you have a trade-off with off-chain HTLCs per-channel throughput (only considering non-dust HTLCs) as they won’t materialize on the commitment transaction. In practice, I think it’s really rare when LN nodes are reaching a concurrent max_accepted_htlcs of ~10/20 per-channel. It might be better to have a constraining MAX_STANDARD_TX_WEIGHT and have them open more pinning relatively hardened channels to scale their off-chain payment throughput (of course, it incentivizes LN nodes to occupy more space in the UTXO set…trade-offs).

@ajtowns
Copy link
Contributor

ajtowns commented May 16, 2024

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.

@instagibbs
Copy link
Member

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.

@glozow glozow changed the title policy: restrict all TRUC (v3) transactions to 25KvB policy: restrict all TRUC (v3) transactions to 10KvB May 17, 2024
@glozow
Copy link
Member Author

glozow commented May 17, 2024

I've changed the limit to 10KvB. This means the maximum v3 package vsize is 11KvB.

The test_nondefault_package_limits descendant limit test hit a snag (CPFP carve out grants a free +10kvB which makes it impossible to make a descendant limit stricter than 11KvB), so I added a commit to explicitly disable CPFP carve out for v3. This change should be fine since CPFP carveout's intended purpose clearly doesn't apply in the v3 world. Alternatively we can delete this test, but it felt weird that v3 could benefit from CPFP carve out.

@sdaftuar
Copy link
Member

Concept ACK making it 10kvb. Assuming we stick with that, then the commit message in the second commit should be updated (1d95eee).

@ariard
Copy link
Member

ariard commented May 17, 2024

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 max_accepted_htlcs at max 483 both-sides. With no dynamic upgrades deployed, no way to restraint, without force-close and re-open. Worst than pinning exposure is no propagating TRUC transaction due to “naive” upgrade.

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 max_package_kvb (or WU) to be of any practical benefit (still modulo all other safety considerations).

Copy link
Member Author

@glozow glozow left a 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 👍

src/policy/v3_policy.h Show resolved Hide resolved
@murchandamus
Copy link
Contributor

Concept ACK on making the TRUC transaction vsize limit 10 kvB.

Copy link
Contributor

@murchandamus murchandamus left a 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".

src/validation.cpp Outdated Show resolved Hide resolved
test/functional/mempool_accept_v3.py Outdated Show resolved Hide resolved
@DrahtBot DrahtBot requested a review from sdaftuar May 20, 2024 19:41
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.
@glozow glozow changed the title policy: restrict all TRUC (v3) transactions to 10KvB policy: restrict all TRUC (v3) transactions to 10kvB May 21, 2024
@glozow
Copy link
Member Author

glozow commented May 21, 2024

Thanks @murchandamus, addressed nits

Copy link
Member

@instagibbs instagibbs left a 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

src/validation.cpp Show resolved Hide resolved
@t-bast
Copy link
Contributor

t-bast commented May 22, 2024

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.

@sdaftuar
Copy link
Member

ACK 154b2b2

@DrahtBot DrahtBot requested a review from t-bast May 22, 2024 18:06
Copy link
Contributor

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 154b2b2

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

crACK 154b2b2

src/validation.cpp Show resolved Hide resolved
Copy link

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

ack 154b2b2

@achow101
Copy link
Member

ACK 154b2b2

@achow101 achow101 merged commit 867f6af into bitcoin:master May 23, 2024
16 checks passed
@glozow glozow deleted the 2024-04-truc-25k branch May 29, 2024 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants