-
Notifications
You must be signed in to change notification settings - Fork 4
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
Package Relay Draft 1 (Single In-Flight Approach) #8
Changes from all commits
43fd360
f154c34
5d09d97
64fc0da
1f40f8c
3639ad0
a781945
daf6b81
569c284
faf8b23
f9b1993
2d0d4c0
9bfb4a1
87e43a5
2109d36
9bcefd6
0c011bc
e42d8d9
0821236
bcb33bb
718ce4a
60566f4
dce6d8b
2e72630
49aa386
9a65983
2861927
3df9a46
8325214
c24c133
44c3725
e4d7b44
b46bafb
26d7bdc
6023be6
f5b00f3
5c7331b
945814c
3c0e90c
424eb15
25d3fe4
797ed82
f7d6fea
8164ad2
e6656a9
0389892
21acc89
cc800b3
ba5e401
852da8e
fbe58b5
8859429
252ff5a
54c4fbe
efa53b7
d682a10
b647166
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,24 +80,37 @@ test accepts): | |
If any transactions in the package are already in the mempool, they are not submitted again | ||
("deduplicated") and are thus excluded from this calculation. | ||
|
||
To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate | ||
(`-minrelaytxfee`) and the dynamic mempool minimum feerate, the total package feerate is used instead | ||
of the individual feerate. The individual transactions are allowed to be below the feerate | ||
requirements if the package meets the feerate requirements. For example, the parent(s) in the | ||
package can pay no fees but be paid for by the child. | ||
|
||
*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not | ||
meeting minimum fees on its own. This would allow contracting applications to adjust their fees at | ||
broadcast time instead of overshooting or risking becoming stuck or pinned. | ||
|
||
*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as | ||
we do not want a transaction's fees to be double-counted. | ||
To meet the dynamic mempool minimum feerate, i.e., the feerate determined by the transactions | ||
evicted when the mempool reaches capacity (not the static minimum relay feerate), the total package | ||
feerate instead of individual feerate can be used. For example, if the mempool minimum feerate is | ||
5sat/vB and a 1sat/vB parent transaction has a high-feerate child, it may be accepted if | ||
submitted as a package. | ||
|
||
*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a presigned | ||
transaction (i.e. in which a replacement transaction with a higher fee cannot be signed) being | ||
rejected from the mempool when transaction volume is high and the mempool minimum feerate rises. | ||
|
||
Note: Package feerate cannot be used to meet the minimum relay feerate (`-minrelaytxfee`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the state of the discussion about modifying or removing From my understanding, With current So I’m thinking if a “wildcard” could be granted to nversion=3 transactions where the parent transaction can be any feeerate and this compensated by the CPFP ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we are planning to remove Yes, the plan is to allow v3 transactions to be 0 fee when they are cpfp'd |
||
requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is | ||
set to 5satvB, a 1sat/vB parent transaction with a high-feerate child will not be accepted, even if | ||
submitted as a package. | ||
|
||
*Rationale*: Avoid situations in which the mempool contains non-bumped transactions below min relay | ||
feerate (which we consider to have pay 0 fees and thus receiving free relay). While package | ||
submission would ensure these transactions are bumped at the time of entry, it is not guaranteed | ||
that the transaction will always be bumped. For example, a later transaction could replace the | ||
fee-bumping child without still bumping the parent. These no-longer-bumped transactions should be | ||
removed during a replacement, but we do not have a DoS-resistant way of removing them or enforcing a | ||
limit on their quantity. Instead, prevent their entry into the mempool. | ||
|
||
Implementation Note: Transactions within a package are always validated individually first, and | ||
package validation is used for the transactions that failed. Since package feerate is only | ||
calculated using transactions that are not in the mempool, this implementation detail affects the | ||
outcome of package validation. | ||
|
||
*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as | ||
we do not want a transaction's fees to be double-counted. | ||
|
||
*Rationale*: Packages are intended for incentive-compatible fee-bumping: transaction B is a | ||
"legitimate" fee-bump for transaction A only if B is a descendant of A and has a *higher* feerate | ||
than A. We want to prevent "parents pay for children" behavior; fees of parents should not help | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active | |
} | ||
uint64_t num; | ||
file >> num; | ||
LOCK2(cs_main, pool.cs); | ||
while (num) { | ||
--num; | ||
CTransactionRef tx; | ||
|
@@ -77,8 +78,12 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active | |
pool.PrioritiseTransaction(tx->GetHash(), amountdelta); | ||
} | ||
if (nTime > TicksSinceEpoch<std::chrono::seconds>(now - pool.m_expiry)) { | ||
LOCK(cs_main); | ||
const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false); | ||
// Use bypass_limits=true to skip feerate checks, and call TrimToSize() at the very | ||
// end. This means the mempool may temporarily exceed its maximum capacity. However, | ||
// this means fee-bumped transactions are persisted, and the resulting mempool | ||
// minimum feerate is not dependent on the order in which transactions are loaded | ||
// from disk. | ||
const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/true, /*test_accept=*/false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my understanding, if you shutdown at block |
||
if (accepted.m_result_type == MempoolAcceptResult::ResultType::VALID) { | ||
++count; | ||
} else { | ||
|
@@ -104,6 +109,13 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active | |
for (const auto& i : mapDeltas) { | ||
pool.PrioritiseTransaction(i.first, i.second); | ||
} | ||
const auto size_before_trim{pool.size()}; | ||
// Ensure the maximum memory limits are ultimately enforced and any transactions below | ||
// minimum feerates are evicted, since bypass_limits was set to true during ATMP calls. | ||
pool.TrimToSize(pool.m_max_size_bytes); | ||
const auto num_evicted{size_before_trim - pool.size()}; | ||
count -= num_evicted; | ||
failed += num_evicted; | ||
|
||
std::set<uint256> unbroadcast_txids; | ||
file >> unbroadcast_txids; | ||
|
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.
I think this new “CPFP within a package” ability might alter pinning considerations on legacy channels (i.e before
option_anchor_outputs
is still the default on CLN iirc and at least on LDK).The
to_remote
output format on legacy commitment transaction is a P2WPKH. There is no 1 CSV as with anchor output. As soon as the local commitment transaction is broadcast on the transaction-relay network, the counterparty can attach a CPFP to amplify the propagation probabilities of the local commitment transaction. This CPFP attachment might be not be seen by the broadcaster of the local commitment transaction. This CPFP can be used as a pinning child vector (e.g rule 3 on absolute fee). The broadcast of a local commitment transaction can be be trigger by a malicious counterparty by refusing anupdate_fee
even in the lack of pending HTLC outputs.So from my understanding the deployment of a “CPFP within a package” might enable some new pinning scenarios against legacy channels (even when there is no pending HTLC outputs) ? I think in term of severity those scenarios are equivalent where the pinnings are trigger by a counterparty and there are pending HTLC outputs. Note there is no currently standardized or deplyed channel upgrade mechanism to uplift legacy channel types to newer anchor output.
Independently of pinnings, I wonder if those low-feerate commitment transactions carried on by a “CPFP within a package” could be leveraged to open the way to some miner fee-harvesting attacks ?
We might have to swallow the bullet after all. Though if we have do it, I prefer to do it in knowledge of the opening attack surface, at least if we can land some easy mitigations on the Lightning-side.