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

glozow commented at 11:19 am on April 15, 2024: member

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)):

2010-09-13 In https://github.com/bitcoin/bitcoin/commit/3df62878c3cece15a8921fbbdee7859ee9368768 satoshi added a 100kB (MAX_BLOCK_SIZE_GEN/5 with MBS_GEN = MAX_BLOCK_SIZE/2) limit on new transactions in CreateTransaction()
2013-02-04 #2273 In gavin gave that constant a name, and made it apply to transaction relay as well

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.

glozow added the label TX fees and policy on Apr 15, 2024

DrahtBot commented at 11:19 am on April 15, 2024: contributor

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 added the label Brainstorming on Apr 15, 2024

glozow commented at 11:20 am on April 15, 2024: member

cc @sdaftuar @instagibbs @t-bast

instagibbs commented at 1:54 pm on April 15, 2024: 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 commented at 7:39 pm on April 15, 2024: 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 commented at 1:49 am on April 16, 2024: member

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 commented at 3:18 pm on April 16, 2024: member

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

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

t-bast commented at 3:01 pm on April 17, 2024: contributor

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 commented at 3:02 pm on April 17, 2024: member

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 commented at 9:22 pm on April 22, 2024: member

Still, what are TRUCs ? Do you have documentation ?

TRUC = Topologically Restricted Until Confirmation, see https://github.com/bitcoin/bips/pull/1541

glozow marked this as ready for review on May 1, 2024

ariard commented at 1:38 am on May 2, 2024: member

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 commented at 2:50 pm on May 16, 2024: contributor

I think the 100kB value for transaction relay was adopted as follows:

2010-09-13 In 3df62878c3c satoshi added a 100kB (MAX_BLOCK_SIZE_GEN/5 with MBS_GEN = MAX_BLOCK_SIZE/2) limit on new transactions in CreateTransaction()
2013-02-04 #2273 In gavin gave that constant a name, and made it apply to transaction relay as well

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 commented at 3:59 pm on May 16, 2024: 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 force-pushed on May 17, 2024

glozow renamed this:
~~policy: restrict all TRUC (v3) transactions to 25KvB~~
policy: restrict all TRUC (v3) transactions to 10KvB
on May 17, 2024

glozow commented at 12:41 pm on May 17, 2024: member

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.

in src/policy/v3_policy.h:28 in 1d95eee9eb outdated

23@@ -24,11 +24,13 @@ static constexpr unsigned int V3_DESCENDANT_LIMIT{2};
24 /** Maximum number of transactions including a V3 tx and all its mempool ancestors. */
25 static constexpr unsigned int V3_ANCESTOR_LIMIT{2};
26 
27+/** Maximum sigop-adjusted virtual size of all v3 transactions. */
28+static constexpr int64_t V3_MAX_VSIZE{10000};

instagibbs commented at 1:37 pm on May 17, 2024:

probably can assert this in the fuzz target?

glozow commented at 10:00 am on May 20, 2024:

Added. Ran fuzzer for a bit, seems happy

sdaftuar commented at 2:22 pm on May 17, 2024: member

Concept ACK making it 10kvb. Assuming we stick with that, then the commit message in the second commit should be updated (https://github.com/bitcoin/bitcoin/commit/1d95eee9ebfa17dee3c9cf60fea1c28a2cf5d8b5).

ariard commented at 11:33 pm on May 17, 2024: member

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).

glozow force-pushed on May 20, 2024

glozow commented at 10:00 am on May 20, 2024: member

fixed commit message, thanks :+1:

murchandamus commented at 7:03 pm on May 20, 2024: contributor

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

in src/validation.cpp:957 in e3be8501e5 outdated

952@@ -953,7 +953,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
953         // If the new transaction is relatively small (up to 40k weight)
954         // and has at most one ancestor (ie ancestor limit of 2, including
955         // the new transaction), allow it if its parent has exactly the
956-        // descendant limit descendants.
957+        // descendant limit descendants. The transaction also cannot be v3,
958+        // as its topology resrictions do not allow a second child.

murchandamus commented at 7:06 pm on May 20, 2024:

Typo:

0        // descendant limit descendants. The transaction also cannot be v3,
1        // as its topology restrictions do not allow a second child.

glozow commented at 2:07 pm on May 21, 2024:

done

in test/functional/mempool_accept_v3.py:233 in 74b5e05576 outdated

232+            utxo_to_spend=tx_v3_parent_large1["new_utxo"],
233+            target_weight=child_target_weight,
234+            version=3
235+        )
236+
237+        # Parent and child are within v3 limits, but parent's descendant limit is exceeded

murchandamus commented at 7:20 pm on May 20, 2024:

I don’t understand the test here. My understanding was that TRUC transactions would be generally limited to 10 kvB, and TRUC transactions with a parent would be limited to 1 kvB. It seems to me that they should together be allowed to weigh up to 11 kvB, so it is no clear to me why this comment reads “but parent’s descendant limit is exceeded”.

Perhaps clarify here in the comment that there is a custom descendant limit in play:

0        # Parent and child are within v3 limits, but the reduced descendant limit is exceeded for the parent

glozow commented at 2:07 pm on May 21, 2024:

done

murchandamus commented at 7:40 pm on May 20, 2024: contributor

ACK 0583aeef4f2b09f35547f95f1fe21dc14738b65d

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”.

DrahtBot requested review from sdaftuar on May 20, 2024

[policy] explicitly require non-v3 for CPFP carve out

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.

d578e2e354

[policy] restrict all v3 transactions to 10kvB a29f1df289

[fuzz] V3_MAX_VSIZE and effective ancestor/descendant size limits 154b2b2296

glozow force-pushed on May 21, 2024

glozow renamed this:
~~policy: restrict all TRUC (v3) transactions to 10KvB~~
policy: restrict all TRUC (v3) transactions to 10kvB
on May 21, 2024

glozow commented at 2:08 pm on May 21, 2024: member

Thanks @murchandamus, addressed nits

glozow requested review from murchandamus on May 22, 2024

glozow requested review from instagibbs on May 22, 2024

in src/validation.cpp:971 in d578e2e354 outdated

967@@ -967,7 +968,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
968             .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,
969         };
970         const auto error_message{util::ErrorString(ancestors).original};
971-        if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) {
972+        if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->nVersion == 3) {

instagibbs commented at 2:41 pm on May 22, 2024:

wondering if we should have a constant ala TX_TRUC_VERSION that is easily greppable? Exceptions being sprinkled across the codebase like this will be harder to track

glozow commented at 4:34 pm on May 22, 2024:

Ah that’s a good idea. I can add it to #29496?

instagibbs commented at 4:36 pm on May 22, 2024:

sgtm

instagibbs approved

instagibbs commented at 3:16 pm on May 22, 2024: member

ACK 154b2b2296edccb5ed24e829798dacb6195edc11

I think a concept re-ack from protocol devs outside this repo is needed for merge

edit: fuzzer ran clean

t-bast commented at 3:46 pm on May 22, 2024: contributor

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 commented at 6:06 pm on May 22, 2024: member

ACK 154b2b2296edccb5ed24e829798dacb6195edc11

DrahtBot requested review from t-bast on May 22, 2024

glozow removed the label Brainstorming on May 23, 2024

t-bast approved

t-bast commented at 8:44 am on May 23, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/commit/154b2b2296edccb5ed24e829798dacb6195edc11

in src/validation.cpp:968 in d578e2e354 outdated

967@@ -967,7 +968,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
968             .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,

murchandamus commented at 2:10 pm on May 23, 2024:

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.

That sounds like an unintentional behavior. Should that be addressed with a bug fix, or can we just get rid of the carve-out altogether in a world where we have TRUC transactions?

glozow commented at 2:32 pm on May 23, 2024:

The first commit gets rid of it for TRUC, and it’ll go away with cluster mempool.

I don’t really think a fix in the interim is worth it; it’s slightly involved to pull all the ancestors and check that their descendant count exceeds 1. I’d rather focus on getting rid of it tbh.